Possible option for chain dispatchers

[GarthSnyder]

Here's a possible approach to #1023 (chain dispatchers) to play around with - see what you think. Should work fine, but still WIP with no docs, not to be merged yet.

This is essentially the "you have to confirm the chain dispatcher when you get near the end" approach, but there are a couple of points that I think will make it nicer than you might initially expect.

First, I think it might help to reframe the existing default system for dispatchers a bit. This will clean up some existing ambiguities and also pave the way for a simpler and more understandable version of chain dispatchers. The main points are:

• Global defaults must be set before use: You've mentioned that conf.Q was never really intended to be a dynamic variable that you switched around on the fly. Since chain-specific dispatchers are a better way of achieving this same effect, we may as well reset the default system back to the original intent and require that you set the defaults before creating any promises. In this PR, setting the defaults after creating promises just produces a warning, but in a future version of PromiseKit it could be a hard error. The point is to clearly distinguish between global and local effects and to remove any ambiguity about when dispatching choices are actually made

• Phase out the terms map and return: Currently, every function fits in one of these categories and default dispatching varies depending on the function. But the terms are a bit vague, both because map and return aren't very descriptive and because you pretty much just have to know which functions are which. Most map functions aren't really map operations, and most return functions don't actually return anything.

• Dispatching becomes an attribute of the chain, not the function: Instead of having map and return, we can reframe the system slightly to direct the focus away from functions and onto promise chains. Instead of saying that every function has an inherent dispatcher category, we can say that every promise chain has a "body" and a "tail" and that those regions have different default dispatchers. Once you enter the tail by calling catch, done, or finally (and only one of those three), you are in the tail for good. Your default dispatching will from that point on always be the tail dispatcher (formerly return).

• This model tells a clearer story about how the average use case does background work in some vaguely-defined ether and then surfaces the results to the UI for action. There's only one surfacing point; the chain doesn't porpoise back and forth between dispatchers unless you specifically ask for this. This is both a conceptual model change and a small change in actual behavior.

• get and ensure are no longer special: As a corollary to this definition of a chain's tail, get and ensure (along with ensureThen), which were formerly return functions, will now just be general functions. Like most functions, if you use them within the tail of a chain, they'll be dispatched on the tail dispatcher. If you use them in the body, they'll be dispatched as body functions.

• conf.Q deprecated and replaced with a function: conf.Q (but not conf.D) will remain around for backward compatibility, but the standard way to set default dispatchers will be a function call as shown below. This change allows for wrappers that accommodate DispatchQueue dot notation.

public struct PMKConfiguration {
    ...
    mutating public func setDefaultDispatchers(body: Dispatcher = NoValue(), tail: Dispatcher = NoValue())
    mutating public func setDefaultDispatchers(body: DispatchQueue? = .unspecified, tail: DispatchQueue? = .unspecified)
    ...
}

// Example
conf.setDefaultDispatchers(tail: .global())

OK, that's the background context. On top of this, we can add chain-specific dispatchers (which I'll just call "chain dispatchers" here, although there may be a better term.)

• Set a chain dispatcher with dispatch(on:): To set the chain default (from this point forward), just call dispatch(on:) within the chain.

