Scheduler improvements #150

joshaber · 2012-11-29T17:16:34Z

A start on solving #94, #136.

Alright so here's an attempt at fleshing out RACScheduler.

It does a couple things:

Adds +currentScheduler.
Only supports using queues that we own.

My current thinking is that all subscriptions would use the +subscriptionScheduler so that we can ensure that by the time we're in a didSubscribe block, we can determine the +currentScheduler.

This would enable us to fix #94 by deferring the re-subscription on the current scheduler.
It supports #136 by making concurrency explicit with the option of creating a serialized scheduler from a concurrent scheduler.

So where are the holes in this? Does this really address the problems? What sorts of crazy edge cases do we need to test?

Coneko · 2012-11-29T19:29:21Z

ReactiveCocoaFramework/ReactiveCocoa/RACScheduler.m

+			@synchronized(scheduler) {
+				NSAssert(scheduler.queue != NULL, @"+concurrentQueueScheduler used without being in a scheduler.");
+
+				NSMutableArray *queue = (__bridge id)dispatch_get_specific(RACSchedulerImmediateSchedulerQueueKey);


dispatch_get_specific / dispatch_queue_set_specific require a GCD dispatch queue, would -[NSThread threadDictionary] work better?

@Coneko GCD queues aren't guaranteed to be pinned to a specific thread. It may change between the execution of blocks, so any thread-local storage is usually a bad idea.

But the weird semantics of this scheduler is to guarantee that it never yields control while executing a scheduled block.

If it's called from a serial queue, if a block is currently executing it's going to be on the same thread. If it's called from a concurrent queue it's unpredictability doesn't change how you would treat a concurrent queue.

@Coneko Good point. @jspahrsummers 👍 ?

I still think what I said in https://github.com/github/ReactiveCocoa/issues/94#issuecomment-10820187 is going to be an issue. Maybe we should continue that discussion at the top level here.

joshaber · 2012-11-29T22:14:41Z

I still think what I said in #94 is going to be an issue. Maybe we should continue that discussion at the top level here.

@jspahrsummers Could you expand on that a bit more?

jspahrsummers · 2012-11-29T22:23:21Z

Well, for instance, I was very close to implementing RACSubscriber with a serial GCD queue instead of @synchronized (and would've, if it weren't for how generally shitty GCD can be), and using dispatch_sync throughout it to serialize received messages.

I imagine that similar patterns are common in RAC consumers, and, with this implementation, any dispatch_sync would:

If using TLS, make the result of +currentScheduler (arguably) incorrect, because it would no longer schedule blocks on the "real" current queue, but rather the previous queue.
If using queue-specific storage, obliterate the result of +currentScheduler.
If using target queues, be even crazier, as can be seen from the discussion in "Safe" synchronous operations can deadlock in the face of target queues jspahrsummers/SafeDispatch#1.

+deferredScheduler suffers from the same problem, since it's basically the current scheduler with some additional behavior.

joshaber · 2012-11-29T22:27:38Z

@jspahrsummers I'm not sure I follow. This proposed implementation doesn't support dispatch_sync nor does it expose the queue directly.

jspahrsummers · 2012-11-29T22:35:07Z

Sorry, I mean a dispatch_sync in application code from a RACScheduler to any GCD queue (created by them or global).

joshaber · 2012-11-29T22:41:31Z

Ah yeah, but I think that's ok-ish. We have to make the strong claim that schedulers are only valid when used with schedulers. They break as soon as you intermingle them with GCD or NSOperationQueue.

Maybe that's too inflexible, but I'd be curious to hear a case where it would be.

jspahrsummers · 2012-11-29T22:46:06Z

Maybe we should back up a little bit.

Why does RACScheduler even need to exist in the first place? Is it just for the concept of an immediateScheduler? What can schedulers do that operation queues can't?

jspahrsummers · 2012-11-30T23:15:59Z

ReactiveCocoaFramework/ReactiveCocoa/RACCompoundDisposable.m

+
+// These properties should only be accessed while synchronized on self.
+@property (nonatomic, readonly, strong) NSMutableArray *disposables;
+@property (nonatomic, assign) BOOL disposed;


This should have a getter = isDisposed.

jspahrsummers · 2012-11-30T23:17:09Z

ReactiveCocoaFramework/ReactiveCocoa/RACDeferredScheduler.h

+
+// A scheduler that executes blocks in the current scheduler, after any blocks
+// already scheduled have completed. If the current scheduler cannot be
+// determined, it uses the main queue scheduler.


*+mainThreadScheduler

jspahrsummers · 2012-11-30T23:19:32Z

ReactiveCocoaFramework/ReactiveCocoa/RACIterativeScheduler.m

+- (void)schedule:(void (^)(void))block {
+	NSParameterAssert(block != NULL);
+
+	NSMutableArray *queue = NSThread.currentThread.threadDictionary[RACIterativeSchedulerQueue];


Why wouldn't we use a serial GCD queue + queue-specific data to represent this?

We can't manually run a GCD queue, so we can't guarantee when the enqueued blocks are executed.

Which part of the interface is that guaranteeing?

👍 I'll clarify the docs that they'll be called immediately after the current block.

jspahrsummers · 2012-11-30T23:28:34Z

ReactiveCocoaFramework/ReactiveCocoa/RACScheduler.m

-	return [RACScheduler schedulerWithScheduleBlock:^(void (^block)(void)) {
-		[queue addOperationWithBlock:block];
-	}];
+ (BOOL)onMainThread {


How about +isOnMainThread?

joshaber · 2012-12-01T06:46:47Z

✒️ 🙏

jspahrsummers · 2012-12-01T20:02:51Z

ReactiveCocoaFramework/ReactiveCocoa/RACIterativeScheduler.m

+
+		[NSThread.currentThread.threadDictionary removeObjectForKey:RACIterativeSchedulerQueue];
+	} else {
+		[queue addObject:block];


This block should be manually copied.

jspahrsummers · 2012-12-01T21:00:34Z

Sorry, one additional note about block management.

joshaber · 2012-12-01T22:40:17Z

✊

Add support for Xcode 8 and Swift 2.3

joshaber added 19 commits November 29, 2012 01:14

first pass at a smarter scheduler

c541ab3

-schedule: now returns a disposable

a8e3a06

docs for asSerialScheduler

d5bc889

just return self if we're not concurrent

6e08b70

just bridge to id

ec011c8

flatten potential recursion in the immediate scheduler

727fe9f

import OSAtomic for iOS

7141960

special case the main queue in the current scheduler

fa7218a

re-arranged the inits

c096324

fix the demo

6b16cba

added +subscriptionScheduler

ea2033b

do the proper bridging retain/release

d225272

added +isOnMainThread

738c8f0

more docs

90d43a9

more tests

60f7c8e

separate the immediateScheduler and currentQueueScheduler

b5d9b3b

nil out the current scheduler key when we're dealloc'd

5e529e4

assert that we have a queue

7364c0d

more docs about caveats

345b5c6

ghost assigned jspahrsummers Nov 29, 2012

Coneko reviewed Nov 29, 2012
View reviewed changes

joshaber added 3 commits November 29, 2012 17:20

use the thread dictionary (ht @Coneko)

f7effac

note that that probably isn't right

16f623c

update the docs

0cbdb71

remove the reference to queues

02d90c3

jspahrsummers reviewed Nov 30, 2012
View reviewed changes

clarify that there may still be a valid current scheduler

7c589ec

jspahrsummers reviewed Nov 30, 2012
View reviewed changes

joshaber added 2 commits November 30, 2012 18:17

explicitly method +mainThreadScheduler

a441a7b

make explicit methods to the class methods

ae5e46b

jspahrsummers reviewed Nov 30, 2012
View reviewed changes

joshaber added 2 commits November 30, 2012 18:21

docs cleanup

6aff978

don't need to check queue for NULLness anymore

187e393

jspahrsummers reviewed Nov 30, 2012
View reviewed changes

joshaber added 3 commits November 30, 2012 18:45

omg all the documentation fixes

cfa2d2a

added a getter for disposed

b17e64c

fewer block lulz

6c5a7d6

jspahrsummers reviewed Dec 1, 2012
View reviewed changes

copy dat block

3cfa653

jspahrsummers closed this Dec 2, 2012

jspahrsummers added a commit that referenced this pull request Dec 2, 2012

Merge remote-tracking branch 'origin/scheduler' (pull request #150)

87eb9ec

Coneko mentioned this pull request Nov 7, 2013

+create: and a compound disposable on <RACSubscriber> #917

Merged

8 tasks

jifang mentioned this pull request Feb 4, 2016

Deadlock of NSLock on same(?) thread #2707

Closed

andersio pushed a commit that referenced this pull request Sep 22, 2016

Merge pull request #150 from sharplet/swift-2.3

6243fc5

Add support for Xcode 8 and Swift 2.3

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Scheduler improvements #150

Scheduler improvements #150

joshaber commented Nov 29, 2012

Coneko Nov 29, 2012

jspahrsummers Nov 29, 2012

Coneko Nov 29, 2012

joshaber Nov 29, 2012

jspahrsummers Nov 29, 2012

joshaber commented Nov 29, 2012

jspahrsummers commented Nov 29, 2012

joshaber commented Nov 29, 2012

jspahrsummers commented Nov 29, 2012

joshaber commented Nov 29, 2012

jspahrsummers commented Nov 29, 2012

jspahrsummers Nov 30, 2012

jspahrsummers Nov 30, 2012

jspahrsummers Nov 30, 2012

joshaber Nov 30, 2012

jspahrsummers Nov 30, 2012

joshaber Nov 30, 2012

jspahrsummers Nov 30, 2012

joshaber commented Dec 1, 2012

jspahrsummers Dec 1, 2012

jspahrsummers commented Dec 1, 2012

joshaber commented Dec 1, 2012

Scheduler improvements #150

Scheduler improvements #150

Conversation

joshaber commented Nov 29, 2012

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joshaber commented Nov 29, 2012

jspahrsummers commented Nov 29, 2012

joshaber commented Nov 29, 2012

jspahrsummers commented Nov 29, 2012

joshaber commented Nov 29, 2012

jspahrsummers commented Nov 29, 2012

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joshaber commented Dec 1, 2012

Choose a reason for hiding this comment

jspahrsummers commented Dec 1, 2012

joshaber commented Dec 1, 2012