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

Pitch: V7 chain-specific default dispatchers #1023

Open
GarthSnyder opened this issue Mar 18, 2019 · 2 comments
Open

Pitch: V7 chain-specific default dispatchers #1023

GarthSnyder opened this issue Mar 18, 2019 · 2 comments

Comments

@GarthSnyder
Copy link
Collaborator

GarthSnyder commented Mar 18, 2019

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:

  • Promises gain setDefaultMapDispatcher and setDefaultReturnDispatcher methods (terrible placeholder names...) with a signature something like
public func setDefaultMapDispatcher(_ on: Dispatcher? = nil) -> Promise<T>
  • As usual, the returned promise is different from the original; it's a wrapper. Only subsequent chain elements added via the wrapper see the new default.

  • Any subsequent method uses the specified default dispatcher if nothing more specific is requested. conf.Q still exists but is consulted only if no chain-specific default has been set.

  • Methods that return new promises silently propagate the default dispatcher to them.

Benefits

  • Clearer and more self-documenting than conf.Q. I would hazard a guess that some users initially assume that conf.Q is consulted at the point of actual dispatch. But in fact, it's consulted at chain construction time through the default argument mechanism. This is useful because it allows different conf.Q values to be set for different regions of code, but it's not very discoverable without experimentation.

  • Chains don't affect each other. By contrast, there's only one conf.Q, and code has to take explicit precautions (save/restore) to avoid fighting over it.

  • Code is cleaner without repeating on: for every clause.

  • Purely additive: nothing changes unless you use the feature.

  • Facilitates debugging because you can easily set a temporary default for any portion of a chain. It's one line added or removed which affects no other code. With conf.Q, you have to add a variable assignment at the start of the chain, stop in the middle of the chain to set conf.Q, then resume the chain from the variable.

  • The big one: it allows functions that yield or propagate promises to specify (suggest, really) dispatching policy. Result types that aren't thread-safe can return themselves with dispatching set to a serial queue. For example, a routine that returns a Promise<NSManagedObject> might set the chain's default dispatcher to a CoreDataDispatcher for the appropriate NSManagedObjectContext so that the managed object can be freely manipulated with no additional code on the receiver's end. E.g.,

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

Thoughts?

@mxcl
Copy link
Owner

mxcl commented Mar 19, 2019

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
Copy link
Collaborator Author

GarthSnyder commented Mar 20, 2019

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:

  • Require chain-specific dispatchers to be unset by the end of the chain. I guess the specifics would be something like: if 1) you reach the transition from map-type elements to return-type elements, and 2) there's a chain-specific default for map, and 3) there is no chain-specific default for return, then print a warning message. Optionally, add the additional requirement that 4) both conf.Q values are DispatchQueue.main (to cut down on warnings). Disadvantage: requires an additional, useless dispatchOn(nil) just to suppress the warning.

  • Propagate defaults up the chain instead of down. When you set a chain-specific default, it does not affect future blocks. Instead, it traverses up the chain, setting the dispatcher for each block whose dispatcher was previously determined by the global default. When you reach a block that already has an explicit dispatcher (including one set by a chain-specific default), stop. Disadvantages: 1) It's really weird. 2) Now you have the reverse problem of potentially inadvertently setting a dispatcher on blocks that were created by other code. (But they must have just been using the defaults, so maybe it's ok...) 3) You lose the ability to return promises with default dispatchers. 4) Touches a lot of code - currently all the dispatchers are tucked away in closures. You'd have to pull them into a Promise instance variable and reference them from there. Also, I don't think there's existing infrastructure to go back in the chain.

  • Explicit scoping with brackets. This is essentially a formalization of the way conf.Q actually works now, so it's easy to implement. For example:

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.

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

No branches or pull requests

2 participants