Clarify the behavior of CoroutineExceptionHandler - #4596

You've somewhat created a bunch of new terms to replace a single old one. It used to be uncaught exceptions, now it is unpropagated exceptions, and it is introduced indirectly through mentions of a mix of wordings:

• clear propagation paths
• exception propagation paths
• clear exception propagation paths
• propagation channels

To mean "means of handling exceptions which come after try-catch but before CoroutineExceptionHandler".

It definitely needs to be standardised. I suggest either "exception propagation paths" or "propagation channels", or something else. (Drop the "clear". It makes it sound informal, but we do want to signal that it is a formal term).

No strong opinion on which term to use, as both have downsides and I cannot come up with a better one. "exception propagation paths" are too lengthy and "propagation channels" misleadingly mention channels.

"Propagation channels" is a nice catch! We have already discussed it once already under the PR with the structured concurrency documentation, and you raised a valuable point that "channel" is a specific term in concurrent programming. Removed all mentions of channels to prevent confusion.

Drop the "clear". It makes it sound informal, but we do want to signal that it is a formal term

I don't think that we do at that point. "Clear" only occurs in the first section, whose goal is to provide a high-level intuition of the forces at play. In my view, it is perfectly fine to add extra adjectives/verbosity to aid intuition there. The goal of "clear" is to contrast the propagation paths discussed here with those implied by the business logic. Generally, before it's clear that "propagation paths" is a precise characterization and not just a description of circumstances, "coroutine exceptions without a propagation path" looks meaningless to me, and "coroutine exceptions that kotlinx.coroutines can not propagate to where the client code can be expected to observe and react to them" feels detailed enough to obscure the high-level idea.

It feels somehow off to refer to the library name. I like how you refer to the magic coroutine machinery below. Use that?

The other usages of "coroutine machinery" (git grep machinery) indicate that it's the sum of JobSupport, CancellableContinuationImpl, etc.—a specific implementation of how coroutines interact, suspend, resume, etc. With that in mind, using "the coroutines machinery" here becomes misleading, as not only the low-level implementation of corouttines themselves follows this principle, but other parts of the library as well: git grep handleCoroutineException. The mention of kotlinx.coroutines here is intended to evoke the feeling of it being a general guiding principle for our APIs.

Too verbose.

ensures that every exception thrown in a coroutine is handled.

Your proposed formulation misses an important nuance. By "handling", people often mean "appropriately reacting to", but kotlinx.coroutines can't do that. The appropriate action heavily depends on the use case. The extra wording here weakens the "handling" to "somehow reacting to". I don't think rewording this would help with verbosity, either, as this is in any case an important point that shouldn't stay implicit.

I only took a prefix of your text.

"instead of being forgotten about" and "This helps failures not to go unnoticed, with the program silently entering an incorrect state" feel like general discussion on why it's important to handle exceptions, not relaxation of the meaning of "handle".

Let's relax the meaning of "handle" into "reacted to" and also shorten the discussion (which is useful to mention on its own merit)

kotlinx.coroutines ensures that every [exception][Throwable] thrown in a coroutine is reacted to by some code. This helps to draw attention to failures and notice when the program enters an incorrect state.

The problem isn't the exception hygiene and error handling best practices, though. It's that asynchronous computations don't have an inherent exception propagation path, the way exceptions can bubble up to the public static void main and crash the program if you leave them untouched on their way up.

A more disciplined, math-book-like way to structure this piece of documentation would be to extract a separate section called something like "Error handling in concurrent code" and explain in general terms that for queue-based-executor-based concurrency, some exceptions will inherently have nowhere to go. I was hoping to avoid this heavy methodical approach in favor of a specific example of an "intuitively" bad behavior the existence of CoroutineExceptionHandler aims to address.

Let's relax the meaning of "handle" into "reacted to"

We can't get rid of "handling" as a word entirely, since the interface is called CoroutineExceptionHandler. Might as well lean into it.

You want to communicate (a 10% of):

1. In an async framework, exception propagation is not as straightforward as in a regular program.
2. We can't truly handle exceptions without a propagation path, we can just react to them.

Just FYI, this quote does neither for me, doesn't even clue at these.

... instead of being forgotten about. This helps failures not to go unnoticed, with the program silently entering an incorrect state.

I understand how you went from 1+2 to this by simplifying, but this turned out to be a very erasing transformation.

And for what it is now, it looks just like a general discussion on exceptions.

1. I don't want this couple of sentences to communicate that, no. The purpose is to lead up to it. The example of supervisorScope and launch is supposed to demonstrate the problem in specific terms.
2. Likewise, I don't intend to make this contradiction. The idea is to clarify what exactly kotlinx.coroutines seeks to do, and that is that exceptions don't get completely forgotten about.

