Feature Request: invokeOnCompletion handler for suspendCancellableCoroutine

[jschools]

It is inconvenient to use suspendCancellableCoroutine to wrap an API that requires a callback to be unregistered after receiving the call. Examples include "event bus" or "broadcast" APIs which do not unregister a callback automatically and will leak the callback.

We have invokeOnCancellation to handle situations where the coroutine is cancelled, although there is no handler for when the continuation is resumed successfully. This potentially could be done in the callback itself prior to calling cont.resume(), but it is not possible to get a reference to the callback if it is a lambda. It may also duplicate the code in the invokeOnCancellation block.

For example:

suspendCancellableCoroutine<Int> { cont ->
  val callback: (Int) -> Unit = {
    someApi.unregisterCallback(???) // no way to get reference to a lambda - cannot use `this` or `callback`
    cont.resume(it)
  }
  
  someApi.registerCallback(callback)
  
  cont.invokeOnCancellation {
    // not called after successful resume!
    someApi.unregisterCallback(callback)
  }
}

I propose adding invokeOnCompletion to CancelableContinuation which could be used to handle both cancellation and successful resume, similar to Job.invokeOnCompletion.

If there is a simpler way around this that I am missing, please do let me know.

[joffrey-bion]

Note that you could define your callback as an object instead of a lambda to get access to this:

    val callback = object : (Int) -> Unit {
        override fun invoke(value: Int) {
            someApi.unregisterCallback(this)
            cont.resume(value)
        }
    }

But the argument about repeating code still holds though.

[joffrey-bion]

Examples include "event bus" or "broadcast" APIs which do not unregister a callback automatically and will leak the callback.

@jschools Thinking about this, why would you wrap those APIs into suspend functions, though? Looks like a callbackFlow use case to me instead.

If you just want the first event from those APIs, then simply call first() on the returned flow.

[jschools]

@joffrey-bion I was thinking it would be convenient for suspendCancelableCoroutine to support a kind of "try/finally" option, in case an API needed to be destroyed or torn down after a successful resume. There are plenty of clumsy SDKs out there...

I agree callbackFlow solves all of these cases. Wouldn't that be an argument that it could replace suspendCancelableCoroutine altogether? I'm happy to close this issue if you think it's not a large enough concern 😀

[joffrey-bion]

I agree callbackFlow solves all of these cases. Wouldn't that be an argument that it could replace suspendCancelableCoroutine altogether?

They solve different problems, though. callbackFlow is meant to wrap APIs that call a callback repeatedly and expose results as a Flow, while suspendCancellableCoroutine is for single-shot callbacks and exposes this call as a suspend function.

There are plenty of clumsy SDKs out there...

Yeah, if some SDKs take a single-shot callback that needs to be explicitly unregistered, it's pretty crappy indeed. But when this happens there are options, like the above.

I'm happy to close this issue if you think it's not a large enough concern

I'm not opposed to this addition, though, don't get me wrong. Also I am by no means of any authority in this repository 😄 I'll let the maintainers decide for themselves

[LouisCAD]

@joffrey-bion I was thinking it would be convenient for suspendCancelableCoroutine to support a kind of "try/finally" option, in case an API needed to be destroyed or torn down after a successful resume. There are plenty of clumsy SDKs out there...

Actually, you can use try/finally with suspendCancellableCoroutine, you can surround the call to suspendCancellableCoroutine itself with the try block. Just make sure you don't risk crashing because resume has been called twice while the coroutine was completing, but didn't reach the finally block yet to unregister the callback because of the dispatch being queued.

[joffrey-bion]

@LouisCAD I think it would not be very convenient to do with a try/finally around the suspendCancellableCoroutine, because the finally likely needs a reference to the callback to unregister it, and the callback needs a reference to the continuation provided by suspendCancellableCoroutine. So it becomes a chicken-and-egg problem here 😄 and this would require some kind of lazy mechanism or lateinit var, which I guess is even uglier than just repeating the cleanup in both places

[dovchinnikov]

What's the status of this task?

Is there any reason why this was not implemented from the day 0?

[joffrey-bion]

@dovchinnikov could you please describe why you need this?

Are you using a single-shot callback API that requires unregistering the callback explicitly? If that's the case, I believe it's a problem in the underlying API.

If the API is a multi-shot callback, then it should instead be wrapped in callbackFlow IMO, which solves this problem. You can then use first() if you're interested in one element from the flow in your specific case. And even wrap that flow+first() in a new suspend function. This feels more correct semantically as it make the contract of the original API more clear. The fact that you're interested in only one event from that flow is a separate concern from adapting the callback API to coroutines.

[jschools]

My original intent for this feature request was to bring the API for suspendCancellableCoroutine in line with callbackFlow in order to add convenient support for "teardown" code. I found it surprising that suspendCancellableCoroutine does have invokeOnCancellation to support the cancellation case, but there is nothing similar to support the success case.

In my understanding, suspendCancellableCoroutine and callbackFlow both serve the same purpose: they are "bridges" between Coroutines and callbacks. suspendCancellableCoroutine is for single-shot APIs and callbackFlow is for multi-shot APIs.

Since they are so similar in purpose, they should be similar in syntax. callbackFlow has awaitClose { } for teardown code, so it would be nice to have a similar function for suspendCancellableCoroutine.

Imagine some API that requires you to call unregisterCallback with the same instance you passed to registerCallback so that it can do some internal reference counting and close a socket or stop some internal thread:

suspendCancellableCoroutine<Int> { cont ->
 // verbose anonymous class syntax required to get reference to `this` inside the callback
 val callback = object : (Int) -> Unit = {
   override fun invoke(result: Int) {
     // tear down
     someApi.unregisterCallback(this)

     cont.resume(it)
   }
 }
 
 someApi.registerCallback(callback)
 
 cont.invokeOnCancellation {
   someApi.unregisterCallback(callback) // redundant teardown code
 }
}

A more concise syntax would be analogous to callbackFlow's awaitClose { }:

suspendCancellableCoroutine<Int> { cont ->
 val callback = {
     cont.resume(it)
 }
 
 someApi.registerCallback(callback)
 
 // proposed new API: invokeOnCompletion
 cont.invokeOnCompletion {
   // called when continuation is either Resumed or Cancelled
   someApi.unregisterCallback(callback)
 }
}

[joffrey-bion]

Imagine some API that requires you to call unregisterCallback with the same instance you passed to registerCallback so that it can do some internal reference counting and close a socket or stop some internal thread

That's exactly why I'm asking for a use case. By definition a single shot API could unregister the callback itself after calling it, so I have trouble finding any real life situation where this requirement would exist.

[jschools]

The API being single-shot is not relevant in my thinking. The use case would be a library that requires a reference to its callback in order to destroy itself, which is not impossible to imagine. Why does suspendCancellableCoroutine provide a convenient way to clean up when it is cancelled, but not when it is not cancelled? (Also, I appreciate the constructive debate 😀)

[joffrey-bion]

The API being single-shot is the reason why it makes no sense to require the user to unregister the callback. The library can do it itself because it controls when it calls it, and thus knows when the callback won't be needed anymore. For a multi-shot callback, the library cannot know when the callback stops being useful unless the user tells it.

[dkhalanskyjb]

@joffrey-bion, thank you for driving the discussion in a use-case-driven direction.

Whether an API is single-shot is indeed crucial. An important point that was only raised in this thread in passing is that each continuation may only be resumed once. Resuming it several times will throw an exception.

With that in mind, let's list some scenarios:

1. A single thread can invoke the callback, and will no longer do that after invoking it once. suspendCancellableCoroutine is a good fit for this scenario and provides all the utilities already.
2. A single thread can invoke the callback, and may do so repeatedly until stopped, but we are only interested in one element. suspendCancellableCoroutine lacks the ability to say: "Hey, one element only, then stop." The proposed API would work well in this scenario.
3. Many threads may invoke the callback until they are stopped, but we are only interested in the first element. suspendCancellableCoroutine can not be easily made to work in this scenario, because callbacks could be invoked in parallel to the invokeOnCompletion code block, causing an exception.
4. A single thread can invoke the callback once, but has a memory leak: the reference to the callback needs to be cleaned up manually, or it will stay in the system, despite it no longer being needed. To work around this bug on the side of the caller, invokeOnCompletion could be used to unconditionally clear this reference.

If I missed some other cases where invokeOnCompletion could potentially be useful, please share them.

If scenario 4) is somehow widespread, it makes sense to introduce some convenient API to deal with this. It's strange if there are so many buggy APIs that the code repetition makes a huge difference, but if that's true, then sure, we could try making the use case more convenient.

Scenarios 2) and 3) are dangerously alike. I can easily imagine a situation where a library starts off with just one thread, but later, it becomes possible for several threads to invoke the callback in parallel, making the code using suspendCancellableCoroutine with invokeOnCompletion retroactively buggy. For multishot callbacks, it's far more robust to use callbackFlow and call first() on that to receive the first value only. Though if there are libraries that explicitly provide scenario 2) and guarantee that all callbacks will be invoked one after another synchronously, and it's often useful to only obtain the first value emitted by those libraries, then it also may make sense to discuss a shorthand form for that, but are there such use cases in reality? Even then, because of how similar 2) and 3) are, I think it's safer to guide the users to callbackFlow in both scenarios.

There are plenty of clumsy SDKs out there...

Imagine some API that requires you to call unregisterCallback with the same instance you passed to registerCallback so that it can do some internal reference counting and close a socket or stop some internal thread

The use case would be a library that requires a reference to its callback in order to destroy itself, which is not impossible to imagine

Could you provide some specific examples of existing APIs suffering from this problem?

Wouldn't that be an argument that [callbackFlow] could replace suspendCancelableCoroutine altogether?

callbackFlow is a high-level entity, whereas suspendCancellableCoroutine is a fundamental building block. Each has its purpose. You can cut cheese with a chainsaw, and you can fell a tree with a knife, but neither is the norm.

Actually, you can use try/finally with suspendCancellableCoroutine, you can surround the call to suspendCancellableCoroutine itself with the try block.

This will work in scenario 4, but not in scenario 2, because the finally block may not be able to fire in time for the callback-invoking thread to stop new emissions.

Since they are so similar in purpose, they should be similar in syntax.

This is not enough for us to consider introducing a new API. Both List<Int> and Set<Int> store elements, but there is no fun Set<Int>.reverse(): Set<Int> that would reverse the iteration order of a Set. The operation makes sense, but it's not useful enough.