Generalize DispatchQueues to Dispatcher-protocol objects

[GarthSnyder]

This set of diffs addresses issue #878, "DispatchQueues should be generalized". It's for review; I wouldn't merge this just yet even if you like it.

Summary

This PR adds a Dispatcher protocol and makes DispatchQueues conformant. The good news is that this will work smoothly without adding ambiguity. Backward compatibility should be flawless.

The bad news is that there is no way to do it without adding function overloads. The wrappers are trivial but numerous. They seem somewhat out of place in this otherwise very clean code base. For reasons of API convenience described below, I think they would need to be retained going forward. They're not just transitional.

So, all things considered, I will completely understand if you don't want to take this on.

Why wrappers are required

One of the underlying motivations to move away from DispatchQueues is that they cannot be subclassed. Not just "shouldn't be subclassed" but "mechanically cannot". class MyDispatchQueue: DispatchQueue {} gives a compiler error.

If Dispatcher is a protocol, arguments of that type can accept DispatchQueues directly. However, they cannot be made to accept forms like then(on: .main) because protocols cannot have static members. (Well, they can, but they just impose this requirement on implementors, like any other protocol requirement. You cannot define Dispatcher.main on the abstract protocol type.)

If Dispatcher is a specific type, then it can define .main et al, but it can't accept arguments of type DispatchQueue, so you would still need overloads for that.

Form of wrappers

Here's the general pattern. Canonical function:

func done(on: Dispatcher = conf.D.return, _ body: @escaping(T) throws -> Void) -> Promise<Void> {}

Corresponding wrapper:

func done(on: DispatchQueue? = .pmkDefault, flags: DispatchWorkItemFlags? = nil, _ body: @escaping(T) throws -> Void) -> Promise<Void> {}

This pattern has a couple of nice advantages.

First, dispatch flags are properly a concern and implementation detail of Dispatcher objects. This pattern takes them out of the canonical API entirely while continuing to fully support them for backward compatibility.

Second, the fact that wrappers have a flags: argument and canonical functions do not heads off any potential function overloading ambiguities. Swift strongly prefers to resolve function calls to the signature that requires the fewest defaulted arguments.

In fact, this preference is so strong that a form such as

{ ... }.done(on: DispatchQueue.main) { ... }

calls the canonical function directly, bypassing the wrapper entirely. Normally, a function with a protocol-type argument would be a second choice relative to a type-specific function, but the minimization of defaulted arguments has an even higher priority. (Of course, {...}.done {...} calls the canonical function directly as well.)

Of course, if there is a flags: argument supplied, then the call can only be resolved in one way and there is no ambiguity.

The only code paths that will use the wrappers are 1) calls with flags: arguments, and 2) forms such as done(on: .main) and done(on: .global(qos: .background)). The need to infer the types in #2 locks the call to the wrapper, even though the resulting type would bypass the wrapper were it supplied independently.

What is conf.D?

The Dispatcher analog of conf.Q with type (map: Dispatcher, return: Dispatcher).

conf.Q remains in the API for compatibility. It's now a gateway to conf.D. The only pattern that will mess things up is setting a non-DispatchQueue value for one of the conf.D fields and then attempting to read it back or modify it through the conf.Q interface. You'll just get nil. (But if you're messing around with Dispatcher and conf.D, you've already left the realm of perfect backward compatibility.)

conf.D isn't necessarily the optimal name for this, I'll allow. Pick your poison.

Why do conf.D and canonical functions have non-optional Dispatcher arguments instead of arguments of type Dispatcher?

Because Dispatcher objects can implement dispatching however they like, there's no need for on: nil to be a special case anymore. You can just dispatch to a CurrentThreadDispatcher (supplied) that runs the closure directly.

The on: nil form still works, but it goes through the wrapper path.

What is .pmkDefault in the wrapper signature?

