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: properly remove subscriptions in ReplayOperator #1483

Merged
merged 1 commit into from
Jan 12, 2024

Conversation

markusdlugi
Copy link
Contributor

Fixes a memory leak caused by old subscriptions not being removed properly. In addition, this fixes the edge case where no completion event would be sent for replay subscriptions if the number of requested items was exactly equal to the number of available items.

Fixes #1482

@markusdlugi
Copy link
Contributor Author

We tested this fix with our application that had the memory leak, and it is working fine now again. Comparison of the application's heap usage during our load test before and after the fix:

Before:
image

After:
image

Copy link

codecov bot commented Jan 11, 2024

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (d855d57) 89.30% compared to head (90c2609) 89.28%.
Report is 1 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##               main    #1483      +/-   ##
============================================
- Coverage     89.30%   89.28%   -0.02%     
- Complexity     3362     3365       +3     
============================================
  Files           459      459              
  Lines         13399    13405       +6     
  Branches       1640     1642       +2     
============================================
+ Hits          11966    11969       +3     
  Misses          802      802              
- Partials        631      634       +3     
Files Coverage Δ
...y/operators/multi/replay/AppendOnlyReplayList.java 94.02% <0.00%> (-1.43%) ⬇️
.../mutiny/operators/multi/replay/ReplayOperator.java 91.83% <87.50%> (-0.64%) ⬇️

... and 9 files with indirect coverage changes

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.

Thanks for the PR, it's definitely going to fix a bug.

See my comments for changes.

I'd have preferred to keep private access rather than protected, but it definitely became apparent why you had done so given the final test. Can we think of another way to write this test without:

  1. going from private to protected, and
  2. not having to directly instantiate the operator in the test case? (our tests usually create Multi pipelines from the user facing API rather than instantiating internals)

Thanks!

@jponge jponge added this to the 2.5.4 milestone Jan 12, 2024
@jponge jponge added the bug Something isn't working label Jan 12, 2024
@jponge jponge changed the title fix: Properly remove subscriptions in ReplayOperator fix: properly remove subscriptions in ReplayOperator Jan 12, 2024
@jponge
Copy link
Member

jponge commented Jan 12, 2024

Also: you have a merge commit from main, I'd rather see 1 commit on top of main (you can always rebase). If you're unsure I can always do it myself when the PR is ready to be merged.

@markusdlugi
Copy link
Contributor Author

Thanks for the review! I will incorporate the requested changes. Regarding this:

I'd have preferred to keep private access rather than protected, but it definitely became apparent why you had done so given the final test. Can we think of another way to write this test without:

  1. going from private to protected, and
  2. not having to directly instantiate the operator in the test case? (our tests usually create Multi pipelines from the user facing API rather than instantiating internals)

Apart from making the list protected, I can only think of the following alternatives, which one do you prefer?

  • A getter in ReplayOperator to make the subscriptions list accessible - potentially wrapped in an unmodifiable collection
  • A new constructor in the ReplayOperator to be able to set your own subscriptions list on instantiation
  • Try to access the private attribute using reflection magic

I don't see another way of ensuring that the subscriptions are properly removed after completion without access to the list. But even then, I wouldn't be able to do this without working with the ReplayOperator directly, at the very least casting to it after instantiating it via Multi.createBy().replaying(). I fear you can't have your cake and eat it, too :)

If none of these options are viable, we'd have to scrap the test. But then you won't have any safety net in case of a regression.

@jponge
Copy link
Member

jponge commented Jan 12, 2024

I agree the protected route is the cleanest, we can't have it all 😉

Fixes a memory leak caused by old subscriptions not being removed properly.
In addition, this fixes the edge case where no completion event would be sent for replay
subscriptions if the number of requested items was exactly equal to the number of available items.

Fixes smallrye#1482
@markusdlugi
Copy link
Contributor Author

Alright then, thanks 👍
I adjusted the naming of the test methods and rebased onto main as requested, so it should be good to go now, hopefully.

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.

Many thanks for spotting and resolving this issue!

@jponge jponge merged commit 44894c1 into smallrye:main Jan 12, 2024
6 of 7 checks passed
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.

Memory Leak when using Multi Replay
2 participants