fix: properly remove subscriptions in ReplayOperator

[markusdlugi]

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]

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:

After:

[codecov]

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

@@             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

[jponge]

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]

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]

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]

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

[markusdlugi]

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.

[jponge]

Many thanks for spotting and resolving this issue!