-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
fixes stack overflow possibility with merge operators #2616
base: main
Are you sure you want to change the base?
Conversation
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
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 usesCurrentThreadScheduler
and theisScheduleRequired
prop to know if it needs to schedule or not. This is similar toProducer
.All tests pass