firstly {
    fetchUser(name)
}.dispatch(on: .main).then { userRecord -> Promise<Credentials> in
    userPicture.image = UIImage(named: userRecord.imageName)
    return fetchServerCredentials(userRecord)
}.done { creds in  // Dispatched on .main
   ...

• Permanent until you change it: A dispatcher set with dispatch(on:) remains in effect until it's canceled with dispatch(on: .default), you set a different default dispatcher, or the chain terminates. (Of course, any individual function can have its own on: argument to override the chain default for that call only. But that does not cancel an underlying chain dispatcher.) There's only a single chain dispatcher; it doesn't vary depending on whether you're in the body or the tail.

• on: .default is general: All individual functions now also accept on: .default, which selects one of the two global default dispatchers depending on where you happen to be in the chain.

• You have to "help" the chain dispatcher to cross the body/tail boundary: When a chain dispatcher has been set, the first use of the chain dispatcher within the tail must be explicit. The standard way to do this is to just specify on: .chain as the argument to the first tail function. Alternatively, you can call dispatch(on: .chain), set a new chain dispatcher, or terminate the chain dispatcher with dispatch(on: .default). These are all fine; it doesn't matter as long as you don't use the chain dispatcher implicitly (typically by failing to specify any on: clause at all).

If you fail to confirm a chain dispatcher, PromiseKit prints a warning. However, the chain dispatcher remains in effect and the actual behavior is unaffected. You can silence the warning by adding an explicit on: .chain or by setting conf.requireChainDispatcherConfirmation to false to waive this requirement globally.

In the example chain below, everything from map onward is dispatched on the same CoreDataDispatcher. The chain dispatcher is properly confirmed by catch(on: .chain), so no warning is printed.

episode.updateMediaSize().then { didSetSize in
    episode.updateMediaAttributesIfNeeded().map { didSetSize }
}.dispatch(on: episode.dispatcher).map { didSetSize -> Void in
    if didSetSize, episode.mediaType == .unknown {
        episode.mediaType = .somethingElse
    }
}.ensure {
    episode.markGroomed()
}.catch(on: .chain) {
    // Other errors should be logged at lower levels already
    if case MediaSizeError.tooManyRequests(let delay) = error {
        print("Media size read failed for episode \(episode.title)")
        self.reenqueue(sourceEpisode, delay: delay)
    }
}.finally {
    if episode.mangedObjectContext?.hasChanges {
        do {
            episode.mangedObjectContext?.save()
        } catch {
            // Error handling
        }
    }
}

Pitch points:

• Single point of control: Because there's only one interface between the body and the tail of any given chain, there is only one, well-defined point that potentially prints an error or requires confirmation. And that point is easy to find: it's the first instance of done, catch, or finally.

• Fits well with functions that return promises: In most cases, promise chains returned by lower-level code will not have entered the tail segment, and the client will add a done or catch block to create a tail. If the lower-level code has set a chain dispatcher and the client does not confirm it, the client will be warned. At the same time, the lower-level code does have the option to set a chain dispatcher.

• Low confirmation burden: Adding on: .chain to an existing function achieves confirmation en passant without a lot of ceremony. You're guaranteed to have the on: argument available for this purpose; if you want to use a different dispatcher, go right ahead and do a normal on:. You're not required to confirm until (and if) you go back to the chain dispatcher. The only rule is that the first use of the chain dispatcher within the tail must be explicit.

• Easy to explain: The system pretty much boils down to "when you set a chain dispatcher, that's the dispatcher you get until you say otherwise." There are no subtleties regarding map vs. return, or multiple dispatchers, or segments of a chain, or operations that terminate your chain dispatcher as an unexpected side effect. But at the same time, if you don't seem to be aware of the basic distinction between the body and tail of a chain, or you don't seem to realize that someone has handed you a chain with an embedded dispatcher, PromiseKit will draw your attention to this with a warning.

When I converted some existing code to this scheme, I was a bit surprised to find that most simplification actually came from the body/tail idea rather than from chain dispatchers themselves. ensure and get are useful anywhere in a chain, so it's nice that they now self-adapt and do the right thing depending on context.

As far as implementation, it's a pretty clean change. Every thenable has a DispatchState struct that tracks progress along the chain and manages the chain dispatcher, if there is one. All the logic for managing dispatching goes in DispatchState; the promise functions don't have to do anything but propagate the state to their children. Cancellable promises don't really have to do much at all since they inherit most of this behavior from their wrapped promises.

There's another interesting wrinkle in this PR, but I'll leave it for later... Everything above should work as described.

Thoughts?

[mxcl]

I like this.

Perhaps a way to (further†) ensure the chaining only happens where the user expects is like so:

defer { episode.dispatcher.retire() }

episode.updateMediaSize().then { didSetSize in
    episode.updateMediaAttributesIfNeeded().map { didSetSize }
}.dispatch(on: episode.dispatcher).map { didSetSize -> Void in
    if didSetSize, episode.mediaType == .unknown {
        episode.mediaType = .somethingElse
    }
}.ensure {
    episode.markGroomed()
}.catch(on: .chain) {
    // Other errors should be logged at lower levels already
    if case MediaSizeError.tooManyRequests(let delay) = error {
        print("Media size read failed for episode \(episode.title)")
        self.reenqueue(sourceEpisode, delay: delay)
    }
}.finally {
    if episode.mangedObjectContext?.hasChanges {
        do {
            episode.mangedObjectContext?.save()
        } catch {
            // Error handling
        }
    }
}

Since the user has to opt into this pattern, I think it is a nice addition. Adding a way to retire the chain would be the recommended way when returning chains but optional.

---

Additionally I think we can retire the additions where we allow passing a queue. Instead add a few convenience methods on Dispatcher like Dispatcher.qos.userInitiated and Dispatcher.qos.background that dispatch to GCD. Thoughts?

† having catch etc default to main is clever.

[GarthSnyder]

@mxcl: I think we can retire the additions where we allow passing a queue. Instead add a few convenience methods on Dispatcher like Dispatcher.qos.userInitiated and Dispatcher.qos.background that dispatch to GCD.

DispatchQueues are already Dispatchers through an extension, so if SE-0299 works out, there won't need to be any mention of DispatchQueues in the API at all.

At the same time, it's probably a good idea to mirror the DispatchQueue conventions pretty closely (in Dispatcher) so that anyone who wants to continue to think of Dispatchers as DispatchQueues won't experience friction. That would make the notation for queues other than .main something like .global(qos: .userInitiated). I wouldn't object to also having .qos.UserInitiated as well, though.

Speaking of making things similar among APIs, it's probably worthwhile to cross-reference with Combine's dispatching system now that it's been around for a while.

defer { episode.dispatcher.retire() }

Let me paraphrase this back to you to check that I'm correctly interpreting the concept: There should be a standard and facilitated way (ideally, a declarative way) to ensure that promise-returning functions don't inadvertently return promise chains that have an active nondefault dispatcher. If the "this function should always return a defaultly-dispatched promise" declaration can be made up front, that's clearer and less error-prone than requiring you to remember to add .dispatch(on: .default) at the end of the chain. Is that the gist? I'm guessing that the intent of retire() specifically is to lock the dispatcher so that further dispatch attempts fail or emit a warning.

I like the general idea, but I suspect that it's probably not a good idea to try to involve dispatchers themselves in this mechanism. Dispatchers are dumb. They can be shared global instances (e.g., DispatchQueue.main), and they can also be value types. Both of those cases make it hard to give them responsibility for context.

When Swift 5.5 comes out, I'll do another round of revisions and see if I can come up with a better way to achieve this goal.