Suspended coroutines can be garbage-collected when listener list is weak

[elizarov]

Consider the following hypothetical object that keeps a list of weak references to attached listeners:

object Foo {
    private val listeners = CopyOnWriteArrayList<WeakReference<() -> Unit>>()
    fun addListener(listener: () -> Unit) = listeners.add(WeakReference(listener))
    fun removeListener(listener: () -> Unit) = listeners.removeIf {  it.get() === listener }
    fun fire() = listeners.forEach { it.get()?.invoke() ?: println("Gone!") }
}

We can wrap this callback-based interface into a suspending function using suspendCancellableCoroutine like this:

suspend fun foo() = suspendCancellableCoroutine<Unit> { cont ->
    val listener = { cont.resume(Unit) }
    Foo.addListener(listener)
    cont.invokeOnCancellation { Foo.removeListener(listener) }
}

Now let us write the following test code that invokes GC before firing a listener:

fun main() = runBlocking {
    launch {
        delay(1000)
        System.gc()
        Foo.fire()
    }
    println("Suspending")
    foo() // (1)
    println("Resumed")
}

It works as expected. It prints:

Suspending
Resumed

and exits. However, if we replace line (1) with withContext(NonCancellable) { foo() } then it prints:

Suspending
Gone!

and never exits, because a reference to listener was garbage-collected, because in NonCancellable context there is no strong reference from a parent coroutine to prevent garbage collection. This could be accepted in launch(NonCancellable) { ... } (to be discussed), but is definitely puzzling and unexpected in withContext(NonCancellable) { ... }.

See also discussion here: https://youtrack.jetbrains.com/issue/KT-30588

[LouisCAD]

NonCancellable could be tweaked to keep references to its children while they are not completed, and I can work on a PR for that (let me know if I should).

Not sure about GlobalScope and custom scopes though.

[elizarov]

NonCancellable cannot be tweaked this way. It is a global object and keeping a list of its children creates a shared global mutable state. We can maybe somehow tweak withContext, though.

[LouisCAD]

Wouldn't the issue persist when using launch, async and a custom scope with NonCancellable?

[elizarov]

To clarify, this issue should reproduce with:

1. withContext(NonCancellable). It seems that it could be fixed.
2. myScope.launch/async/etc(NonCancellable). It seems that it could be fixed.
3. CoroutineScope(NonCancellabe).launch/async/etc. This cannot be satisfyingly fixed, due the fact that NonCancellable is a global singleton object. However, we can (maybe?) deprecate and forbid such coroutine scopes altogether, as they do not seem to have any use-case.

[fvasco]

@elizarov this example can be minified discarding Foo and replacing foo with

suspend fun foo() = suspendCancellableCoroutine<Unit> { cont -> }

This is a simpler way to trash cont. To detect this issue it is possible to register the cont in a java.lang.ref.Cleaner and invoke resumeWith in case of GC (or use legacy finalize method instead of a Cleaner).

Is this issue bounded to NonCancellable context?

[elizarov]

Cleaners are finalizers in disguise. Any design that has to rely on them is inherently broken. We will not even start going down that path.

P.S. I used Foo in my code to visually demonstrate that the continuation was garbage-collected. Otherwise, you cannot see if the object you've lost reference to was garbage-collected or not.

[fvasco]

Have NonCancellable to be a singleton? Probably yes, to avoid breaking change. At same time it does not respect the Job contract. Should NonCancellable be deprecated?

[elizarov]

Deprecating NonCancellable is a possible option. We can recommend its replacement with Job(), however it's idiomatic usage will not be as self-explanatory. Compare:

withContext(NonCancellable) {
    // it is clear that we are in a non-cancellable block
}

with:

withContext(Job()) {
    // effect is the same, but it is harder to understand what's going on.
    // plus it is less efficient -- it creates one extra Job instance.
}

[fvasco]

val NonCancellable get() = Job() // really bad trick :)

However whould be nice to have the construct

try {
    // cancellable
} catch (e: Exception) {
    // non cancellable
} finally {
    // non cancellable
}

NonCancellable is not the goal, in my opinion.

[fvasco]

withContext(Job()) { // effect is the same, but it is harder to understand what's going on. // plus it is less efficient -- it creates one extra Job instance. }

This does not fix launch(NonCancellable), I propose to define a NonCancellableJob(), this should avoid confusion and errors.

Regarding performance, I consider the KT-16222 a bigger issue for my use case (already posted here https://discuss.kotlinlang.org/t/coroutine-memory-management-issue/10089), unfortunately a long server workload is not measured in micro profiling.

[LouisCAD]

Context, though.

NonCancellable cannot be tweaked this way. It is a global object and keeping a list of its children creates a shared global mutable state. We can maybe somehow tweak withContext, though.

@elizarov @qwwdfsad Is this still a problem, now that Kotlin/Native has a garbage collector?

[qwwdfsad]

It is still a problem: shared mutable state for the whole app is the problem by itself; K/N GC back then was rather an obstacle than a blocker.

Regarding the original problem, we are in the process of figuring out whether we can provide a universal API layer that will cover this issue along with other https://github.com/Kotlin/kotlinx.coroutines/labels/structured%20concurrency issues