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

Update the documentation for the Channel interface #4241

Merged
merged 13 commits into from
Oct 25, 2024

Conversation

dkhalanskyjb
Copy link
Collaborator

@dkhalanskyjb dkhalanskyjb commented Oct 1, 2024

Note: a major piece of terminology that's changed in this is that when a channel is closed with a cause, it's no longer called a "failed channel". I found it immensely confusing to have channelResult.isClosed that means "the channel is closed", but channelResult.isFailed that doesn't mean "the channel is failed". Also, every cancelled channel is a "failed" channel by the definition of being closed with a cause, which doesn't seem to be the intention. So, I suggest that we remove mentions of "failed channels" throughout and replace them with "closed with a cause". Luckily, it's only relevant in a limited number of places.

@dkhalanskyjb dkhalanskyjb requested a review from qwwdfsad October 1, 2024 11:30
Copy link
Contributor

@lukellmann lukellmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While reading through the updated documentation, I noticed that it doesn't clarify how onUndeliveredElement and BufferOverflow.DROP_OLDEST/BufferOverflow.DROP_LATEST interact (i.e. do dropped elements get passed to the onUndeliveredElement function). It is only briefly mentioned in the example of the Channel factory function ("The dropped resources were closed by the channel itself").

I also think the behavior of SendChannel and ReceiveChannel functions when the channel was cancelled could be described in more detail. Instead of only mentioning in the documentation of ReceiveChannel.cancel that other functions will throw CancellationExceptions, this could be clarified in the documentation of each function this applies to.

Comment on lines 360 to 361
* Returns `true` if either the sending side of this channel was [closed][SendChannel.close]
* and all previously sent items were already received (which also happens for [cancelled][cancel] channels).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This sentence seems to miss an "or" (or is off in some other way).