The ideas that asynchronously thrown exceptions are problematic + not being able to fully handle exceptions are the underlying reasons for this piece of text, not its message.

I'd also add an overview of the upcoming exception handling hierarchy:

1. try-catch
2. "exception propagation paths"
3. CoroutineExceptionHandler
4. final resort

The #2 could also benefit from a brief overview (lexically scoped, parent, return value (deferred)).

It would be better to put it in a very brief listicle rather than complete sentences and acknowledge that it doesn't provide the full information yet.

I'm wary that this would postpone explaining the general idea of what a CoroutineExceptionHandler is and when it's needed. Would be a waste to have a reader go through this list, figure out the exception propagation paths, and then realize that hey, their code is already completely idiomatic, there is no need to be concerned about such low-level details.

I appreciate that you want to cut to the chase as early as possible. Consider doing it even sooner with a tiny example after the first sentence (for those who are familiar but only want to lookup the syntax of adding a CEH), and then you'll be able to keep the narrative linear without forward referencing. Currently it reads, "if your coroutines don't have something vague, for example in this case, use CoroutineExceptionHandler".

You already mention the supervisorScope example twice: once in this section, and once at the end of the Propagation channels section.

If you add a quick supervisorScope example at the top and have a linear story, it will only appear once later, on the border of the Propagation Channels and CEH sections.

I attempted this and didn't like the result:

  * There are scenarios when there is no clear propagation path that `kotlinx.coroutines` can take to propagate an
  * exception. Example:
  *
  * ```
  * supervisorScope {
  *     launch { error("Failure") }
  * }
  * ```
  *
  * Here, the coroutine created by `launch` fails with the exception `"Failure"`.
  * If instead of [supervisorScope], [coroutineScope] were used, the exception would be propagated to the caller of
  * [coroutineScope]. First, the failed [launch] would call [Job.cancel] on the [coroutineScope], which would lead
  * to [coroutineScope] failing. Then, since [coroutineScope] returns its result to the caller, the exception would
  * be rethrown. `kotlinx.coroutines` considers this a propagation path.
  *
  * However, [supervisorScope] does not react to exceptions from its children, as opposed to [coroutineScope],
  * which means [Job.cancel] would not be called.
  *
  * In such cases, a [CoroutineExceptionHandler] should be used to process the exceptions:
  *
  * ```
  * withContext(CoroutineExceptionHandler { ctx, ex ->
  *     println("Exception $ex thrown from coroutine context $ctx")
  * }) {
  *     supervisorScope {
  *         launch { error("Failure") }
  *     }
  * }
  * ```
  *
  * In most scenarios, a clear exception propagation path allows processing failures in coroutines.
  * Code should normally be structured in a way that ensures the existence of exception propagation paths.
  * See "Propagation paths recognized by `kotlinx.coroutines`" below.

• The minor role of CoroutineExceptionHandler is not as prominent. "There are scenarios" does not give any indication that these scenarios are unusual, and I didn't find a more natural way of expressing this thought than just starting with the idea that CoroutineExceptionHandler is unnecessary in most cases.
• The logic is backwards: to explain why supervisorScope { launch } fails to meet the propagation path criteria, we have to introduce them afterwards, overloading the reader.

What about this? This uses "exception propagation paths" as a level of abstraction. Even though you don't know if this applies to you or not if you don't know what is "exception propagation paths", you get the big picture, and now you only need to learn if your current situation has a propagation paths already or not.

 * An optional element in the [CoroutineContext] to handle coroutine exceptions without a clear propagation path.
 *
 * Code should normally be structured in a way that ensures the existence 
 * of exception propagation paths.
 * For example, a [coroutineScope] call will rethrow an exception to the caller.
 * In rare scenarios when there is no exception propagation path,
 * a [CoroutineExceptionHandler] should be used to process exceptions.
 * In such cases, not using a [CoroutineExceptionHandler] is considered a programming error
 * and will invoke _last-resort exception handling_ which may crash the program.
 *
 * Here, the coroutine created by `launch` fails with the exception `"Failure"`
 * but [supervisorScope] does not react to exceptions from its children,
 * as opposed to [coroutineScope].
 *
 * ```
 * supervisorScope {
 *     launch { error("Failure") }
 * }
 * ```
 *
 * In such cases, a [CoroutineExceptionHandler] should be used to process the exceptions:
 *
 * ```
 * withContext(CoroutineExceptionHandler { ctx, ex ->
 *     println("Exception $ex thrown from coroutine context $ctx")
 * }) {
 *     supervisorScope {
 *         launch { error("Failure") }
 *     }
 * }
 * ```
 *
 * Read below on general philosophy, exception propagation paths, and _last-resort exception handling_.

In fact, it is not re-ordering of sections, it is a replacement for the intro section. The current intro section is hard to digest and get a big picture exactly because it already dives into details of a propagation path for coroutineScope + launch.

I'd say it's more important to give a cleaner big picture without one chunk ("exception propagation paths") then to give a harder-to-digest big picture with a vague idea of that chunk.

The idea behind the coroutineScope { launch { } } detour is that the reader will look at it, recognize their code is not affected by any problems CoroutineExceptionHandler is designed to solve, and move on.

In your proposed alternative, we risk involving the reader that may not at all be interested in the contents—there is no "happy fast path".

* Code should normally be structured in a way that ensures the existence of exception propagation paths.
* In rare scenarios when there is no exception propagation path,
* a [CoroutineExceptionHandler] should be used to process exceptions.

The reaction I expect: "Okay, so it turns out, I must structure my code somehow. I guess I'll need to go figure out how to do that. Why am I only learning about this here? The coroutine tutorial didn't mention anything about needing to structure my code in some specific way" Then, the reader ends up in the technical deep-dive into the propagation paths, which, ideally, they shouldn't even need. It takes effort to learn the details and then recognize that unknowingly, you've already been doing everything right.

We can plug the coroutineScope back in:

 * An optional element in the [CoroutineContext] to handle coroutine exceptions 
 * without a clear propagation path.
 *
 * Code should normally be structured in a way that ensures the existence 
 * of exception propagation paths.
 * For example, a [coroutineScope] call will rethrow an exception to the caller.

Edited my suggested snippet above to include this, and some other minor wordings.

The difference with the committed version is frontloading the form of the text.

The outline you provide already reads too abstract to me, I'm wary of losing the reader in these general words. Attempting to pad out the outline to a full text results in a yet longer span of technical information without anything to ground the reader using the concept they may be familiar with.

Can we by any chance add a hyperlink to anywhere where "lexically scoped" is defined?

"there exists"

Too many negations make it hard to parse. Maybe something like "This exception does not need to be propagated because it gets caught", to have the same form as the examples below ("This exception is propagated by...").

Consider something like scope.launch { ... }, so that it's not lexically scoped and does not interfere with the previous example.

Although I see that it's a bit tricky that it requires asserting that the scope must have a CEH installed.

Then maybe introduce the caller for the line 124, something like a function where coroutineScope is called.

it requires asserting that the scope must have a CEH installed.

This particular example requires asserting that the parent job is a normal one and not a SupervisorJob. Manually constructing such a scope would detract from the example and highlights CoroutineScope(), whose usage we want to discourage. In any case, it's unclear to me what issue you're describing. The example is the simplest one that illustrates children propagating exceptions to their parents.

Simplify and use a single form for all "propagated" mentions in all three examples, or at least this one and the next one: "an exception is propagated if ..."

I don't see the value in it, and there are slight semantic differences between the three cases.

• In the first scenario, rethrowing is the direct act of propagation.
• In the second scenario, being transferred is like a checkmark on the propagation sheet: it may not be the only one, or even the most important act of propagation, but it suffices.
• In the third scenario, the exception isn't meaningfully propagated in the program-logic sense. Instead, kotlinx.coroutines considers its propagation job done if the exception can in theory be queried afterwards somehow.

"cannot"

Failure -> Won't ever be invoke, to be consistent with other examples

"in the given context" can be misinterpreted as if the handler is called from within the provided context.

Spell out the rules according to which this example works:

launch has a propagation path, so the error is propagated to the parent, and the CEH will never be invoked

Spell out "this" to avoid accidentally referring to the previous sentence and specify where the try-catch should go (there are two options: inside and outside launch).

To avoid the parent coroutine from being cancelled from a launch exception, wrap the call to launch into a try-catch (and also possibly reconsider your design?)

Something is confusing me in this example, but I can't quite pinpoint it. I'll take a look at it again after the next set of changes.

Possibly, it's that there are a few mistakes/misconceptions combined together.

• A CEH doesn't prevent a coroutine from failing.
• A CEH is not called if there exists a propagation path.
• And possibly something else?

Your analysis is spot-on. Indeed, to make a mistake shown in this section, you need to be completely unaware of what coroutine exception handlers do. We can expect people to be that unaware, though! Context: #1886 (comment)
Also, believing CoroutineExceptionHandler behaves as catch does isn't even unreasonable. In languages with effect handlers, you can redefine the behavior of throw. Even absent that, it's quite natural to believe an exception handler will at least prevent a coroutine from cancelling its parent: after all, the exception was "handled".