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. |
dkhalanskyjb
force-pushed
the
dk-pass-value-to-cancellableContinuation-resumption
branch
from
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.
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). Theclosetypically 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 aCoroutineContextfor 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.
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.
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
dkhalanskyjb
force-pushed
the
dk-pass-value-to-cancellableContinuation-resumption
branch
from
6b91cd0
to
c6eb76b
Compare
| availablePermits == 0 | ||
| override val isLocked: Boolean | ||
| get() = | ||
| availablePermits == 0 |
There was a problem hiding this comment.
Autoformatting did something strange.
dkhalanskyjb
changed the title
resume and tryResume that pass the value to the callbackresume and tryResume that pass the value to the callback
| * | ||
| * 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.
This is effectively an internal class with no documentation. Probably backticked version will do
There was a problem hiding this comment.
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>
dkhalanskyjb
deleted the
dk-pass-value-to-cancellableContinuation-resumption
branch
Fixes some concerns in #4088