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

Implement receiveOrClosed for channels and select clause #330

Closed
qwwdfsad opened this issue Apr 14, 2018 · 7 comments
Closed

Implement receiveOrClosed for channels and select clause #330

qwwdfsad opened this issue Apr 14, 2018 · 7 comments

Comments

@qwwdfsad
Copy link
Member

qwwdfsad commented Apr 14, 2018

We need receiveOrClosed method on top of channels and onReceiveOrClosed for select which will have return type Either<E, CloseToken>.

Currently, pattern to work with channels always has the following form which is clumsy and obfuscates code:

 try {
    source.onReceive { ... }
} catch (e: ClosedReceiveChannelException) {
  // source is closed
}

The better alternative may be

source.onReceiveOrClosed {
  when (it) {
    CloseToken -> // end up consumption
    _ -> // proceed normally
  }
}

For implementation to be efficient we need inline classes (Kotlin/KEEP#104)

@fvasco
Copy link
Contributor

fvasco commented Apr 14, 2018

I agree with you, but in my experience the onClose is missing, this can become channel.job.onJoin

@elizarov
Copy link
Contributor

Let me clarify that the considerable fraction of value is going to come from support for onReceiveOrClosed clause in select expression. With that you can write operations on multiple channels with less boilerplate, without having to deal with concurrency, and with ability to code any behavior on the close of any of the source channels.

Note, that when we have channel.job.onJoin you still will NOT be able to use onJoin in select together with onReceive, e.g this:

select {
    channel.onReceive { ... }
    channel.job.onClose { ... }
}

will not work as expected. We shall make sure it fails fast in this case with a clear error message.

Another note, is that we should not actually expose CloseToken in public API, since trying to send this token over another channel will lead to unpredictable results, so we shall encapsulate it by introducing a special ValueOrClosed<T> type (that will be implement as inline class in the future).

@fvasco
Copy link
Contributor

fvasco commented Apr 15, 2018

You are right @elizarov
however I still prefer something like:

select {
    channel.onClose { ... } // onClose first
    channel.onReceive { ... }
}

Kotlin/KEEP#104 is WIP, but I cannot understand how is it possible apply to onReceiveOrClosed, the only way to implement a ValueOrClosed<T> is a sealed class (and sealed class cannot be inlined).

@elizarov
Copy link
Contributor

twyatt added a commit to JuulLabs/able that referenced this issue Oct 26, 2018
Typical behavior of `Channel` is to throw a
`ClosedReceiveChannelException` on invocation of `close` if a `receive`
is suspended. This is unfortunately a bit counter intuitive and can
produce misleading stacktraces.

In order to be more explicit with our exception stacktraces related to
`Channel` cancelation, using `receiveOrNull` and explicitly throwing
`GattClosed` exception when receiving `null`.

May need to revisit this behavior in the future after `receiveOrClosed`
is implemented per Kotlin/kotlinx.coroutines#330. Also noteworthy is
that `receiveOrNull` was undeprecated via Kotlin/kotlinx.coroutines#739.
twyatt added a commit to JuulLabs/able that referenced this issue Oct 29, 2018
Typical behavior of `Channel` is to throw a
`ClosedReceiveChannelException` on invocation of `close` if a `receive`
is suspended. This is unfortunately a bit counter intuitive and can
produce misleading stacktraces (i.e. stacktrace will include `close`
invocation but not `receive`).

In order to be more explicit with our exception stacktraces related to
`Channel` cancelation: catching `ClosedReceiveChannelException` and
explicitly throwing `GattClosed` exception (that carries original
`ClosedReceiveChannelException` as it's cause).

May need to revisit this behavior in the future after `receiveOrClosed`
is implemented per Kotlin/kotlinx.coroutines#330.
elizarov pushed a commit that referenced this issue Jul 17, 2019
* The corresponding ReceiveChannel methods are deprecated.
* Introduced corresponding extensions with the same semantic and generic
  Any bound.
* Introduce internal ReceiveChannel.[on]receiveOrClosed
  * Using internal inline class ValueOrClosed.
  * To be stabilized and made public in the future when inline classes
    ABI stabilizes.
  * It is related to #330 but does not resolve it yet.
* Includes todos for future public ValueOrClose design.
elizarov pushed a commit that referenced this issue Jul 17, 2019
* The corresponding ReceiveChannel methods are deprecated.
* Introduced corresponding extensions with the same semantic and generic
  Any bound.
* Introduce internal ReceiveChannel.[on]receiveOrClosed
  * Using internal inline class ValueOrClosed.
  * To be stabilized and made public in the future when inline classes
    ABI stabilizes.
  * It is related to #330 but does not resolve it yet.
* Includes todos for future public ValueOrClose design.
elizarov pushed a commit that referenced this issue Jul 18, 2019
* The corresponding ReceiveChannel methods are deprecated.
* Introduced corresponding extensions with the same semantic and generic
  Any bound.
* Introduce internal ReceiveChannel.[on]receiveOrClosed
  * Using internal inline class ValueOrClosed.
  * To be stabilized and made public in the future when inline classes
    ABI stabilizes.
  * It is related to #330 but does not resolve it yet.
* Includes todos for future public ValueOrClose design.
* Simplify AbstractChannel select implementations.
* AbstractChannel implementation is optimized to avoid code
  duplication in suspension of different receive methods:
  receive, receiveOrNull, receiveOrClosed.
elizarov pushed a commit that referenced this issue Jul 18, 2019
* The corresponding ReceiveChannel methods are deprecated.
* Introduced corresponding extensions with the same semantic and generic
  Any bound.
* Introduce internal ReceiveChannel.[on]receiveOrClosed
  * Using internal inline class ValueOrClosed.
  * To be stabilized and made public in the future when inline classes
    ABI stabilizes.
  * It is related to #330 but does not resolve it yet.
* Includes todos for future public ValueOrClose design.
* Simplify AbstractChannel select implementations.
* AbstractChannel implementation is optimized to avoid code
  duplication in suspension of different receive methods:
  receive, receiveOrNull, receiveOrClosed.
elizarov pushed a commit that referenced this issue Jul 18, 2019
* The corresponding ReceiveChannel methods are deprecated.
* Introduced corresponding extensions with the same semantic and generic
  Any bound.
* Introduce internal ReceiveChannel.[on]receiveOrClosed
  * Using internal inline class ValueOrClosed.
  * To be stabilized and made public in the future when inline classes
    ABI stabilizes.
  * It is related to #330 but does not resolve it yet.
* Includes todos for future public ValueOrClose design.
* Simplify AbstractChannel select implementations.
* AbstractChannel implementation is optimized to avoid code
  duplication in suspension of different receive methods:
  receive, receiveOrNull, receiveOrClosed.
qwwdfsad added a commit that referenced this issue Jul 18, 2019
* The corresponding ReceiveChannel methods are deprecated.
* Introduced corresponding extensions with the same semantic and generic
  Any bound.
* Introduce internal ReceiveChannel.[on]receiveOrClosed
  * Using internal inline class ValueOrClosed.
  * To be stabilized and made public in the future when inline classes
    ABI stabilizes.
  * It is related to #330 but does not resolve it yet.
* Includes todos for future public ValueOrClose design.
* Simplify AbstractChannel select implementations.
* AbstractChannel implementation is optimized to avoid code
  duplication in suspension of different receive methods:
  receive, receiveOrNull, receiveOrClosed.
qwwdfsad added a commit that referenced this issue Mar 18, 2021
    * Introduce three-state ChannelResult, a domain-specific Result class counterpart
    * Introduce receiveCatching/onReceiveCatching, make it public
    * Get rid of receiveOrClosed/onReceiveOrClosed without migrations, it was @InternalCoroutinesApi anyway

Fixes #330
qwwdfsad added a commit that referenced this issue Mar 18, 2021
    * Introduce three-state ChannelResult, a domain-specific Result class counterpart
    * Introduce receiveCatching/onReceiveCatching, make it public
    * Get rid of receiveOrClosed/onReceiveOrClosed without migrations, it was @InternalCoroutinesApi anyway

Fixes #330
qwwdfsad added a commit that referenced this issue Mar 22, 2021
    * Introduce three-state ChannelResult, a domain-specific Result class counterpart
    * Introduce receiveCatching/onReceiveCatching, make it public
    * Get rid of receiveOrClosed/onReceiveOrClosed without migrations, it was @InternalCoroutinesApi anyway

Fixes #330
qwwdfsad added a commit that referenced this issue Mar 22, 2021
    * Introduce three-state ChannelResult, a domain-specific Result class counterpart
    * Introduce receiveCatching/onReceiveCatching, make it public
    * Get rid of receiveOrClosed/onReceiveOrClosed without migrations, it was @InternalCoroutinesApi anyway

Fixes #330
qwwdfsad added a commit that referenced this issue Mar 22, 2021
    * Introduce three-state ChannelResult, a domain-specific Result class counterpart
    * Introduce receiveCatching/onReceiveCatching, make it public
    * Get rid of receiveOrClosed/onReceiveOrClosed without migrations, it was @InternalCoroutinesApi anyway

Fixes #330
@qwwdfsad
Copy link
Member Author

Will be named receiveCatching and onReceiveCatching, see #974 (comment)

@matejdro
Copy link

matejdro commented Apr 16, 2021

onReceiveOrClosed implementation mentions that it will not be internal once https://youtrack.jetbrains.com/issue/KT-27524 is fixed.

That bug has been fixed. What is the status for this?

@qwwdfsad
Copy link
Member Author

qwwdfsad commented Apr 16, 2021

It will be added to coroutines 1.5.0 under receiveCatching and onReceiveCatching names.

It's already implemented and merged, the rationale of naming is explained in this comment

pablobaxter pushed a commit to pablobaxter/kotlinx.coroutines that referenced this issue Sep 14, 2022
    * Introduce three-state ChannelResult, a domain-specific Result class counterpart
    * Introduce receiveCatching/onReceiveCatching, make it public
    * Get rid of receiveOrClosed/onReceiveOrClosed without migrations, it was @InternalCoroutinesApi anyway

Fixes Kotlin#330
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

4 participants