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

No need to release downstream subscriber references #712

Merged
merged 1 commit into from Oct 7, 2021
Merged

Conversation

jponge
Copy link
Member

@jponge jponge commented Oct 7, 2021

We used to have an over interpretation of the RS TCK that downstream subscriber references had to be null on cancellation.

It's actually not necessary (and NPE-prone) since operators are instantiated per-subscription, so the publisher does not actually keep references on cancelled subscribers.

Fixes #705

We used to have an interpretation of the RS TCK that it had to be null on cancellation to release the subscriber.
It's actually not necessary (and NPE-prone) since operators are instantiated per-subscription, so the *publisher* does not actually keep references on cancelled subscribers.

Fixes #705
@codecov
Copy link

codecov bot commented Oct 7, 2021

Codecov Report

Merging #712 (408d0b7) into main (45615fd) will increase coverage by 0.29%.
The diff coverage is 70.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##               main     #712      +/-   ##
============================================
+ Coverage     89.94%   90.24%   +0.29%     
- Complexity     2985     2999      +14     
============================================
  Files           374      374              
  Lines         11791    11789       -2     
  Branches       1478     1480       +2     
============================================
+ Hits          10606    10639      +33     
+ Misses          609      586      -23     
+ Partials        576      564      -12     
Impacted Files Coverage Δ
...smallrye/mutiny/operators/multi/MultiWindowOp.java 90.83% <0.00%> (ø)
...mutiny/operators/multi/MultiOperatorProcessor.java 90.38% <100.00%> (-0.36%) ⬇️
...a/io/smallrye/mutiny/helpers/BlockingIterable.java 88.77% <0.00%> (-1.03%) ⬇️
...mallrye/mutiny/operators/multi/MultiFlatMapOp.java 86.15% <0.00%> (-0.31%) ⬇️
...utiny/operators/multi/MultiWindowOnDurationOp.java 70.71% <0.00%> (ø)
...iny/operators/multi/builders/BaseMultiEmitter.java 83.33% <0.00%> (ø)
...operators/multi/builders/CollectionBasedMulti.java 93.54% <0.00%> (ø)
...erators/multi/builders/SerializedMultiEmitter.java 81.01% <0.00%> (ø)
...java/io/smallrye/mutiny/helpers/Subscriptions.java 81.21% <0.00%> (+2.20%) ⬆️
...subscription/SwitchableSubscriptionSubscriber.java 99.18% <0.00%> (+2.45%) ⬆️
... and 5 more

@jponge
Copy link
Member Author

jponge commented Oct 7, 2021

Also waiting for a check of this branch from the reporter in #705

@indalaterre
Copy link
Contributor

Perfect this fix solved the problem. Thanks!

@jponge jponge merged commit 8aa8f05 into main Oct 7, 2021
@jponge jponge deleted the fix/705 branch October 7, 2021 18:44
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.

Cannot invoke "MultiSubscriber.onCompletion()" because "this.downstream" is null
3 participants