Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TimeoutCancellationException is thrown inconsistently between flatMapConcat and flatMapLatest #3392

Open
rossdanderson opened this issue Aug 5, 2022 · 4 comments
Labels
docs KDoc and API reference flow

Comments

@rossdanderson
Copy link

rossdanderson commented Aug 5, 2022

In the code below, the TimeoutCancellationException thrown by withTimeout is propagated as expected when using flatMapConcat, but not propagated when using flatMapLatest.
If withTimeout is replaced with check(false), the exception is propagated as expected in both.

import kotlinx.coroutines.delay
import kotlinx.coroutines.flow.*
import kotlinx.coroutines.runBlocking
import kotlinx.coroutines.withTimeout

val flow = flowOf("A", "B")

runBlocking {
    var threw1 = false
    try {
        flow
            .flatMapConcat {
                callbackFlow<String> {
                    withTimeout(100) { delay(1000) }
                }
            }
            .collect()
    } catch (t: Throwable) {
        threw1 = true
        println("Error1 $t")
    }

    var threw2 = false
    try {
        flow
            .flatMapLatest {
                callbackFlow<String> {
                    withTimeout(100) { delay(1000) }
                }
            }
            .collect()
    } catch (t: Throwable) {
        threw2 = true
        println("Error2 $t")
    }

    if (!threw1) println("flatMapConcat did NOT throw")
    if (!threw2) println("flatMapLatest did NOT throw")
}

Produces:

Error1 kotlinx.coroutines.TimeoutCancellationException: Timed out waiting for 100 ms
flatMapLatest did NOT throw
@rossdanderson rossdanderson changed the title withTimeout exception is thrown inconsistently between flatMapConcat and flatMapLatest TimeoutCancellationException is thrown inconsistently between flatMapConcat and flatMapLatest Aug 5, 2022
@rossdanderson
Copy link
Author

Digging deeper and I'm not sure whether this is an issue or not - or the right issue perhaps. I guess it depends on what TimeoutCancellationException is meant to be treated as - is it meant inherit the semantics of CancellationException which it extends - i.e. "It indicates normal cancellation of a coroutine." or does it override that to be used for "unexpected" cancellation of a coroutine?
I assume the former else it probably wouldn't be extending, but it feels most use cases of withTimeout that I can imagine would be for unexpected (or undesired) cancellation. If it's not meant to be used this way then we can replace our uses of withTimeout for one which throws something else.

@getmkt
Copy link

getmkt commented Aug 10, 2022

I noticed flatMapLatest intenrally uses ChannelFlowOperator which uses coroutines to collect and trandform. If you try that appraoch in flatMapConcat then shows the same results as in flatMapLatest.

    class CancelationException2 : CancellationException()

    val flow = flowOf("A", "B")

    runBlocking {
        var threw1 = false
        try {
            flow
                .flatMapConcat {
                    flow<String> {
//                        throw CancellationException2() // prints Error1 CancellationException2
//                        val job = launch {
//                            throw CancellationException2() // does not prints Error1 CancellationException2
//                        }
//                        job.join()
                    }
                }
                .collect()
        } catch (t: Throwable) {
            threw1 = true
            println("Error1 $t")
        }

It does say in CancellationException:

Thrown by cancellable suspending functions if the coroutine is cancelled while it is suspended. It indicates normal cancellation of a coroutine.

@qwwdfsad qwwdfsad added the flow label Aug 22, 2022
@qwwdfsad
Copy link
Member

This is not an inconsistency, but rather a semantic difference: see #2964 with an explanation and #2942 with actual rationale.

Documentation could've been better here though.

I guess it depends on what TimeoutCancellationException is meant to be treated as - is it meant inherit the semantics of CancellationException which it extends - i.e. "It indicates normal cancellation of a coroutine." or does it override that to be used for "unexpected" cancellation of a coroutine?

It is the former, but it should be the latter: #1374. Right now it's just our design mistake that is really tough to fix

@qwwdfsad qwwdfsad added the docs KDoc and API reference label Aug 22, 2022
@rossdanderson
Copy link
Author

I appreciate the clarification, thanks very much

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs KDoc and API reference flow
Projects
None yet
Development

No branches or pull requests

3 participants