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

Fix Flow.timeout swallowing the channel closure exception #4072

Merged
merged 1 commit into from
Mar 25, 2024

Conversation

dkhalanskyjb
Copy link
Collaborator

Fixes #4071

@dkhalanskyjb dkhalanskyjb requested a review from qwwdfsad March 18, 2024 14:08
@@ -394,6 +394,7 @@ private fun <T> Flow<T>.timeoutInternal(
value.onSuccess {
downStream.emit(it)
}.onClosed {
it?.let { throw it }
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it correct that the reason this issue manifested itself is our fusing optimization?

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, it boils down to the following:

val job = Job()
val produce = produce<Int>(job) { close(TestException()) }
(produce as Job).join()
job.isCancelled // False

Which makes sense for produce specifically (when it's used as a standalone coroutine builder), but not much when used as part of the flow machinery

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, kind of confusing semantics-wise: close with a cause fails the channel, and when a coroutine fails, the channel fails as well. Yet the channel failure on its now does not represent failure.

@dkhalanskyjb dkhalanskyjb merged commit 0ca7358 into master Mar 25, 2024
1 check passed
@dkhalanskyjb dkhalanskyjb deleted the dk-fix-flow-timeout-swallowing-exceptions branch March 25, 2024 13:16
dkhalanskyjb added a commit that referenced this pull request Mar 25, 2024
Fix `Flow.timeout` swallowing the channel closure exception (#4072)

Was merged into `master` instead of `develop` by mistake.
knisht pushed a commit to JetBrains/intellij-deps-kotlinx.coroutines that referenced this pull request Apr 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Callbackflow close exception misbehavior with timeout
2 participants