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

Kotlin-Module: Cancel Multi subscription in case of cancelled coroutine context #734

Merged
merged 1 commit into from
Oct 25, 2021

Conversation

heubeck
Copy link
Collaborator

@heubeck heubeck commented Oct 24, 2021

Investivated and fixed due to issue #733:
A Kotlin Flow 'first()' aborts the Flow, this was not propagated to the
MultiSubscriber. Only after the complete Multi was produced, the Flow
terminated. It affected not every Multi, could reproduce it with a
Multi.fromEmitter, but not with a Multi.fromTicks.

Not absolutely sure, if it fixes #733, but I could reproduce a similar behavior (see the newly added tests).

Remember, we've discussed the meaning of the CancellationException, and decided to completely suppress it, as it's (also) a side effect of a coroutine context cancellation, Julien? Feeling a bit stupid now, to not have tested the simple case of a Flow.first()...

@codecov
Copy link

codecov bot commented Oct 24, 2021

Codecov Report

Merging #734 (07dadab) into main (fe6f537) will increase coverage by 0.22%.
The diff coverage is 64.70%.

Impacted file tree graph

@@             Coverage Diff              @@
##               main     #734      +/-   ##
============================================
+ Coverage     89.83%   90.05%   +0.22%     
- Complexity     2986     2994       +8     
============================================
  Files           374      374              
  Lines         11790    11797       +7     
  Branches       1478     1484       +6     
============================================
+ Hits          10591    10624      +33     
+ Misses          616      598      -18     
+ Partials        583      575       -8     
Impacted Files Coverage Δ
...main/kotlin/io/smallrye/mutiny/coroutines/Multi.kt 82.85% <64.70%> (-17.15%) ⬇️
...subscription/SwitchableSubscriptionSubscriber.java 95.08% <0.00%> (-2.46%) ⬇️
...mallrye/mutiny/operators/multi/MultiFlatMapOp.java 86.46% <0.00%> (+1.23%) ⬆️
...erators/multi/builders/SerializedMultiEmitter.java 82.27% <0.00%> (+1.26%) ⬆️
...tiny/operators/multi/MultiBufferWithTimeoutOp.java 74.78% <0.00%> (+1.68%) ⬆️
...y/operators/multi/processors/UnicastProcessor.java 97.41% <0.00%> (+1.72%) ⬆️
...perators/multi/processors/SerializedProcessor.java 84.15% <0.00%> (+1.98%) ⬆️
...y/operators/multi/builders/IterableBasedMulti.java 84.04% <0.00%> (+2.12%) ⬆️
...java/io/smallrye/mutiny/helpers/Subscriptions.java 81.76% <0.00%> (+2.76%) ⬆️
...mallrye/mutiny/operators/multi/FlatMapManager.java 98.61% <0.00%> (+2.77%) ⬆️
... and 3 more

…ne context

Investivated and fixed due to issue 733:
A Kotlin Flow 'first()' aborts the Flow, this was not propagated to the
MultiSubscriber. Only after the complete Multi was produced, the Flow
terminated. It affected not every Multi, could reproduce it with a
Multi.fromEmitter, but not with a Multi.fromTicks.

Signed-off-by: Florian Heubeck <[email protected]>
@heubeck heubeck force-pushed the fix/issue733-BlockingKotlinFlow branch from cc9c64d to 07dadab Compare October 25, 2021 07:08
Copy link
Member

@jponge jponge left a comment

Choose a reason for hiding this comment

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

Looks good, thanks for digging this one!

Let's hear from @pwlan on confirming that this fixes #733.

@jponge jponge added this to the 1.2.0 milestone Oct 25, 2021
@heubeck
Copy link
Collaborator Author

heubeck commented Oct 25, 2021

Looks good, thanks for digging this one!

Let's hear from @pwlan on confirming that this fixes #733.

Shall we already merge it @jponge , as it's an bugfix anyhow, and reinvestigate the RestEasy Reactive thing separately, if not fixed by this?

@jponge
Copy link
Member

jponge commented Oct 25, 2021

Agreed, you can merge!

@heubeck heubeck merged commit 93c0b64 into main Oct 25, 2021
@heubeck heubeck deleted the fix/issue733-BlockingKotlinFlow branch October 25, 2021 10:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Quarkus + Mutiny Kotlin - io.vertx.core.VertxException: Thread blocked
2 participants