It's an arbitrary sentinel value. Wrappers need to distinguish between explicitly-specified DispatchQueue values, explicitly-specified nils, and the "no value specified" case. You can theoretically supply flags: without on:; this continues to work fine as long as the default Dispatcher is in fact a DispatchQueue.

.pmkDefault never appears in synthesized framework header files or in option-click help. It just shows up as "default".

Why doesn't the Dispatcher protocol just mirror the DispatchQueue API?

For reference, this is the entire protocol:

public protocol Dispatcher {
    func dispatch(_ body: @escaping () -> Void)
}

It could be called async just for familiarity, but in fact dispatching is not required to be asynchronous, cf CurrentThreadDispatcher. Also, existing DispatchQueue async functions would not satisfy this simplified signature, so an adapter function would still be required.

If those functions on DispatchQueue are all called async, it triggers ambiguity warnings and the potential for unintended infinite recursion.

Dispatcher objects are of course free to make use of the entire DispatchQueue API if they wish. The supplied DispatchQueueDispatcher adapter fully supports flags and could be easily extended to support QoS overrides and DispatchGroups.

Note that wrappers generate a DispatchQueueDispatcher only when they have flags to propagate. For plain-vanilla cases, the underlying DispatchQueue is just passed directly as a Dispatcher with no overhead.

Other comments and open issues

Function-level documentation comments should be fully accurate and updated. I did not change any of the separate documentation files but will do so if you are interested in pursuing this change.

Since turning off dispatching (i.e., use of CurrentThreadDispatcher) is primarily a debugging tool, one could argue that when conf.* is set to that mode, it should override any explicitly-specified Dispatcher or DispatchQueue arguments and force synchronous dispatch everywhere. That is not the current behavior, though, so I have followed the existing scheme. I'm not even sure if this would work without causing deadlocks, but if you were interested in exploring it, now might be a good time.

I updated the deprecations but didn't touch any Objective-C material. I also don't have a ready Linux testbed for this and am not sure what to look out for there.

There are unit tests for dispatching.

Dispatcher.dispatch(.promise) {} behaves analogously to the DispatchQueue version.

Arguably, wrappers should be right next to their canonical versions for ease of maintenance. Currently they are all bundled into Dispatcher.swift.

Some parts of the existing code use dispatch queues, dispatch groups, and features such as barriers. That's fine, and to the extent that I understand the details, I see no reason to mess with this. It's internal implementation.

Although I think this API is pretty free from ambiguity and overload conflicts, I have seen a couple of cases where Swift reports the error "DispatchQueue is not convertible to DispatchQueue?" when it can't figure out the type information in a chain. I suppose this is no worse than the average nonsense error that Swift usually generates in such situations.

[mxcl]

Nice patch, and very thoroughly PR’d.

Second, the fact that wrappers have a flags: argument and canonical functions do not heads off any potential function overloading ambiguities. Swift strongly prefers to resolve function calls to the signature that requires the fewest defaulted arguments.

Probably not true, since flags has a default. But the fact you added all the existing function signatures as separate functions will probably make Swift happy for most cases.

I'll merge this and try it on a few projects to see if it just works.

Arguably, wrappers should be right next to their canonical versions for ease of maintenance. Currently they are all bundled into Dispatcher.swift.

I’m cool with it this way. It's already pretty crowded in the other files.

Dispatcher.dispatch(.promise) {}

The .promise namespacer maybe should be removed. We added it for eg. DispatchQueue.async so Swift wouldn't error due to ambiguity for usual usage with the non promise version. Eg:

let p = DispatchQueue.global().async { foo() }

Is ambiguous (sadly) despite the normal version returning Void.

But since Dispatcher is our own class we don't need the namespacer, I understand it is consistent, but since the original purposes is disambiguation I think we should stick with that rationale.

Possibly we should just bite the bullet and make this PMK7, this would allow us to be more ruthless with making this system the default and provide some convenience overloads so dispatch works well (since most people will use that).

Mainly I just worry about corner case issues that may result for our innocent and unsuspecting PMK6 users who didn't want this feature (yet).

[mxcl]

Any clue on the Linux 3.x failures on Travis?

[GarthSnyder]

OK, great, I'll proceed on this. Work list:

• Update documentation
• Investigate Linux build issue
• Update Dispatcher chain starter functions not to use PMKNamespacer

I'm leaving for vacation soon and won't get to this until I return; that will be around July 18.

I'm looking forward to the results of your spot checks.

[GarthSnyder]

I'm ready to start working on this again.

Max, did you have a chance to run this version against any existing codebases?

[mxcl]

I think we should make this a version 7 feature, along with #896, thus there's no risks.

Then it's easy for us all to test an alpha release of that and offer feedback.

[mxcl]

So we should merge into a branch like 7-alpha, and well, the CI issues should be resolved first.

[GarthSnyder]

Sounds good. I'll get back with an updated branch to replace this PR in a week or two.

[mxcl]

Just pushed this to master #909

[GarthSnyder]

Sounds like you're up to date on this, but just to be explicit: the issue with Linux testing related to an oddity of older Linux Swift compilers in Swift 3 compatibility mode.

As outlined above, this whole approach depends on Swift enforcing a fewest-defaulted-arguments rule for disambiguating overloaded functions. This always worked fine for both Swift 3 and Swift 4 in all versions of Xcode. But for some reason, there was a recent span of versions of the Linux Swift codebase that were not applying this rule in Swift 3 mode. That appears to be fixed in more recent builds.

TL;DR: Unless you care strongly about 1) Swift 3 compatibility 2) on Linux 3) consistently across every historical Linux compiler version, the Travis errors are essentially spurious and can be ignored/reconfigured out.

[mxcl]

It’s weird but I can’t change the branch this merges into, can you change it to v7 which I just pushed? Then we’ll merge and review in actual code, we can apply any changes as subsequent PRs.

[GarthSnyder]

Sure, I can work on this tomorrow.

[GarthSnyder]

Sorry it's taken me a while longer to get to this - I spent some time wrestling with Travis CI. I will be more available over the next couple of months.

I have a branch that rebases onto the current v7, but I didn't realize this PR was an active reference into my clone and I moved the head pointer in a way that github didn't like, so it closed the PR. I'm reopening now and I'm going to see if I can get that original "dispatch" branch rehabilitated, but if it doesn't work I'll submit a separate PR for "dispatch-v7".

This is just a baseline. I haven't added docs or addressed any of the issues above yet.

As mentioned above, this code depends on the fact that the compiler prefers overloads that default fewer arguments. Looking more into the multiple test builds, I've discovered that this really has nothing to do with the version of Swift and everything to do with the compiler. Evidently it's not part of the language specification but part of the compiler's implementation latitude.

The result is that this branch is compatible with both Swift 3 and Swift 4, but you must use a relatively current compiler. That seems to be true across all platforms. Older compilers will complain about ambiguous method calls in client code and even in the internal implementation of PromiseKit. I'm not sure how much of a barrier this really presents, but if someone runs into the problem in the wild, it's going to be hard for them to figure it out unless it's a FAQ.

I'm also seeing sporadic hangs in the Travis testing phases, usually in WrapTests or WhenTests. These seem to occur on the current master, too, so my working hypothesis is that it's not related to these updates.

[GarthSnyder]

I think this original PR is now good again, but the dispatch-v7 branch referenced below is cleaner.

[GarthSnyder]

I'm not sure why Travis failed here. When I merge 68763cf into 02e84ce in my clone, it builds fine.

[mxcl]

Shall I close in favor of #975?

[GarthSnyder]

Yes, I guess that's the right thing, although there's a lot of useful context above. There's no way to convert a PR to an Issue, is there?

[mxcl]

No, but the connection is in the other, so we can refer back. I'll close so this one doesn't distract.