Skip to content
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

Consider stabilizing Channel.invokeOnClose #3358

Closed
JakeWharton opened this issue Jul 5, 2022 · 3 comments
Closed

Consider stabilizing Channel.invokeOnClose #3358

JakeWharton opened this issue Jul 5, 2022 · 3 comments

Comments

@JakeWharton
Copy link
Contributor

It was added in 0.24 and made experimental in 1.0 later that year. Both of those events were 4 years ago. Time to stabilize?

@qwwdfsad
Copy link
Contributor

qwwdfsad commented Jul 11, 2022

Indeed, it's time to, thanks for pointing it out.

If you have any other use-cases apart from the linked one (https://github.com/cashapp/zipline/blob/79cd02d6dfd4936c8de20fd3950a68a121fc8aee/zipline/src/commonMain/kotlin/app/cash/zipline/internal/bridge/flows.kt#L74-L83), please don't hesitate to share.

The most interesting questions here are whether more than one handler is ever required (though I believe we can change this particular restriction at any time) and whether exceptions from the handler should be rethrown right from caller's close/cancel even though they are clearly undeclared

@JakeWharton JakeWharton changed the title Consider stabilizing Channel.invokeOnClose Consider stabilizing Channel.invokeOnClose Sep 9, 2022
@whyoleg
Copy link
Contributor

whyoleg commented Nov 26, 2022

Have you considered using CompletionHandler in place of plain lambda - it's typealias, so there is no difference except for consistency with CancellableContinuation.invokeOnCancellation(handler: CompletionHandler) and Job.invokeOnCompletion(handler: CompletionHandler)?

@qwwdfsad
Copy link
Contributor

@whyoleg that's an interesting suggestion, thanks.
We'll consider this; one of the potential benefits is the ability to align documentation between various handlers

qwwdfsad added a commit that referenced this issue Mar 14, 2023
CompletionHandler is not user deliberately, as its contract requires some additional refinement along with `onCancelling` handler stabilization. Note that replacing functional type with the very same typealias is backwards-compatible in the current state of linkage, so we are not giving up any future opportunities.

Also, fix behavioral mismatch: `CancellationException` is supplied (and always has been) to `invokeOnClose` when a channel was cancelled normally instead of `null` as documentation stated.
This behaviour is aligned with other cancellation handlers and also allows handler to distinguish whether the channel was closed or cancelled.

Fixes #3358
woainikk pushed a commit that referenced this issue Apr 4, 2023
* Stabilize Channel.invokeOnClose

CompletionHandler is not used deliberately, as its contract requires some additional refinement along with `onCancelling` handler stabilization. Note that replacing functional type with the very same typealias is backwards-compatible in the current state of linkage, so we are not giving up any future opportunities.

Also, fix behavioural mismatch: `CancellationException` is supplied (and always has been) to `invokeOnClose` when a channel was cancelled normally instead of `null` as documentation stated.
This behaviour is aligned with other cancellation handlers and also allows the handler to distinguish whether the channel was closed or cancelled.

Fixes #3358
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants