Skip to content

Improve docs for CoroutineExceptionHandler #1886

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 5 commits into from
Apr 22, 2020
Merged

Improve docs for CoroutineExceptionHandler #1886

merged 5 commits into from
Apr 22, 2020

Conversation

elizarov
Copy link
Contributor

Fixes #1746

@elizarov elizarov requested a review from qwwdfsad March 30, 2020 16:42
@elizarov elizarov mentioned this pull request Mar 30, 2020
Closed
elizarov and others added 2 commits March 31, 2020 17:51
@elizarov @EdwarDDay
Update kotlinx-coroutines-core/common/src/CoroutineExceptionHandler.kt
9f6d4ff 
Co-Authored-By: EdwarDDay <[email protected]>
@elizarov
Further clarifications and better style for exception handling
ec111bb 
* Consistent terminology on "uncaught exceptions".
* Clarified special relations of exception handling with supervision.
* Clearer text in CoroutineExceptionHandler examples.
* Minor stylistic corrections.

Fixes #871
@elizarov
Copy link
Contributor Author

elizarov commented Apr 1, 2020

Another go. Also fixes #871

@mtopolnik
Copy link
Contributor

I've read through these docs again, I couldn't find any statement about catching CancellationException. You can catch a CancellationException and if you don't take care to rethrow it, it prevents the coroutine from completing. This does conform to my expectation from Java, but I could also imagine Kotlin generating such bytecode that always catches and rethrows a CancellationException. So I had to go and try it to know for sure.

We have statements like

it is ignored by the coroutines' machinery

these exceptions are ignored by all handlers

Lacking an explicit statement about the catch-blocks, the user can be easily misled.

@elizarov elizarov requested a review from qwwdfsad April 7, 2020 16:49
qwwdfsad
qwwdfsad approved these changes Apr 21, 2020
View reviewed changes
Copy link
Collaborator

@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.

Great!

@elizarov
Copy link
Contributor Author

@mtopolnik

I've read through these docs again, I couldn't find any statement about catching CancellationException.

I'm not sure where would be the right place (one that users will notice) to add additional details on cancellation exception. It does not seem related to the docs for CoroutineExceptionHandler, though, so I'm merging this doc improvements as is.

@elizarov elizarov merged commit 94970df into develop Apr 22, 2020
@elizarov elizarov deleted the exception-handler-doc branch April 22, 2020 10:58
qwwdfsad pushed a commit that referenced this pull request Apr 24, 2020
@elizarov @EdwarDDay @qwwdfsad
Improve docs for CoroutineExceptionHandler (#1886)
4589d4c 
* Further clarifications and better style for exception handling
* Consistent terminology on "uncaught exceptions".
* Clarified special relations of exception handling with supervision.
* Clearer text in CoroutineExceptionHandler examples.
* Minor stylistic corrections.

Fixes #1746
Fixes #871

Co-Authored-By: EdwarDDay <[email protected]>
recheej pushed a commit to recheej/kotlinx.coroutines that referenced this pull request Dec 28, 2020
@elizarov @EdwarDDay @recheej
Improve docs for CoroutineExceptionHandler (Kotlin#1886)
33f923d 
* Further clarifications and better style for exception handling
* Consistent terminology on "uncaught exceptions".
* Clarified special relations of exception handling with supervision.
* Clearer text in CoroutineExceptionHandler examples.
* Minor stylistic corrections.

Fixes Kotlin#1746
Fixes Kotlin#871

Co-Authored-By: EdwarDDay <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@EdwarDDay EdwarDDay EdwarDDay left review comments

@mtopolnik mtopolnik mtopolnik approved these changes

@qwwdfsad qwwdfsad qwwdfsad approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@elizarov @mtopolnik @qwwdfsad @EdwarDDay