Fix issue in ShareReplay publisher Switch to removing subscriptions after completion is sent

[jabeattie]

Hey folks, you may remember my previous PR: #86

In this I tried to fix a memory leak that we'd identified, however, it seems like this solution caused a different problem that we've only uncovered when updating to 1.5.0.

I'm not 100% of what in our usage caused the issue - it is weirdly only presented in tests -but I've checked that this change both allows the tests in our consuming app to pass and works when the app is running for users.

I've removed the call to cancel() and instead clear the subscriptions after they are forwarded a completion. This seems to make sense to me logically, as once a subscriber has received a completion it can't do anything else.

I appreciate however that this is a change that had a problem for a new unknown change. If you aren't sure about this, I can instead revert my original PR. This would bring back the memory leak, but avoid the failing scenario we are seeing

[jabeattie]

@jasdev I know you reviewed the previous PR, would you be able to take a look at this?

[codecov]

Codecov Report

Merging #109 (e413147) into main (fc3e405) will decrease coverage by 0.10%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #109      +/-   ##
==========================================
- Coverage   97.23%   97.12%   -0.11%     
==========================================
  Files          62       62              
  Lines        3323     3340      +17     
==========================================
+ Hits         3231     3244      +13     
- Misses         92       96       +4     

Impacted Files	Coverage Δ
Sources/Subjects/ReplaySubject.swift	100.00% <100.00%> (ø)
Sources/Operators/WithLatestFrom.swift	93.15% <0.00%> (-6.85%)	⬇️
Tests/WithLatestFromTests.swift	100.00% <0.00%> (ø)
Sources/Common/Sink.swift	57.50% <0.00%> (+2.50%)	⬆️

---

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 fc3e405...e413147. Read the comment docs.

[jasdev]

Seems reasonable to me after a quick check — any chance we can add a test to make sure this doesn’t accidentally regress down the line? 🙏🏽

[jabeattie]

Yeah absolutely. I'll try and write a failing test for the issue we experienced

[freak4pc]

so the point of this is to remove all of the subscriptions upon completion?

[troupmar]

Hello @jabeattie
I have created a bug fix (#120) related to incorrect behavior of the share-reply operator for sequential upstream publisher. I have added a unit test proving the problem and fix. It seems the test is also passing for your change here. Based on the discussion in my PR, we have agreed to proceed with your change. Would you mind adding the unit test available in my PR? It is available here.

[mRs-]

I've created #124

[freak4pc]

Closed in favor of the now-merged #124. Thank you!