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

suspendCancellableCoroutine

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

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?

Pretty much

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(...)
}

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

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?

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

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.

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.

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.

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

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.

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.

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.