Fix/1252 driver scheduling

[TheAdamBorek]

This is the PR which fixes #1252

As we discussed the issue on RxSwift's slack, the reason of the issue is all the Drivers share the same instance of MainScheduler.

The solution is to use different scheduler instances for Drivers.

MainScheduler.instance still returns a shared instance of the scheduler, so it is possible to have similar issue if people will use observeOn(MainScheduler.instance) on Observables. @kzaher I'm thinking if this is ok 🤔

[brentleyjones]

Why can't we instead use ConcurrentMainScheduler.instance for subscribing but MainScheduler.instance for observing? The way this works it seems that each time it asks for a scheduler it makes a new one, so it's effectively ConcurrentMainScheduler.instance everywhere, even when calling asDriver(), which can run into issues since you want observing to queue up like MainScheduler.instance.

Here is my proposed solution, which I'll make into a PR if needed: master...brentleyjones:sharedSequence-subscriptionScheduler

[TheAdamBorek]

@brentleyjones 🤔 actually Driver used to have driverSubscribeOnScheduler at least in Swift 2.3
It seems it was removed. @kzaher can you give as a small reason why? ;)

However, even in Swift 2.3 I used to have issues with Driver that it sent it's events on async queue :/
I would have to debug more to find out what was the root of the problem to create sample & short test case...

[brentleyjones]

Driver will still send events on async queue, if they go through asDriver(). My fix will only fix "pure" Drivers, where you start with .just() (or another constructor) and never change away from Driver. Once you call asDriver() it uses observeOn, and you are back to possibly being async.

[RxPullRequestBot]

	1 Warning
⚠️	No CHANGELOG changes made

Generated by 🚫 Danger

[TheAdamBorek]

Yep, @brentleyjones I've just figured that out also :D

I changed my test to use Driver which is transformed from Observable.create to not have just dependency which has subscribeOn applied.

So... basically your changes won't fix my issue 😉

[kzaher]

Hi @TheAdamBorek @brentleyjones ,

I think there could be some issues if you are using ConcurrentMainScheduler.instance and have a lot of operator combinations (because call stack could be deep and potentially cause stack overflow). For example, I think that you could have a stack overflow in case you are sending a large array on ConcurrentMainScheduler.instance.

Having said that, I think we might investigate adding another customization point in RxSwift 4.0 like we had in previous version subscribeOnScheduler.

I don't think this would cause any performance concerns since MainScheduler only has an integer and a reference to main dispatch queue. Creating new instance once per Driver entry point should be cheap.

@brentleyjones the reason why we can't add this now is because adding this to SharingStrategyProtocol isn't a backwards compatible change. But yes, we can maybe optimize this a bit for 4.0 version.

[kzaher]

@brentleyjones could you for now try this PR and tell us if this solves your issue and maybe make a PR for 4.0 version?

[brentleyjones]

@kzaher I'll check it out after lunch 😄.

[brentleyjones]

@kzaher It works. I do wonder if using MainScheduler() for onObserve will introduce any subtle bugs though?

Though, again, it currently works for me, so I'm 👍 😄.

[kzaher]

I've been thinking about what are the other consequences of changing this.

The nice thing about the previous solution with shared instance was that there wasn't any reentrancy possible in Driver (that "Warning: Recursive call ....") but now that will not be the case.

On the other hand it did cause these async dispatches unpredictably.

I'm still for merging this thing. I'm just trying to consider all of the effects of this change.

One will probably be us needing to add another observeOn() operator to SharedSequence because now a use case for that has appeared.

We will also try to improve that reentrancy code. I'll try to think about how to best adapt it.

[TheAdamBorek]

So @kzaher do you want to me to fix anything in the PR? Are code style, tests written ok?

[kzaher]

Hi @TheAdamBorek ,

I was just thinking a bit. Merged this. Made some changes to unit tests 8e6d467.