mxcl / PromiseKit

Promises for Swift & ObjC.
MIT License
14.22k stars 1.45k forks source link

Pitch: V7 chain-specific default dispatchers #1023

Open GarthSnyder opened 5 years ago

GarthSnyder commented 5 years ago

In RxSwift, dispatching is an attribute of the chain. You switch dispatchers with a separate chain operator, e.g.

observable
    .observeOn(MainScheduler.instance)
    .subscribe { event in
        ...
    }
    ...

I believe ReactiveSwift works similarly, but I'm not as familiar with that library.

I love PromiseKit's simple convention of requiring an explicit dispatcher specification for each step in a chain unless you want to use the global default. It's direct and easily understood. I wouldn't want to change this system as the default.

Nevertheless, there's no reason why PromiseKit couldn't also, optionally, allow you to set a default dispatcher for a chain, and there are a couple of nice benefits that would come from this ability.

I'll mention some of the advantages below, but let me first clarify exactly what I'm talking about. Here's a straw-man proposal:

public func setDefaultMapDispatcher(_ on: Dispatcher? = nil) -> Promise<T>

Benefits

fetchUser(managerID).map { manager in
    (manager.firstName, manager.lastName)
}.done { name in
    button.setTitle("Email \(name.0) \(name.1)", for: .normal)
}

Thoughts?

mxcl commented 5 years ago

People have asked this before (being able to change the default queue for a specific chain) and my argument generally was you return promises all over the place, and in use of such returns you wouldn't know what dispatcher you were getting which could easily lead to bugs.

Like the argument still stands, but perhaps we should just make this a thing the user has to be aware of, that is, you need to reset the dispatcher before you return or document the fact that the dispatcher is not the default?

Certainly we’ve all wanted this feature from time to time. Though PMKv6 distinguishing between in-between closures and done-type closures has helped me a lot there at least since I typically now set conf.Q.map to .global().

Side note: conf.Q is meant to be one-time-set app-wide; it used to be set with a dispatch_once and thus if you created any Promises before setting it, setting it failed. But Swift does not expose dispatch_once so in PMKv6 I had to remove it (hacks with objc bridging wouldn't work with SwiftPM etc.).

GarthSnyder commented 5 years ago

Yes, that seems like a real issue and a potential concern. I agree that the docs should advise strongly against returning promises that have a default dispatcher unless there's a specific reason to do that.

I suspect that a predominant source of problems is likely to be that people set a default dispatcher for notational convenience and then send their promises elsewhere without consciously realizing that they're imposing a dispatcher on the recipient. It's obvious if you even think about it, but it's the kind of cleanup that's easy to overlook.

I was thinking about what the various failure modes might be and trying to pick out the ones that are most worth worrying about. They're not all equally bad.

For example, if you use the default dispatchers and someone puts you on a different thread, and then you try to do main-thread UI stuff, that's usually self-evident pretty quickly because of warning messages or crashes. That seems fine. And similarly, if you're using a nonstandard global default queue, that's probably a pretty good sign that you understand the dispatching system and can be left alone.

The really bad failure mode, I think, is when you believe your own code is all main-threaded, but parts actually end up running on two or more threads. So it almost always works, but every so often there's a completely unexplained crash. 😣

A few possible ways to be safer:

dispatchOn(.global()) {
    firstly {
        doTimeConsumingThing()
    }. then { a in
        doOtherTimeConsumingThing().map { (a, $0) }
    }
}.done { ab in ... }

Disadvantages: 1) You lose the ability to return promises with default dispatchers. 2) Ugly code with another layer of nesting to track. 3) firstly would probably have to run on the stated dispatcher, different from now.