-
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
Extend the KDoc for some channel APIs #4148
Conversation
Before, the description of `SUSPEND` was phrased in terms of what will happen, while the rest of the entries were described in an imperative form, that is, as commands as to what should happen. Now, all entries are clarified using a descriptive form.
Added explanations of what exactly happens on each code path, how these operators ensure that all elements get processed eventually, and provided some usage examples.
Currently, the documentation states that uncaught exceptions will lead to the channel being closed. "Uncaught exceptions" is a special thing in kotlinx.coroutines: <https://kotlinlang.org/docs/exception-handling.html#coroutineexceptionhandler> These are not just exceptions that are not wrapped in a try-catch, these are exceptions that can not be propagated to a root coroutine via structured concurrency. Fixed the wording and added a test that shows that uncaught coroutine exceptions are not handled in any special manner.
Turns out, only a single invocation of either `awaitClose` or `invokeOnClose` is allowed in the lifetime of a channel. Document that.
Instead, use `Channel.RENDEZVOUS` so that a meaningful constant is shown in Dokka's output.
Currently, the docs claim that attempting to receive from a failed channel fails. However, the documentation for `Channel` itself correctly states that before `receive` fails, the elements that were already sent will be processed first. Corrected this and added a test demonstrating the behavior.
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.
well done!
* print("in the producer: ") | ||
* it.cleanup() | ||
* } | ||
* yield() |
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.
yield is redundant and rather confusing here, let's avoid that?
* } | ||
* // Grab the first value and discard everything else | ||
* launch(Dispatchers.Default) { | ||
* val firstElement = channel.consumeFirst() |
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.
Maybe invoke it in-place instead of a single coroutine?
* } | ||
* ``` | ||
* | ||
* In this example, all ten values created by the producer coroutine will be processed: one by `consumeFirst`, |
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.
Reading this, it seems like the example is mixing two concepts: one is about consuming, and the other about undelivered clean up (which we don't consider as "processed", it has a different meaning).
Maybe something more straightforward?
Like the following (outline, haven't tried to compile it):
val producer = produce {
var currentElement = 0
try {
while(true) {
send(currentElement++)
}
} finally {
println("Cancelled after: $currentElement")
}
}
consumeFirst { ... }
It's deterministic, more trivial and straightforward. WDYT?
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, good point. The point of consume
seems to be just notifying the producer that there are no more consumers. I noticed that consume
is a reliable way not to miss any elements and became too fixated on that, but this really is a just side effect.
* | ||
* This function will attempt to receive elements and put them into the list until the channel is | ||
* [closed][SendChannel.close]. | ||
* Calling [toList] without closing the channel is always incorrect: |
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.
Let's make it a bit less ambigous? "Calling toList on channels that are not closed" or something like that
* ``` | ||
* val values = listOf(1, 5, 2, 9, 3, 3, 1) | ||
* val channel = Channel<Int>() | ||
* GlobalScope.launch { |
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.
Let's use produce
for that?
Esp. here, where finally
block is required otherwise
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 found produce
too high-level and thought that using it could detract from the point of the sample. Is it wrong? Is produce
common knowledge and part of any coroutine user's toolkit?
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.
produce
is more high-level, but it's also the idiomatic way to create a producer coroutine, so it's worth mentioning. It's true that it may overwhelm the reader, but we can probably work around this by adding extensive comments.
* Example of usage: | ||
* ``` | ||
* val callbackEventsStream = produce { | ||
* val disposable = registerChannelInCallback(channel) | ||
* awaitClose { disposable.dispose() } | ||
* } | ||
* ``` | ||
* | ||
* Internally, [awaitClose] is implemented using [SendChannel.invokeOnClose]. |
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 catch!
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.
Thanks! All good except one redundant example
* | ||
* There is no way to recover these lost elements. | ||
* If this is unsuitable, please create a [Channel] manually and pass the `onUndeliveredElement` callback to the | ||
* constructor. |
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 example is irrelevant to produce
documentation, it's Channel
doc that should explain that
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 beg to differ. produce
calls the Channel
function, passing a null
to onUndeliveredElement
. This is produce
's implementation detail (that we must document) and not the Channel
's.
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.
Or do you mean the snippet below? It's not an example, it's an explanation of how to approximate the produce
above with an explicitly created channel. Would it work for you if I explain this in text explicitly?
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 mean the snippet itself, yes. I would say it's sufficient to refer to onUndeliveredElement
where it has more comprehensive explanation and self-sufficient example
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.
Non-obvious things this snippet highlights:
- Where the
onUndeliveredParameter
is (you can't refer to it directly, the user has to chase the docs) finally
with theclose
in the coroutine
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.
Where the onUndeliveredParameter is (you can't refer to it directly, the user has to chase the docs)
- "If this is unsuitable, please create a [Channel] manually and pass the
onUndeliveredElement
callback to the constructor: [Channel(onUndeliveredElement = ...)]]Channel]" probably will suffice.
I realize that using it might be less obvious, but still believe it's not the produce's documentation job to explain that
One of the pieces of #4133.
Each file can be reviewed independently (and changes to other files will be added later). I'm keeping this a single pull request to avoid spamming.