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

Concurrent RACSchedulers can result in delivery race conditions #136

Closed
jspahrsummers opened this issue Nov 26, 2012 · 9 comments
Closed
Assignees
Labels

Comments

@jspahrsummers
Copy link
Member

If two nexts, or a next and a completed, etc., get sent to a concurrent GCD or operation queue, they might be processed simultaneously. Most <RACSignal> and <RACStream> methods are not written to accommodate this.

<RACScheduler> should serialize scheduled blocks, even when ultimately delivering to a concurrent queue. The deferredScheduler is also extremely dangerous, because it seems like we can't make the same guarantees there.

@Coneko
Copy link
Member

Coneko commented Nov 26, 2012

deferredScheduler uses dispatch_get_current_queue so it probably shouldn't be used except for logging or testing purposes in the first place.

@joshaber
Copy link
Member

I think the lesson of this and #94 is that RACScheduler needs to become a lot more capable.

@Coneko
Copy link
Member

Coneko commented Nov 26, 2012

This also implies calling send on a RACSubject from different threads isn't safe if some of it's subscriptions are done indirectly through the non-threadsafe <RACSignal> methods.

I've always used RAC very conservative thread-wise, so I haven't looked into it much, but it seems to me the documentation doesn't make it very clear what is and what is not thread-safe.

@jspahrsummers
Copy link
Member Author

@Coneko That's a good point, but we can solve it by serializing RACSubject message delivery in the same way we're discussing for RACScheduler.

I definitely don't think <RACSignal> and <RACStream> methods should need explicit synchronization. They should be able to treat the stream serially.

@jspahrsummers
Copy link
Member Author

From @Coneko in #143:

[Is] #136 also covered by #138? It doesn't matter if a scheduler delivers concurrently if the subscriber which is the receiver of the delivery takes care of serializing the deliveries.

I want to say that this can be closed once #138 is implemented, but I'm worried there's something I'm overlooking. I'll think about it a bit more.

@jspahrsummers
Copy link
Member Author

Based on how I've fixed RACSubscriber's thread safety in #147, this might still be valuable. That implementation merely applies an arbitrary order to incoming concurrent messages – there's no real concept of thread FIFO.

For example:

RACSubject *subject = [RACSubject subject];

[RACScheduler.backgroundScheduler schedule:^{
    [subject sendNext:@0];
}];

[RACScheduler.backgroundScheduler schedule:^{
    [subject sendNext:@1];
}];

[RACScheduler.backgroundScheduler schedule:^{
    [subject sendNext:@2];
}];

Would you expect 0, 1, and 2 to arrive in that order? There's no guarantee that's how it'll happen right now, because each scheduled block is asynchronous on a concurrent queue, and context switching could cause a later send to complete before an earlier one.

Now, whether this is actually an issue in practice, I still don't know.

@Coneko
Copy link
Member

Coneko commented Nov 28, 2012

It makes sense to document this behavior in RACScheduler and let the user decide whether they'd rather have a serializing scheduler to ensure values are received in the same order they're sent in or a parallel scheduler to process values as fast as possible.

@jspahrsummers
Copy link
Member Author

👍 Fair point.

@ghost ghost assigned jspahrsummers Nov 28, 2012
@jspahrsummers
Copy link
Member Author

As part of this, it'd also be nice to add a couple things to RACScheduler to solidify a serial vs. concurrent distinction:

// Whether this scheduler is concurrent.
@property (nonatomic, readonly, getter = isConcurrent) BOOL concurrent;

// Returns a serial RACScheduler that schedules its blocks on the receiver.
- (instancetype)asSerialScheduler;

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants