fix: handle cancelled scope and empty flow in Flow.stateIn

[francescotescari]

Fix for #4322

[francescotescari]

Notice: I think we shouldn't cancel the collecting scope here, as there is nothing wrong with the flow collection itself, the exception should be limited to stateIn IMHO.

[dkhalanskyjb]

Yes, good point, let's not cancel the collecting scope. This is tricky to do ensure cleanly, though! Here's a slightly convoluted solution:

    /* If `scope`'s job is cancelled, both `deferredParentJob` and the deferred will be cancelled.
    If the deferred is cancelled, this will not cancel `deferredParentJob` (as it's a `SupervisorJob`),
    and `CoroutineExceptionHandler` present in `scope` (if any) won't get invoked,
    as `Deferred` values are themselves responsible for propagating their exceptions. */
    val deferredParentJob = SupervisorJob(scope.coroutineContext[Job])
    val result = CompletableDeferred<StateFlow<T>>(deferredParentJob)
    scope.launchSharingDeferred(config.context, config.upstream, result)
    return try {
        result.await()
    } finally {
        deferredParentJob.cancel()
    }

then the modified test is:

    @Test
    fun testThrowsNoSuchElementExceptionOnEmptyFlow() = runTest {
        val flow = emptyFlow<Any>()
        assertFailsWith<NoSuchElementException> {
            flow.stateIn(this)
        }
        assertEquals(true, coroutineContext[Job]?.isActive)
        coroutineContext.cancelChildren()
    }

(Note this runTest is not kotlinx.coroutines.test.runTest!) If the main coroutine of runTest gets cancelled, the whole test fails; also, if there are any uncaught exceptions in child coroutines of runTest, they do get reported at the end of the test. So, this test does check that NoSuchElementException doesn't propagate upwards.

The only other option I see is to rely on our internal low-level job.invokeOnCompletion function to avoid the parent-child relationship between scope and result altogether. After implementing that option, I don't like how it recreates parts of child cancellation logic. This looks like a weird micro-optimization.

[francescotescari]

Another simpler (not elegant) trick is to make CompletableDeferred of type CompletableDeferred<Result<StateFlow<T>>> and complete the deferred successfully but with Result.failure and unpack with getOrThrow it stateIn side.

[dkhalanskyjb]

Good idea!

[dkhalanskyjb]

@francescotescari, would you like to implement this, or should I do it?

[francescotescari]

Sorry for the delay, addressed it now.
LMK if it looks good to you 🙏

[dkhalanskyjb]

Thank you both for the bug report and a well-thought-out fix!