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
WIP: provide resume
and tryResume
that pass the value to the callback
#4090
base: develop
Are you sure you want to change the base?
WIP: provide resume
and tryResume
that pass the value to the callback
#4090
Conversation
This is clearly far from optimal, and also, there are no tests, nor documentation. Requesting a review anyway to discuss if the approach is viable. |
…back Fixes some concerns in #4088
34a5e1e
to
e29c85b
Compare
@@ -198,6 +198,8 @@ public interface CancellableContinuation<in T> : Continuation<T> { | |||
*/ | |||
@ExperimentalCoroutinesApi // since 1.2.0 | |||
public fun resume(value: T, onCancellation: ((cause: Throwable) -> Unit)?) | |||
|
|||
public fun <R: T> resume(value: R, onCancellation: ((cause: Throwable, value: R) -> Unit)?) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice trick with the signature!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not a trick: we're notifying the type system that the type of onCancellation
's value is decided not by the view of the continuation, but primarily by the type of value
passed to the function. This type bound promises that onCancellation
will receive exactly the value
that this same function receives, and not just any value that CancellableContinuation
could be resumed with in theory.
Without this information, onCancellation
would have to be ready to accept an arbitrary supertype of T
(effectively, any value): because of the in
variance of T
in CancellableContinuation
, resume
could be called on a more specialized view of CancellableContinuation
, that is, if we have CancellableContinuation<Any>
, the call could be CancellableContinuation<Int>.resume
, and also, in parallel, the continuation could be actually resumed with a Double
. We know this can't happen, but with the bound, the type system knows that, too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the explanation, that's what I've meant :) "Trick" is a substitute for a "a nice and smart solution to our specific issue"
@@ -198,6 +198,8 @@ public interface CancellableContinuation<in T> : Continuation<T> { | |||
*/ | |||
@ExperimentalCoroutinesApi // since 1.2.0 | |||
public fun resume(value: T, onCancellation: ((cause: Throwable) -> Unit)?) | |||
|
|||
public fun <R: T> resume(value: R, onCancellation: ((cause: Throwable, value: R) -> Unit)?) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd say we need to decide two things:
- Whether we want to keep the previous overload (probably not, worth pointing out)
- Whether we want to provide a third parameter to be
CoroutineContext
. It's not the selects performance I worry about, but the general applicability: consider I have aresume(resource, closeResourceFunIfCancelled)
. Theclose
typically can fail and the exception can either be ignored, re-thrown (then we'll handle it as part of our last-ditch mechanism) or report it to the application-specific log. Whether it needs aCoroutineContext
for that is an open question, I'm not really sure (and this decision does not block anything really)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One more: whether we want to break source compatibility in most cases or almost none of them. The latter can be achieved with onCancellation: ((R).(cause: Throwable) -> Unit)?
and nicely supports the pattern cont.resume(response) { close() }
, but it can also silently change the semantics of existing code.
@@ -198,6 +198,8 @@ public interface CancellableContinuation<in T> : Continuation<T> { | |||
*/ | |||
@ExperimentalCoroutinesApi // since 1.2.0 | |||
public fun resume(value: T, onCancellation: ((cause: Throwable) -> Unit)?) | |||
|
|||
public fun <R: T> resume(value: R, onCancellation: ((cause: Throwable, value: R) -> Unit)?) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A bit of historical context: most likely, we haven't provided any value
because the pattern we expected this to be used is kind of the following:
suspend fun Integration.await() = suspendCancellableCoroutine { cont ->
val callback = this.createCallback()
callback.then(cont) // invokes resume(). Can do resume(value, this) to save an allocation
cont.invokeOnCancellation(callback)
}
Though, under closer inspection, you might notice that the same signature is used twice in different contexts and it doesn't really work
Fixes some concerns in #4088