I’m trying to understand when I need to use `suspe...

# coroutines

[Stylianos Gakis]

I’m trying to understand when I need to use suspendCancellableCoroutine over suspendCoroutine . If I got a use case where all I need to do is something like:

suspend fun MediaPlayer.seekToPercent(
    @FloatRange(from = 0.0, to = 1.0) percentage: Float,
) = suspendCoroutine<Unit> { cont ->
    val callback = MediaPlayer.OnSeekCompleteListener {
        this.setOnSeekCompleteListener(null)
        cont.resume(Unit)
    }
    setOnSeekCompleteListener(callback)
    val positionToSeekTo = (duration.toFloat() * percentage).toInt()
    seekTo(positionToSeekTo)
}

Is there just no reason to use suspendCancellableCoroutine?
Basically I wonder if there are any other implications regarding the cancellability of suspendCoroutine and if it behaves in any different way to suspendCancellableCoroutine, or all that they do differently is that in a suspendCancellableCoroutine we have access to more than just resume(value: T) and resumeWithException(exception: Throwable) ?
Reading the documentation doesn’t fill all the gaps in my head

[Dominaezzz]

suspendCancellableCoroutine supports cancellation, so if the current job is cancelled then it will throw immediately.

[Stylianos Gakis]

So if we’re inside a suspendCoroutine, and the job that has launched it gets cancelled, it just wouldn’t throw? It would prevent it from finishing cancellation until it eventually resumed?

[Dominaezzz]

Pretty much

[Stylianos Gakis]

In this case, if I were to use suspendCancellableCoroutine I’d have to manually make sure that if the callback that I am registering is for sure unregistered if there is a cancellation before the callback has a chance to trigger.
Would adding this look reasonable?
cont.invokeOnCancellation { setOnSeekCompleteListener(null) }

All together:

suspend fun MediaPlayer.seekToPercentCancellable(
    @FloatRange(from = 0.0, to = 1.0) percentage: Float,
) = suspendCancellableCoroutine<Unit> { cont ->
    cont.invokeOnCancellation { setOnSeekCompleteListener(null) } // Adding this
    val callback = MediaPlayer.OnSeekCompleteListener {
        this.setOnSeekCompleteListener(null)
        cont.resume(Unit)
    }
    setOnSeekCompleteListener(callback)
    seekTo(...)
}

[Joffrey]

I don't believe removing the callback is of any use. The point is to cancel whatever asynchronous thing you're doing. If the callback-based API you're calling is not cancellable itself, then you should just use suspendCoroutine

[Stylianos Gakis]

But with what was said above, if we’re in this scenario:
• We’re inside suspendCoroutine and we’re waiting for the callback in order to resume execution
• The job that has called this suspend function is cancelled, and with it the mediaPlayer is cleaned up and will never call its OnSeekCompleteListener
• We’re stuck waiting for the callback to trigger which never will, and the parent job is stuck in “cancelling” state never able to finish it’s job.
Isn’t this a scenario that we should take care of?

[Joffrey]

and with it the mediaPlayer is cleaned up

How does that happen? I'm unfamiliar with this API, but if you can cancel whatever the media player is doing, then this is what you should do in invokeOnCancellation. Otherwise, cancelling this suspend function gives the impression of cancellation (control flow returns to the caller) but the underlying work keeps going, which is basically a leak

[Joffrey]

But yeah suspendCoroutine will prevent cancellation until the media player calls the callback (which may be never), while suspendCancellableCoroutine will ensure this suspend function is cancelled, even if you don't clean up the actual underlying operation and leak it

[Joffrey]

You can play with this here: https://pl.kotl.in/S9g7RZNsS

You can see that with suspendCoroutine, the cancellation doesn't affect the job and it still calls the callback before completing. With suspendCancellableCoroutine the job is immediately cancelled (and completes), but the work done behind the scenes carries on and you can see the callback being called later

[Adam Powell]

Unless you're writing an API that makes use of @RestrictsSuspension you should pretty much always use suspendCancellableCoroutine and not suspendCoroutine. Just because an underlying API doesn't support cancelling the actual work being performed does not mean that the caller does not want to be able to stop waiting for it by cancelling the request.

[Stylianos Gakis]

but the work done behind the scenes carries on and you can see the callback being called later

Which is exactly what I don’t want, so actually adding this cont.invokeOnCancellation { setOnSeekCompleteListener(null) } is what I want indeed.

pretty much always use suspendCancellableCoroutine

I think this is what I am getting out of this, being eternally stuck inside a suspendCoroutine sounds like a very easy way to introduce bugs in your code. With emphasis on the “pretty much” of course, as every case might be different.

[Stylianos Gakis]

You can play with this here: https://pl.kotl.in/S9g7RZNsS

Adding if (cont.isActive) { doStuff() } seems to also work to avoid doing the work as well if the continuation was cancelled before it reached that point. I’m thinking it should have equivalent behavior to yield(); doStuff() if we were actually in a suspend context, but I inside the suspendCancellableCoroutine we’re not so this will do.

[Joffrey]

Just because an underlying API doesn't support cancelling the actual work being performed does not mean that the caller does not want to be able to stop waiting for it by cancelling the request.

Fair enough, but I don't believe this should be taken as a go-to solution and lead to blanket statements like:

you should pretty much always use suspendCancellableCoroutine and not suspendCoroutine

Cancellation is cooperative in coroutines, so it really depends on the API we're talking about here. In this specific case, hanging forever is definitely not a solution, so I guess it's fine to use suspendCancellableCoroutine and just let the mediaplayer continue even though we're not interested in the result of the seekTo (because we don't seem to have a choice).

In many cases, the operation will terminate, it's just not cancellable. So giving control back to the caller is giving a false impression of cancellation that is IMO not desirable. There is still work being done here, and it should be expected that you can't always reclaim control from a coroutine immediately.

> but the work done behind the scenes carries on and you can see the callback being called later

Which is exactly what I don’t want, so actually adding this cont.invokeOnCancellation { setOnSeekCompleteListener(null) } is what I want indeed.

My point is that, if what you don't want is "the work being done", removing the callback is useless, it just prevents the continuation from being resumed a second time and crashing, but it still hasn't cancelled the extra work. So if you have other means to actually cancel the work, that should be done instead. If you don't have a choice, then yeah at least remove the callback.

Adding if (cont.isActive) { doStuff() } seems to also work to avoid doing the work as well if the continuation was cancelled before it reached that point

That might partly help, but I think in the majority of cases the work in doStuff() has already started, and in these cases it doesn't help.

[Adam Powell]

blanket statements [...] giving a false impression of cancellation that is IMO not desirable

Hence, "pretty much". 🙂 There are exceptions, but they're rare enough to be the exceptions that prove the rule. If you have one you probably know it. Thanks to structured concurrency there could be a lot waiting for child jobs to join before other work or cleanup can proceed, and wedging a caller until strict completion when the alternative of ignoring a late-arriving result would not affect observable correctness or ability for that caller to clean up its own resources early has consistently been the worse option in my experience

[Joffrey]

The whole point of structured concurrency is to avoid leaked work by ensuring every coroutine is accounted for. If coroutines stop tracking actual asynchronous work without cancelling it, it feels like defeating the purpose of structured concurrency (at least to me). I'm not totally convinced either way about the best default, because as you pointed out there probably are lots of cases for which we are ok leaking background work. But I would argue that it should be decided on a case-by-case basis, rather than defaulting to "surface" cancellation. Maybe there is a reason the underlying API is not cancellable in the first place.

[Adam Powell]

After working with a lot of teams exposing async apis, it's usually because supporting cancellation without the aid of a framework like kotlinx.coroutines or rxjava is somewhere between cumbersome and hard, and they can justify omitting it since a lot of their users won't object. Often the justification is, "if the caller doesn't care anymore they can just ignore the result." The burden of proof is on why cancellation can't be supported and why the caller additionally isn't permitted to assume something on the other end of an async call didn't unrecoverably crash or experience some kind of bug, stop waiting for a callback that may never come, and attempt to clean up/recover gracefully itself. Processes and machines on a network come up and down all the time, storage abstractions can come, go, get wedged for all sorts of reasons. If you can't trust a withTimeout(...) { ... } then it becomes pretty hard to write code that uses these things together. The ecosystem would be worse off if non-cancellable behavior of these things weren't reserved for highly exceptional circumstances.

[Joffrey]

it's usually because supporting cancellation without the aid of a framework like kotlinx.coroutines or rxjava is somewhere between cumbersome and hard

That makes sense.

The burden of proof is on why cancellation can't be supported

Exactly. And depending on the why, the decision may be different.

Processes and machines on a network come up and down all the time, storage abstractions can come, go, get wedged for all sorts of reasons.

There usually are failure callbacks, though, even in non-cancellable APIs. So those situations should already be covered by that.

I think we agree on pretty much everything ("pretty much" 🙂) - the only difference is the extent to which you consider uses of suspendCoroutine exceptional. Maybe I'm wrong in my personal stats then.