kotlinx-coroutines-core/common/src/channels/Channel.kt Outdated Show resolved Hide resolved
@@ -200,107 +393,302 @@ public interface ReceiveChannel<out E> {
public val isClosedForReceive: Boolean

/**
* Returns `true` if the channel is empty (contains no elements), which means that an attempt to [receive] will suspend.
* This function returns `false` if the channel [is closed for `receive`][isClosedForReceive].
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Explicitly mentioning the false for channels that are closed for receive was clearer to me.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For me, it's the opposite, because the second sentence used to contradict the first one.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, it's very subjective :)

kotlinx-coroutines-core/common/src/channels/Channel.kt Outdated Show resolved Hide resolved
* If a channel was [closed][close] before [send] was called and no cause was specified,
* an [ClosedSendChannelException] will be thrown from [send].
* If a channel was [closed][close] with a cause before [send] was called,
* then [send] will rethrow the exact object that was passed to [close].
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* then [send] will rethrow the exact object that was passed to [close].
* then [send] will rethrow the exact exception that was passed to [close].

ReceiveChannel.receive uses "exception" instead of "object" and i think that's clearer.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was trying to evoke the feeling that the same runtime object (not just an exception with the same class etc.) was going to be thrown. Reworded.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Honestly, "exception" is much clearer to me

Comment on lines +801 to +803
* If the operation [failed][isFailure] because the channel was closed for that operation, [isClosed] returns `true`.
* The opposite is also true: if [isClosed] returns `true`, then the channel is closed for that operation
* ([ReceiveChannel.isClosedForReceive] or [SendChannel.isClosedForSend]).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be simpler to say something like "... if and only if ..." (stating the logical equivalence and not the implication in both directions).

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Math-style succinctness is not a good friend when designing APIs or writing documentation for general use. As an example, look at how vacuous truth caused a confusion: https://www.reddit.com/r/Kotlin/comments/q2llvo/what_should_a_function_called/

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense, I didn't think about that.

*/
public fun getOrThrow(): T {
@Suppress("UNCHECKED_CAST")
if (holder !is Failed) return holder as T
if (holder is Closed && holder.cause != null) throw holder.cause
error("Trying to call 'getOrThrow' on a failed channel result: $holder")
error("Trying to call 'getOrThrow' on a channel closed without a cause")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This exception might also be thrown when the result represents a failed operation on a channel that isn't closed yet. So the new exception message is a bit misleading.

kotlinx-coroutines-core/common/src/channels/Channel.kt Outdated Show resolved Hide resolved
kotlinx-coroutines-core/common/src/channels/Channel.kt Outdated Show resolved Hide resolved
kotlinx-coroutines-core/common/src/channels/Channel.kt Outdated Show resolved Hide resolved
@lukellmann
Copy link
Contributor

Note: a major piece of terminology that's changed in this is that when a channel is closed with a cause, it's no longer called a "failed channel".

The term is still used here:

@dkhalanskyjb
Copy link
Collaborator Author

I also think the behavior of SendChannel and ReceiveChannel functions when the channel was cancelled could be described in more detail. Instead of only mentioning in the documentation of ReceiveChannel.cancel that other functions will throw CancellationExceptions, this could be clarified in the documentation of each function this applies to.

Semantically, cancel does not put the channel into a "cancelled" state, it only closes the channel with a CancellationException and consumes the elements, and it looks like every operation that throws when the channel is closed already mentions this.

kotlinx-coroutines-core/common/src/channels/Channel.kt Outdated Show resolved Hide resolved
kotlinx-coroutines-core/common/src/channels/Channel.kt Outdated Show resolved Hide resolved
@@ -200,107 +393,302 @@ public interface ReceiveChannel<out E> {
public val isClosedForReceive: Boolean

/**
* Returns `true` if the channel is empty (contains no elements), which means that an attempt to [receive] will suspend.
* This function returns `false` if the channel [is closed for `receive`][isClosedForReceive].
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, it's very subjective :)

Comment on lines +801 to +803
* If the operation [failed][isFailure] because the channel was closed for that operation, [isClosed] returns `true`.
* The opposite is also true: if [isClosed] returns `true`, then the channel is closed for that operation
* ([ReceiveChannel.isClosedForReceive] or [SendChannel.isClosedForSend]).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense, I didn't think about that.

@lukellmann
Copy link
Contributor

Semantically, cancel does not put the channel into a "cancelled" state, it only closes the channel with a CancellationException and consumes the elements, and it looks like every operation that throws when the channel is closed already mentions this.

Oh, I didn't think about it that way (probably because CancellationExceptions are special). Yes, seems alright.

@kjnsn
Copy link

kjnsn commented Oct 3, 2024

Semantically, cancel does not put the channel into a "cancelled" state, it only closes the channel with a CancellationException and consumes the elements, and it looks like every operation that throws when the channel is closed already mentions this.

I think this could be clarified. When trying to read from a closed channel it doesn't throw a CancellationException (at least from my experience).

@dkhalanskyjb
Copy link
Collaborator Author

When trying to read from a closed channel it doesn't throw a CancellationException

@kjnsn, close the channel with a CancellationException (like cancel does), and it will:

import kotlinx.coroutines.*
import kotlinx.coroutines.channels.*

fun main() {
    runBlocking {
        val c = Channel<Int>()
        c.close(CancellationException())
        println(runCatching { c.receive() }) // CancellationException
    }
}

Runnable version; https://pl.kotl.in/vutYqCO0B

Copy link
Contributor

@qwwdfsad qwwdfsad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Superb revamp 👍

I have a few minor comments and it will be good to go

@dkhalanskyjb dkhalanskyjb requested a review from qwwdfsad October 25, 2024 10:15
Copy link
Contributor

@qwwdfsad qwwdfsad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome!

@dkhalanskyjb dkhalanskyjb merged commit 13147c4 into develop Oct 25, 2024
1 check passed
@dkhalanskyjb dkhalanskyjb deleted the dk-channels-kdoc branch October 25, 2024 11:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants