Came across some code in a PR that seems weird to ...

# coroutines

[Colton Idle]

Came across some code in a PR that seems weird to me. Let me know if it's just me or if this is idiomatic coroutines code.

class MyCoolHandler{ 
var mContinuation: Continuation<Foo>? = null
suspend fun waitingForFoo(): Foo = suspendCoroutine { continuation -> 
// code
mContinuation = continuation
}
// some other class functions
}

[Zach Klippenstein (he/him) [MOD]]

This is how you manually write a suspend function that waits for some kind of callback or something. Typically you wouldn’t use this level of api unless you’re gluing two libraries or APIs together, which hopefully this is doing.

[Zach Klippenstein (he/him) [MOD]]

I would question why it’s not using suspendCancellableContinuation though.

[Colton Idle]

Gotcha. So if you're converting a callback to a suspend function you'd do it like this. I could have sworn i once used a callbackCoroutine or something for this... but I just looked and it seems like nothing like that exists. hm.

Thanks for teaching

[streetsofboston]

@Zach Klippenstein (he/him) [MOD]
But the callback (registration and initiation of it) would usually happen within/inside the suspendCoroutine.

Here it (the continuation) is cached to be used at a later time.
Is that a risk....?

[streetsofboston]

Hmmmm .... Thinking more about it... Likely not an issue.
Even if the registration and initiation of the callback is done inside the suspendCoroutine, the continuation is still cached, captured as part of the callback-lambda's closure.

[Zach Klippenstein (he/him) [MOD]]

Yea, that’s kind of the whole point. If you didn’t store the continuation somewhere then you wouldn’t need to suspend in the first place.

[gildor]

suspendCoroutine/suspendCancellableContinuation by itself are totally fine

But the rest of the code with caching continuation is very nasty:
1. If it is used as intended, it should be possible to write a coroutine adapter without any outside state caching. I wrote many of them and never had a need to leak a continuation outside of suspendCoroutine lambda
2. Current implementation doesn't support multiple callers at the same time and potentially even may conflict with each other (depending on the rest of the impl)
It looks that the goal of the code is to wait for something, not an actual coroutine adapter, so it could be implemetend just as a simple await() on a cached Job instance (if it one one-time action) or SharedFlow to suspend until Foo is received, without involvement of suspendCoroutine which is low level machinery

[phldavies]

Would it not make more sense to hold a CompletableDeferred<Foo> and have waitingForFoo await it?

[gildor]

Yep, this what I meant, Job/Deferred

[streetsofboston]

@gildor True, I've never had the need to do this either. Pattern looks weird 😁

But the risk of leaking and calling the continuation more than once is the same in both situations, it explicitly being cached or not.

When you provide a callback/lambda that captures the continuation to a third party service, there's no guarantee that this lambda is called at all by the service, just one time, multiple times or that this lambda (and continuation) is not leaked by this 3rd party service.

[gildor]

It depends on service, and there are ways to implement it correctly if service which is wrapped doesn't so something nasty
Still, it would be way safer, I do not agree that it's the same. Here risk is on level of user, not even on level of service