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

1.x: fix and deprecate evicting groupBy and add new overload #5917

Merged
merged 2 commits into from Mar 19, 2018

Conversation

davidmoten
Copy link
Collaborator

See #5868

There is a problem with the existing 1.x Observable.groupBy overload with an evicting map factory where depending on the eviction behaviour of the supplied map factory, groups may not be completed and may leak memory. The problem extends to the method signature in that the eviction action should report the evicted GroupedObservable not the corresponding key.

This PR

  • fixes the existing method at the expense of doubling up the internal map groups ( into groupsCopy) so that we can always successfully lookup the evicted group from the groupsCopy map. Evictions from groups are mirrored in groupsCopy after the lookup.
  • deprecates the problematic overload indicating that it uses more memory than the preferred new overload
  • adds a new method groupBy that has the corrected signature and offers bufferSize and delayError parameters for a bit more flexibility and to differentiate the erased signature from the deprecated method
  • adds tests that were ported from the 2.x tests

@codecov
Copy link

codecov bot commented Mar 15, 2018

Codecov Report

Merging #5917 into 1.x will decrease coverage by 0.18%.
The diff coverage is 91.33%.

Impacted file tree graph

@@             Coverage Diff             @@
##               1.x    #5917      +/-   ##
===========================================
- Coverage     84.2%   84.01%   -0.19%     
+ Complexity    2894     2893       -1     
===========================================
  Files          290      291       +1     
  Lines        18265    18562     +297     
  Branches      2495     2554      +59     
===========================================
+ Hits         15380    15595     +215     
- Misses        2010     2053      +43     
- Partials       875      914      +39
Impacted Files Coverage Δ Complexity Δ
src/main/java/rx/Observable.java 99.28% <60%> (-0.36%) 450 <3> (+1)
...in/java/rx/internal/operators/OperatorGroupBy.java 76.55% <75%> (-14.97%) 5 <0> (ø)
...rx/internal/operators/OperatorGroupByEvicting.java 92.33% <92.33%> (ø) 5 <5> (?)
.../java/rx/internal/util/unsafe/MpscLinkedQueue.java 75.75% <0%> (-15.16%) 9% <0%> (-2%)
...ternal/operators/OperatorOnBackpressureLatest.java 78.84% <0%> (-1.93%) 3% <0%> (ø)
...x/internal/operators/DeferredScalarSubscriber.java 98.27% <0%> (-1.73%) 24% <0%> (-1%)
...in/java/rx/internal/operators/OnSubscribeRedo.java 80.8% <0%> (-1.61%) 16% <0%> (ø)
...a/rx/internal/operators/BufferUntilSubscriber.java 71.42% <0%> (-1.59%) 11% <0%> (-1%)
...c/main/java/rx/observables/BlockingObservable.java 85.21% <0%> (-1.41%) 37% <0%> (-1%)
... and 7 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 6e6c514...99b1752. Read the comment docs.

@davidmoten
Copy link
Collaborator Author

Added a null check because a race with cancel could clear groupsCopy

@akarnokd akarnokd merged commit e467798 into ReactiveX:1.x Mar 19, 2018
@davidmoten
Copy link
Collaborator Author

Can we get another 1.x release with this change soonish?

@akarnokd
Copy link
Member

Done.

@davidmoten
Copy link
Collaborator Author

Thank you!

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.

None yet

2 participants