fixes stack overflow possibility with merge operators #2616
Conversation
geoffmacd
changed the title
|
This looks like a good candidate for a dot release @freak4pc. What do you think? |
RxSwift/Observables/Merge.swift
Outdated
| @@ -170,7 +170,10 @@ private final class MergeLimitedSinkIter<SourceElement, SourceSequence: Observab | |||
| case .completed: | |||
| self.parent.group.remove(for: self.disposeKey) | |||
| if let next = self.parent.queue.dequeue() { | |||
| self.parent.subscribe(next, group: self.parent.group) | |||
| _ = CurrentThreadScheduler.instance.schedule(()) { _ in | |||
| self.parent.subscribe(next, group: self.parent.group) | |||
There was a problem hiding this comment.
I'm not sure how important this is, but scheduling the subscribe call will take the corresponding modification of self.parent.group out of the lock that is held while synchronized_on is executing.
There was a problem hiding this comment.
yea, I think you are right that there is an issue and I need to ensure the NSRecursiveLock is re-locking inside this block. I think for the current locking cycle from synchronized_on, there is no more mutation after this line to worry about. The lock will recursively lock on the next inner subscribe.
There is a separate issue here which is that the the disposable from _ = CurrentThreadScheduler.instance.schedule(()) needs to be added to the group otherwise this will not dispose if the CompositeDisposable is disposed. This is minor though since the parent.subscribe would have no impact.
|
Hey @geoffmacd, sorry for the long delay on this.
Thanks! |
fix subscribing immediately in merge operators can produce values immediately which can re-enter and cause stack overflows
freak4pc
force-pushed
the
fix-merge-stack-overflow
branch
from
33dfd45
to
168f887
Compare
Fixes #2615 See issue for details
when completing Merge operator iterations (concat(), concatMap() and
merge(maxConcurrent:), fixes subscribing immediately to the next in the queue, which can produce values immediately which can re-enter and cause stack overflows. ultimately usesCurrentThreadSchedulerand theisScheduleRequiredprop to know if it needs to schedule or not. This is similar toProducer.All tests pass