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

3.x: Fix Flowable.groupBy eviction logic double decrement and hang #6975

Merged
merged 1 commit into from
May 10, 2020

Conversation

akarnokd
Copy link
Member

@akarnokd akarnokd commented May 9, 2020

When a group is evicted, the group is synchronously onCompleted during an onNext. If a cancellation happened during this time (for example, when a publish cancels its upstream because its output completes), the cancellation would unconditionally decrement the group counter even though the groups map had no longer the group (because it was evicted before). Then once the eviction logic finishes, it decremented the group counter again, leading to invalid internal state and hangs.

The fix is to only decrement if there was a group actually removed from the map.

Fixes #6974

@akarnokd akarnokd added this to the 3.1 milestone May 9, 2020
@codecov
Copy link

codecov bot commented May 9, 2020

Codecov Report

Merging #6975 into 3.x will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##                3.x    #6975      +/-   ##
============================================
+ Coverage     99.65%   99.66%   +0.01%     
- Complexity     6670     6671       +1     
============================================
  Files           742      742              
  Lines         47254    47254              
  Branches       6370     6371       +1     
============================================
+ Hits          47090    47098       +8     
+ Misses           51       47       -4     
+ Partials        113      109       -4     
Impacted Files Coverage Δ Complexity Δ
...3/internal/operators/flowable/FlowableGroupBy.java 99.68% <100.00%> (ø) 3.00 <0.00> (ø)
...xjava3/internal/observers/FutureMultiObserver.java 96.61% <0.00%> (-3.39%) 27.00% <0.00%> (-1.00%)
...rnal/operators/flowable/FlowableFlatMapSingle.java 95.34% <0.00%> (-2.33%) 2.00% <0.00%> (ø%)
...perators/single/SingleFlatMapIterableFlowable.java 98.33% <0.00%> (-1.67%) 2.00% <0.00%> (ø%)
...ternal/operators/completable/CompletableMerge.java 97.29% <0.00%> (-1.36%) 2.00% <0.00%> (ø%)
...ternal/operators/observable/ObservableGroupBy.java 98.90% <0.00%> (-1.10%) 2.00% <0.00%> (ø%)
.../operators/observable/ObservableCombineLatest.java 99.38% <0.00%> (-0.62%) 6.00% <0.00%> (ø%)
...a3/internal/operators/flowable/FlowableCreate.java 99.02% <0.00%> (+0.32%) 6.00% <0.00%> (ø%)
...ternal/operators/observable/ObservableFlatMap.java 98.58% <0.00%> (+0.35%) 3.00% <0.00%> (ø%)
...a/io/reactivex/rxjava3/subjects/ReplaySubject.java 99.58% <0.00%> (+0.41%) 49.00% <0.00%> (ø%)
... and 5 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 77c2ef1...4a12096. Read the comment docs.

Copy link
Collaborator

@davidmoten davidmoten left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @akarnokd

@akarnokd akarnokd merged commit eb58c59 into ReactiveX:3.x May 10, 2020
@akarnokd akarnokd deleted the GroupByEvictFlatMap branch May 10, 2020 08:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3.0.3 - Hangs when using combination of flatMap and publish
3 participants