Memory leak in Signal#discrete

[mpilquist]

This program should run in constant memory but it doesn't:

object SignalDiscreteSanityTest extends App {
  import TestUtil.S
  Stream.eval(async.signalOf[Task, Unit](())).flatMap { signal =>
    signal.discrete.evalMap(a => signal.set(a))
  }.run.unsafeRun
}

Based on some quick heap analysis, it looks like we are leaking resource cleanup -- e.g., a huge LongMap, a ton of Tokens, etc.

Each element through discrete results in a new Stream.bracket. I suspect the finalizers are getting promoted to higher scope due to the async effect of signal.set.

Note: same test using continuous runs in constant memory:

object SignalContinuousSanityTest extends App {
  import TestUtil.S
  Stream.eval(async.signalOf[Task, Unit](())).flatMap { signal =>
    signal.continuous.evalMap(a => signal.set(a))
  }.run.unsafeRun
}

[mpilquist]

BTW, I'm pretty sure this is the cause of the memory leak in time.awakeEvery reported by @AusCoder in Gitter

[pchlupacek]

https://github.com/functional-streams-for-scala/fs2/blob/series/1.0/core/shared/src/main/scala/fs2/async/mutable/Signal.scala#L106 is definitelly recursing the bracket pilling up cleanups

[pchlupacek]

I think we will need to create proper Tag for the discrete subscriber and cleanup registration from the outer scope of go.

[pchiusano]

@pchlupacek yup we just spotted the same thing... figuring out fix

[pchlupacek]

@pchiusano @mpilquist just for curiosity any progress on that? Eventually, I can tackle this, as we are about to move part of our production to fs2, with heavy signal usage. lmk pls. I would like also make 9.3 with this patch, or if there is something else worth of backporting from 1.0 lmk

[mpilquist]

I haven't made any progress (was busy with work stuff yesterday). I definitely agree we need a 0.9.3 with this fix. I'll look through the 1.0 commits and see what can be backported.

[pchlupacek]

lmk if you want to fix that or I shall do it. My approach will be to introduce Id/Tag of discrete subscriber and clean it on bracket finalisation, instead on every step. Any thoughts?

[mpilquist]

Sure, have at it.

[pchiusano]

Yeah, go for it... sorry I also didn't get to this yesterday.

…

On Thu, Feb 9, 2017 at 9:17 AM, Michael Pilquist ***@***.***> wrote: Sure, have at it. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#799 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAArQnRFfuH-dA0W2bWJo808WXwuuK6gks5rayAVgaJpZM4L67XU> .

[pchlupacek]

@mpilquist, @pchiusano see #802 with backport in #803

[pchlupacek]

also threw in Traverse improvements, hope you won't mind

[mpilquist]

Fixed in 0.9.3 and 1.0.0.