Fix of ShareReplay operator for sequential run #120
Conversation
| @@ -41,17 +41,17 @@ public final class ReplaySubject<Output, Failure: Error>: Subject { | |||
| let subscriptions: [Subscription<AnySubscriber<Output, Failure>>] | |||
|
|
|||
| do { | |||
| lock.lock() | |||
| defer { lock.unlock() } | |||
| lock.lock() | |||
There was a problem hiding this comment.
can you undo all of the re-formatting you did here please?
There was a problem hiding this comment.
I can do that. But it seems you are using Indent 4 everywhere and if you look into this file in the main branch, you will see that just this do scope has Indent 2. Let me know if that is what you want and I can change it back ;)
|
Is this related to this in your opinion ? |
Yes, I think the previously merged change (#86) caused one of the problems I am trying to fix here. However it is just a small part of the overall problem that I consider a bug and present the fix for here. I do not know the complete logic behind the #109 but I have tried to apply the presented changes and revert my logic with |
|
I stumbled upon this PR, when I found my unit tests not working because of the |
Hi, thanks for letting me know! We use the operator at some limited places and it seems to be working just fine. It would be cool if you managed to write a test that simulates the problem. Let me know if there is any progress on that ;) |
|
I haven't been able to come up with a test case, which reproduces our runtime issue, yet. It's somehow related to the I noticed, the linked PR https://github.com/CombineCommunity/CombineExt/pull/109/files solves the same issue, this PR solves, without causing our runtime issues. The cause of the problem seems to be, that the |
|
Hey there, thanks for the contribution for CombineExt! From my understanding we should do a joint venture between #109 and #120. The Implementation from #109 looks more simple and from my understanding it would be better to not to clear the demandBuffer. My Idea would be to use the implementation of #109 and the Tests from #120 |
|
I agree with @mRs- that @jabeattie's solution is simpler. If your test suite passes for that PR it might be better to accept a more succinct change here unless there's a reason not to. |
|
@fabianmuecke @mRs- @freak4pc Yes, I agree. I asked @jabeattie to add the unit test in his PR. |
|
I've created #124 |
|
I'm closing this in favor of the merged #124. |
PROBLEM (Open issue: #115)
The
share(replay:)operator freezes when the upstream publisher runs sequentially.Following operations are called in this order:
receive(subscriber:)- replays buffer to the subscriber. At this point the buffer is empty since it has not received any values from the upstream publisher yet.Just("random-value")is used as an upstream publisher, it emits single value and finished event. The subject receives those viasend(_:)andsend(completion:)and forwards them to the subscriber. At this timerequest(demand:)has not yet been called on the subscriber. Demand iszeroand thus the values are not received by the subscriber.forwardCompletionToBuffer(_:)is called on subscriber aftersend(completion:)is received on the subject. It callscancel()which cancels the subscriber. The subscriber does not receive any values and finishes prematurely.FIX
The replaying of the values happen only after the
request(demand:)is called on the subscriber and thus the subscriber is ready to receive values.The
cancel()on the subscriber is called only after therequest(demand:)is called on the subscriber and thus the subscriber actually has a chance to receive values before being cancelled.