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.

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

Improve exception transparency: explicitly allow throwing exceptions … #3017

Merged
merged 7 commits into from
Nov 16, 2021
Merged

Improve exception transparency: explicitly allow throwing exceptions … #3017

merged 7 commits into from
Nov 16, 2021

Conversation

qwwdfsad
Copy link
Contributor

…from the upstream when the downstream has been failed, but also suppress such exceptions by the downstream one

It solves the problem of graceful shutdown: when the upstream fails unwillingly (e.g. file.close() has thrown in 'finally') block, we cannot treat is as an exception transparency violation (hint: 'finally' is a shortcut for 'catch'+body), but we also cannot leave things as is, otherwise it leads to unforeseen consequences such as successful 'retry' and 'catch' operators that may, or may not, then fail with exception on attempt to emit.

Downstream exception supersedes the upstream exception only if it is not an instance of CancellationException, semantically emulating cancellation-friendly 'use' block.

Fixes #2860

@qwwdfsad
Improve exception transparency: explicitly allow throwing exceptions …
b3eade1 
…from the upstream when the downstream has been failed, but also suppress such exceptions by the downstream one

It solves the problem of graceful shutdown: when the upstream fails unwillingly (e.g. file.close() has thrown in 'finally') block, we cannot treat is as an exception transparency violation (hint: 'finally' is a shortcut for 'catch'+body), but we also cannot leave things as is, otherwise it leads to unforeseen consequences such as successful 'retry' and 'catch' operators that may, or may not, then fail with exception on attempt to emit.

Downstream exception supersedes the upstream exception only if it is not an instance of CancellationException, semantically emulating cancellation-friendly 'use' block.

Fixes #2860
@qwwdfsad @dkhalanskyjb
Update kotlinx-coroutines-core/common/src/flow/Flow.kt
08a3ed3 
Co-authored-by: dkhalanskyjb <52952525+dkhalanskyjb@users.noreply.github.com>
@qwwdfsad
Copy link
Contributor Author

I've changed the contract, so the

 val flow = flow {
     try {
         emit(1)
     } finally {
         throw TestException()
     }
 }.catch { expectUnreached() }.onEach { throw TestException2() }

and

 val flow = flow {
     try {
         emit(1)
     } finally {
         throw TestException()
     }
 }.onEach { throw TestException2() }

now throw the same exception. Please re-review it carefully one more time

@qwwdfsad
Change the exception suppression invariant to work backwards:
9227f94 
All the benefits of the approach stay the same, but additionally, for an arbitrary flow pipeline, adding a catch operator that is not triggered will no longer change the type of resulting exception
@qwwdfsad qwwdfsad force-pushed the tweak-exception-transparency branch from 935b83a to 9227f94 Compare November 12, 2021 15:22
dkhalanskyjb
dkhalanskyjb reviewed Nov 16, 2021
View reviewed changes
Copy link
Collaborator

@dkhalanskyjb dkhalanskyjb left a comment

Choose a reason for hiding this comment

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

All tests are for the situation where upstream fails with TestException; nothing checks that the downstream exception is chosen if the upstream throws a CancellationException.

@qwwdfsad qwwdfsad merged commit 9099b03 into develop Nov 16, 2021
@qwwdfsad qwwdfsad deleted the tweak-exception-transparency branch November 16, 2021 16:10
yorickhenning pushed a commit to yorickhenning/kotlinx.coroutines that referenced this pull request Jan 28, 2022
@qwwdfsad @yorickhenning
Improve exception transparency: explicitly allow throwing exceptions … (
a75ac1a 
Kotlin#3017)

* Improve exception transparency: explicitly allow throwing exceptions from the upstream when the downstream has been failed, suppress the downstream exception by the new one, but still ignore it in the exception handling operators, that still consider the flow failed.

It solves the problem of graceful shutdown: when the upstream fails unwillingly (e.g. `file.close()` has thrown in `finally` block), we cannot treat it as an exception transparency violation (hint: `finally` is a shortcut for `catch` + body that rethrows in the end), but we also cannot leave things as is, otherwise, it leads to unforeseen consequences such as successful `retry` and `catch` operators that may, or may not, then fail with an exception on an attempt to emit.

Upstream exception supersedes the downstream exception only if it is not an instance of `CancellationException`, semantically emulating cancellation-friendly 'use' block.

Fixes Kotlin#2860
pablobaxter pushed a commit to pablobaxter/kotlinx.coroutines that referenced this pull request Sep 14, 2022
@qwwdfsad @pablobaxter
Improve exception transparency: explicitly allow throwing exceptions … (
4aa9b22 
Kotlin#3017)

* Improve exception transparency: explicitly allow throwing exceptions from the upstream when the downstream has been failed, suppress the downstream exception by the new one, but still ignore it in the exception handling operators, that still consider the flow failed.

It solves the problem of graceful shutdown: when the upstream fails unwillingly (e.g. `file.close()` has thrown in `finally` block), we cannot treat it as an exception transparency violation (hint: `finally` is a shortcut for `catch` + body that rethrows in the end), but we also cannot leave things as is, otherwise, it leads to unforeseen consequences such as successful `retry` and `catch` operators that may, or may not, then fail with an exception on an attempt to emit.

Upstream exception supersedes the downstream exception only if it is not an instance of `CancellationException`, semantically emulating cancellation-friendly 'use' block.

Fixes Kotlin#2860
@qwwdfsad qwwdfsad mentioned this pull request Jul 18, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dkhalanskyjb dkhalanskyjb dkhalanskyjb approved these changes

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

Successfully merging this pull request may close these issues.
2 participants
@qwwdfsad @dkhalanskyjb