Skip to content
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

Don’t attempt to remove elements of empty queues. #2009

Merged
merged 1 commit into from
May 16, 2015

Conversation

robrix
Copy link
Contributor

@robrix robrix commented May 15, 2015

Fixes a crasher.

@jspahrsummers
Copy link
Member

This seems like a good idea no matter what, but I'm struggling to understand the chain of events that could lead us here.

It seems like we only try to dequeue a signal producer if one has just completed or interrupted, so shouldn't there be a one-to-one correspondence there? 😕

@robrix
Copy link
Contributor Author

robrix commented May 15, 2015

Yeah, it’s unclear how this is happening—perhaps threading hijinks in my exploration of the deadlocks?

I was misinterpreting this use of the method as intending it to be empty sometimes, rather than intending it to be emptied by the call—mea culpa.

@ikesyo
Copy link
Member

ikesyo commented May 15, 2015

From what I experienced, the following code prevents the crash (but I didn't understand why):

    func startNextSignalProducer(signalProducer: SignalProducer<T, E>) {
        signalProducer.startWithSignal { signal, disposable in
            self.disposable.addDisposable(disposable)

            let d = signal.observe(Signal.Observer { event in
                switch event {
                case .Completed, .Interrupted:
                    if let nextSignalProducer = self.dequeueSignalProducer() {
                        self.startNextSignalProducer(nextSignalProducer)
                    }

                default:
                    self.observer.put(event)
                }
            })
            self.disposable.addDisposable(d)
        }
    }

@jspahrsummers
Copy link
Member

Well, this certainly can't hurt, so I'll merge it.

jspahrsummers added a commit that referenced this pull request May 16, 2015
Don’t attempt to remove elements of empty queues.
@jspahrsummers jspahrsummers merged commit 1c5c38f into swift-development May 16, 2015
@jspahrsummers jspahrsummers deleted the safe-dequeueing branch May 16, 2015 04:32
@@ -1159,7 +1159,7 @@ private final class ConcatState<T, E: ErrorType> {
// Active producers remain in the queue until completed. Since
// dequeueing happens at completion of the active producer, the
// first producer in the queue can be removed.
queue.removeAtIndex(0)
if !queue.isEmpty { queue.removeAtIndex(0) }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just saw a bad access on this line (post-PR). Not sure how that's possible. 😕

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that means either concurrency bug or Swift bug.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not both???

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

He said or not xor 😃

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With Swift, both is practically guaranteed.

ikesyo added a commit that referenced this pull request May 22, 2015
Outer signal observer should be notified of Completed event after all inner producers have completed.

It should fix #2009 (comment)
@jspahrsummers jspahrsummers added this to the 3.0 milestone May 22, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants