-
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 CoroutineStart
#4147
Conversation
By the way, here is our samples guide, might be useful: https://github.com/Kotlin/api-guidelines/blob/main/team_docs/samples.md |
* // Example of lazily starting a new coroutine that doesn't go through a dispatch initially | ||
* runBlocking { | ||
* println("1. About to lazily start a new coroutine.") | ||
* // Create a job to execute on `Dispatchers.Unconfined` later. |
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.
Same note about unconfined here, let's remove this sample
* This is similar to [DEFAULT], but the coroutine is guaranteed to start executing even if it was cancelled. | ||
* This only affects the initial portion of the code: on subsequent suspensions, cancellation will work as usual. | ||
* | ||
* [UNDISPATCHED] also ensures that coroutines will be started in any case. |
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'd say this is redundant as it looks like a typo: I read the doc to ATOMIC
and it mentions UNDISPATCHED
in the form that might imply this is a doc to UNDISPATCHED
.
Let's remove this section altogether, we have undispatched explained nearby anyway
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 reworded it a bit so that it doesn't look like a typo. Is that better?
As for UNDISPATCHED
being explained nearby, the way I see it, UNDISPATCHED
is in some cases a performance optimization over ATOMIC
, and I'm not sure how someone looking into ATOMIC
would learn about the occasionally more suitable UNDISPATCHED
without this explicit mention.
If you still think this section is unnecessary, I will remove it, of course.
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.
Please see my explanation about orthogonal concepts -- I think it explains why this section is unnecessary
* This is similar to [DEFAULT], but the coroutine is guaranteed to start executing even if it was cancelled. | ||
* This only affects the initial portion of the code: on subsequent suspensions, cancellation will work as usual. | ||
* | ||
* [UNDISPATCHED] also ensures that coroutines will be started in any case. |
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 reworded it a bit so that it doesn't look like a typo. Is that better?
As for UNDISPATCHED
being explained nearby, the way I see it, UNDISPATCHED
is in some cases a performance optimization over ATOMIC
, and I'm not sure how someone looking into ATOMIC
would learn about the occasionally more suitable UNDISPATCHED
without this explicit mention.
If you still think this section is unnecessary, I will remove it, of course.
* println("1. About to start a coroutine not needing a dispatch.") | ||
* // Dispatch the job to execute. | ||
* // `Dispatchers.Unconfined` is explicitly chosen. | ||
* val job = launch(Dispatchers.Unconfined) { |
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.
After you suggested this, I did have to spend quite some time pondering the issue!
Here's the table that summarizes the behavior for all combinations of CoroutineStart
and CoroutineDispatcher.isDispatchNeeded
:
Start option | isDispatchNeeded |
Behavior |
---|---|---|
DEFAULT |
yes | dispatch |
ATOMIC |
yes | dispatch, prohibiting cancellation |
LAZY |
yes | dispatch when requested |
UNDISPATCHED |
- | start in-place |
DEFAULT |
no | start in-place |
ATOMIC |
no | start in-place, prohibiting cancellation |
LAZY |
no | the requester starts the coroutine in-place |
I struggle to find an internally consistent model of this behavior that doesn't involve enumerating all of these possibilities in some form.
- We can define the
if (isDispatchNeeded) dispatch() else dispatchToEventLoop()
procedure as some kind of a "standard" dispatch procedure. Then, the table becomes:
Start option | Behavior |
---|---|
DEFAULT |
standard dispatch |
ATOMIC |
standard dispatch, prohibiting cancellation |
LAZY |
standard dispatch when requested |
UNDISPATCHED |
start in-place |
UNDISPATCHED
does not fit it, as it breaks the standard dispatch procedure.
- We can not declare that
isDispatchNeeded = false
simply overrides the dispatching behavior to unconditionally use the event loop, as only half of the cases is covered in this manner:
LAZY
withisDispatchNeeded = false
will still wait until someone requests the result.UNDISPATCHED
will not use the event loop in any case, even though the code is executed in-place.
Also, to add to the problem, I didn't find anything in the contract of https://kotlinlang.org/api/kotlinx.coroutines/kotlinx-coroutines-core/kotlinx.coroutines/-coroutine-dispatcher/is-dispatch-needed.html that prohibited side effects, so maybe every CoroutineStart
entry should either document whether and when it invokes that function or explicitly state that there is this is undefined...
To sum it up, I do not understand the mental model of CoroutineStart
that does not include an explicit description of how isDispatchNeeded = false
is handled. Furthermore, https://github.com/Kotlin/api-guidelines/blob/main/team_docs/samples.md#functions-returning-a-result from the doc you linked suggests that, since isDispatchNeeded
affects the behavior of CoroutineStart
significantly, it should be included as a sample.
That's my current understanding, though of course, I will concur with any decision regarding this.
* - [ATOMIC] -- atomically (in a non-cancellable way) schedules coroutine for execution according to its context; | ||
* - [UNDISPATCHED] -- immediately executes coroutine until its first suspension point _in the current thread_. | ||
* - [DEFAULT] immediately schedules the coroutine for execution according to its context. | ||
* - [LAZY] delays the moment of the initial dispatch until the result of the coroutine is awaited. |
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.
* - [LAZY] delays the moment of the initial dispatch until the result of the coroutine is awaited. | |
* - [LAZY] delays the moment of the initial dispatch until the result of the coroutine is needed. |
The distinction is important because of e.g. LazyActorCoroutine
where it starts when the message is send to a channel
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.
Fixed.
* println("1. About to start a coroutine not needing a dispatch.") | ||
* // Dispatch the job to execute. | ||
* // `Dispatchers.Unconfined` is explicitly chosen. | ||
* val job = launch(Dispatchers.Unconfined) { |
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 see, thanks.
The mental model (here, and, in fact, of any API) is that different concepts are completely orthogonal, and one doesn't have to think about them in total, only separately (otherwise, the learning curve is unreasonable steep).
isDispatchNeeded
is a dispatcher property that hints at how exactly the coroutine is dispatched. Coroutine start is about the start, and it's completely orthogonal in every case except UNDISPATCHED
(and that's the place where it's worth mentioning explicitly).
With such separation in mind, there is no need to enumerate every possibility (the same way we don't enumerate how DEFAULT
+ cancellation + NonCancellable
works and that ATOMIC
here does not make any difference), only the core concept -- the start.
UNDISPATCHED
here is worth a special note in its own documentation.
Hope that clarifies the idea! Having that in mind, it may become more obvious why the Unconfined
example is unnecessary here, and in the third example, one coroutine is enough.
Small side note: ATOMIC
does not "prohibit" cancellation, it delays it to the first cancellation check (whether it's a suspension point or ensureActive
).
* This is similar to [DEFAULT], but the coroutine is guaranteed to start executing even if it was cancelled. | ||
* This only affects the initial portion of the code: on subsequent suspensions, cancellation will work as usual. | ||
* | ||
* [UNDISPATCHED] also ensures that coroutines will be started in any case. |
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.
Please see my explanation about orthogonal concepts -- I think it explains why this section is unnecessary
* println("1. About to start a coroutine not needing a dispatch.") | ||
* // Dispatch the job to execute. | ||
* // `Dispatchers.Unconfined` is explicitly chosen. | ||
* val job = launch(Dispatchers.Unconfined) { |
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 would assume that the confusion comes from the fact that we mentioned isDispatchNeeded
in the original documentation to DEFAULT
.
I maybe made sense back then (2017!) when everything was experimental and we had no idea what coroutines are, but I wouldn't do it nowadays
Seeing this demoted to a draft, is it correct that we are waiting for your next updates? |
The idea was to do this:
(The quote is from #4133 (comment)) |
Thanks for the reminder. We'll take it from here in that case! Thanks for the bootstrapping work and revamp! 👍 |
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.
Very cool and thorough rework of the docs! 🤩 Left a few notes, mainly re. the conciseness and complexity of some of the samples.
All of them except `LAZY` mentioned that cancellability at suspension points depends on the specific suspending functions, and it looks like it applies to `LAZY`, so this information is moved to `CoroutineStart`'s top-level documentation.
Presumably, the import statement was used to ensure that the symbols in the documentation are properly resolved. It does not seem to be necessary at the moment.
After playing around with CoroutineStart.LAZY, I failed to understand when it can be useful.
Co-authored-by: Vsevolod Tolstopyatov <qwwdfsad@gmail.com>
ddfe8f4
to
404e155
Compare
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.
Amazingly done!
Just one question re. idiomatic-ness of val work by lazy { scope.async { println("Running!"); 5 } }
work.invokeOnCompletion { // this access will start the coroutine
println("Completed!")
}
delay(5000)
println(work.await()) |
I'm not sure how realistic this concern is, but adding a line of text to warn people about it surely can't hurt. Done. |
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.
Looks great! 🚀
One of the pieces of #4133.
@fzhinkin, could you take a look?