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.

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

Improve exception transparency: explicitly allow throwing exceptions … #3017

Merged
merged 7 commits into from Nov 16, 2021

Conversation

qwwdfsad
Copy link
Member

@qwwdfsad qwwdfsad commented Nov 11, 2021

…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

…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 qwwdfsad requested a review from dkhalanskyjb Nov 11, 2021
Co-authored-by: dkhalanskyjb <52952525+dkhalanskyjb@users.noreply.github.com>
@qwwdfsad qwwdfsad requested a review from dkhalanskyjb Nov 12, 2021
@qwwdfsad
Copy link
Member Author

@qwwdfsad qwwdfsad commented Nov 12, 2021

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

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 Nov 12, 2021
@qwwdfsad qwwdfsad requested a review from dkhalanskyjb Nov 16, 2021
Copy link
Member

@dkhalanskyjb dkhalanskyjb left a comment

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 requested a review from dkhalanskyjb Nov 16, 2021
@qwwdfsad qwwdfsad merged commit 9099b03 into develop Nov 16, 2021
@qwwdfsad qwwdfsad deleted the tweak-exception-transparency branch Nov 16, 2021
yorickhenning pushed a commit to yorickhenning/kotlinx.coroutines that referenced this issue Jan 28, 2022
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

None yet

2 participants