-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Provide resume
and tryResume
that pass the value to the callback
#4090
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. |
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.
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
6b91cd0
to
c6eb76b
Compare
availablePermits == 0 | ||
override val isLocked: Boolean | ||
get() = | ||
availablePermits == 0 |
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.
Autoformatting did something strange.
resume
and tryResume
that pass the value to the callbackresume
and tryResume
that pass the value to the callback
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 one!
* | ||
* The installed [onCancellation] handler should not throw any exceptions. | ||
* If it does, they will get caught, wrapped into a [CompletionHandlerException] and | ||
* If it does, they will get caught, wrapped into a [CompletionHandlerException], and |
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.
This is effectively an internal class with no documentation. Probably backticked version will do
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.
Yes, it's creating a link to 404: https://kotlinlang.org/api/kotlinx.coroutines/kotlinx-coroutines-core/kotlinx.coroutines/-cancellable-continuation/invoke-on-cancellation.html The reason is Kotlin/dokka#3601, but there is no reason not to work around it here.
Co-authored-by: Vsevolod Tolstopyatov <qwwdfsad@gmail.com>
Co-authored-by: Vsevolod Tolstopyatov <qwwdfsad@gmail.com>
Fixes some concerns in #4088