Release coroutine block after lazy start

[1zaman]

When starting a coroutine lazily by using CoroutineStart.LAZY, it creates a Continuation that would launch the coroutine block, which is stored in the returned Job or Deferred to be invoked lazily. However, this reference is never cleared, even after the coroutine ends.

This prevents the coroutine block and it's enclosing class instance from being garbage collected if the Job or Deferred is retained in another class, or while child coroutines are running (as they also hold a reference to the Job).

Fixed by clearing the reference to the Continuation after invoking it.

[fvasco]

You should consider the try-finally block, or set continuation to null just before its use (using a temporary variable).

I already fixed this issue some time ago 🙄 #767

[1zaman]

Thanks for the suggestion, @fvasco. I have changed the code to use your approach now. Looks like it was reverted in #1548.

[fvasco]

Thank you for your effort.

Personal opinion: an exclamation mark at the end of the message does not add clarity, it is just an extra byte (or 2-bytes). Short messages do not need punctuation.

[elizarov]

It needs a use-case (a practical example where this code matters) and a test (we have FieldWalker infrastructure for writing tests on this kind of features) as the previous example of @fvasco shows that it will not survive for too long without a test.

[fvasco]

Hi, @elizarov,

It needs a use-case

I consider to wipe references a best-practice and this PR is able to detect double start issue (with a bit of luck).

Coroutine life-time is undefined and its memory requirement isn't constrained.
In our project we use a lot coroutines, they are "cheap" and we assume that they are so. We don't enjoy to waste our time to spot well-known memory leaks.
Many of them are alive for many days, bug like KT-16222 affects our production environment, so we need more memory and more CPU for GC cycles.
KT-16222 isn't cool like pattern-matching, but it affects our production cost, I hope to don't worry about memory requirements of each lazy coroutine block.

[1zaman]

I don't have a specific use-case at the moment that would be significantly affected by this. However, as @fvasco mentioned, it's good to release objects once they're no longer needed. Also, the memory implications of async(start = CoroutineStart.LAZY) is currently different from async with some other start value, which might be surprising to some users.

I tried to write some tests using FieldWalker, but it doesn't seem to be able to find the reference to the block, even when creating the coroutine Continuation and checking it directly. See the following test, which is failing:

@Test
fun testBlockIsReachableInCoroutineStartingContinuation() = runTest {
    val block = suspend {}
    val continuation = block.createCoroutineUnintercepted(object : Continuation<Unit> {
        override val context = EmptyCoroutineContext
        override fun resumeWith(result: Result<Unit>) {}
    })
    FieldWalker.assertReachableCount(1, continuation) { it === block }
}

This fails with java.lang.AssertionError: Unexpected number objects. Expected 1, found 0 expected:<1> but was:<0>

[qwwdfsad]

@fvasco KT-16222 is indeed a bummer and we are aiming to fix it soon.

Does this cleanup actually solves application-specific memory leaks or are you concerned that the absence of such cleanup may trigger memory-leaks in the future?

[fvasco]

Thank you @qwwdfsad,
I am watching KT-16222, it is an issue for us.

Regarding this one, we not profiled our server recently, so it is only a threat.

the absence of such cleanup may trigger memory-leaks in the future

I don't agree with this statement: the code always triggers a memory leak, it is not possible to avoid it. I don't think that it is productive to discuss how big it can be.

[qwwdfsad]

Thanks! Superseded with #2511