{"action":"created","issue":{"url":"https://api.github.com/repos/mxcl/PromiseKit/issues/888","repository_url":"https://api.github.com/repos/mxcl/PromiseKit","labels_url":"https://api.github.com/repos/mxcl/PromiseKit/issues/888/labels{/name}","comments_url":"https://api.github.com/repos/mxcl/PromiseKit/issues/888/comments","events_url":"https://api.github.com/repos/mxcl/PromiseKit/issues/888/events","html_url":"https://github.com/mxcl/PromiseKit/pull/888","id":334678814,"node_id":"MDExOlB1bGxSZXF1ZXN0MTk2NTkyMjU3","number":888,"title":"Generalize DispatchQueues to Dispatcher-protocol objects","user":{"login":"GarthSnyder","id":443181,"node_id":"MDQ6VXNlcjQ0MzE4MQ==","avatar_url":"https://avatars3.githubusercontent.com/u/443181?v=4","gravatar_id":"","url":"https://api.github.com/users/GarthSnyder","html_url":"https://github.com/GarthSnyder","followers_url":"https://api.github.com/users/GarthSnyder/followers","following_url":"https://api.github.com/users/GarthSnyder/following{/other_user}","gists_url":"https://api.github.com/users/GarthSnyder/gists{/gist_id}","starred_url":"https://api.github.com/users/GarthSnyder/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/GarthSnyder/subscriptions","organizations_url":"https://api.github.com/users/GarthSnyder/orgs","repos_url":"https://api.github.com/users/GarthSnyder/repos","events_url":"https://api.github.com/users/GarthSnyder/events{/privacy}","received_events_url":"https://api.github.com/users/GarthSnyder/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":0,"created_at":"2018-06-21T22:24:42Z","updated_at":"2018-06-27T14:50:21Z","closed_at":null,"author_association":"CONTRIBUTOR","pull_request":{"url":"https://api.github.com/repos/mxcl/PromiseKit/pulls/888","html_url":"https://github.com/mxcl/PromiseKit/pull/888","diff_url":"https://github.com/mxcl/PromiseKit/pull/888.diff","patch_url":"https://github.com/mxcl/PromiseKit/pull/888.patch"},"body":"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.\r\n\r\n**Summary**\r\n\r\nThis PR adds a `Dispatcher` protocol and makes `DispatchQueue`s conformant. The good news is that this will work smoothly without adding ambiguity. Backward compatibility should be flawless.\r\n\r\nThe 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.\r\n\r\nSo, all things considered, I will completely understand if you don't want to take this on.\r\n\r\n**Why wrappers are required**\r\n\r\nOne of the underlying motivations to move away from `DispatchQueue`s is that they cannot be subclassed. Not just \"shouldn't be subclassed\" but \"mechanically cannot\". `class MyDispatchQueue: DispatchQueue {}` gives a compiler error.\r\n\r\nIf `Dispatcher` is a protocol, arguments of that type can accept `DispatchQueue`s 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.) \r\n\r\nIf `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.\r\n\r\n**Form of wrappers**\r\n\r\nHere's the general pattern. Canonical function:\r\n\r\n```\r\nfunc done(on: Dispatcher = conf.D.return, _ body: @escaping(T) throws -> Void) -> Promise<Void> {}\r\n```\r\n\r\nCorresponding wrapper:\r\n\r\n```\r\nfunc done(on: DispatchQueue? = .pmkDefault, flags: DispatchWorkItemFlags? = nil, _ body: @escaping(T) throws -> Void) -> Promise<Void> {}\r\n```\r\n\r\nThis pattern has a couple of nice advantages.\r\n\r\nFirst, 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.\r\n\r\nSecond, 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.\r\n\r\nIn fact, this preference is so strong that a form such as\r\n\r\n```\r\n{ ... }.done(on: DispatchQueue.main) { ... }\r\n```\r\ncalls 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.)\r\n\r\nOf course, if there *is* a `flags:` argument supplied, then the call can only be resolved in one way and there is no ambiguity.\r\n\r\nThe 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.\r\n\r\n**What is conf.D?**\r\n\r\nThe `Dispatcher` analog of `conf.Q` with type `(map: Dispatcher, return: Dispatcher)`. \r\n\r\n`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.)\r\n\r\n`conf.D` isn't necessarily the optimal name for this, I'll allow. Pick your poison.\r\n\r\n**Why do `conf.D` and canonical functions have non-optional `Dispatcher` arguments instead of arguments of type `Dispatcher?`**\r\n\r\nBecause `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.\r\n\r\nThe `on: nil` form still works, but it goes through the wrapper path.\r\n\r\n**What is `.pmkDefault` in the wrapper signature?**\r\n\r\nIt's an arbitrary sentinel value. Wrappers need to distinguish between explicitly-specified `DispatchQueue` values, explicitly-specified `nil`s, 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`.\r\n\r\n`.pmkDefault` never appears in synthesized framework header files or in option-click help. It just shows up as \"default\".\r\n\r\n**Why doesn't the `Dispatcher` protocol just mirror the `DispatchQueue` API?**\r\n\r\nFor reference, this is the entire protocol:\r\n\r\n```\r\npublic protocol Dispatcher {\r\n    func dispatch(_ body: @escaping () -> Void)\r\n}\r\n```\r\n\r\nIt 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. \r\n\r\nIf those functions on `DispatchQueue` are all called `async`, it triggers ambiguity warnings and the potential for unintended infinite recursion.\r\n\r\n`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`.\r\n\r\nNote 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.\r\n\r\n**Other comments and open issues**\r\n\r\nFunction-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.\r\n\r\nSince 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.\r\n\r\nI 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.\r\n\r\nThere are unit tests for dispatching.\r\n\r\n`Dispatcher.dispatch(.promise) {}` behaves analogously to the `DispatchQueue` version.\r\n\r\nArguably, wrappers should be right next to their canonical versions for ease of maintenance. Currently they are all bundled into Dispatcher.swift.\r\n\r\nSome 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.\r\n\r\nAlthough 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."},"comment":{"url":"https://api.github.com/repos/mxcl/PromiseKit/issues/comments/400700965","html_url":"https://github.com/mxcl/PromiseKit/pull/888#issuecomment-400700965","issue_url":"https://api.github.com/repos/mxcl/PromiseKit/issues/888","id":400700965,"node_id":"MDEyOklzc3VlQ29tbWVudDQwMDcwMDk2NQ==","user":{"login":"mxcl","id":58962,"node_id":"MDQ6VXNlcjU4OTYy","avatar_url":"https://avatars2.githubusercontent.com/u/58962?v=4","gravatar_id":"","url":"https://api.github.com/users/mxcl","html_url":"https://github.com/mxcl","followers_url":"https://api.github.com/users/mxcl/followers","following_url":"https://api.github.com/users/mxcl/following{/other_user}","gists_url":"https://api.github.com/users/mxcl/gists{/gist_id}","starred_url":"https://api.github.com/users/mxcl/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/mxcl/subscriptions","organizations_url":"https://api.github.com/users/mxcl/orgs","repos_url":"https://api.github.com/users/mxcl/repos","events_url":"https://api.github.com/users/mxcl/events{/privacy}","received_events_url":"https://api.github.com/users/mxcl/received_events","type":"User","site_admin":false},"created_at":"2018-06-27T14:50:21Z","updated_at":"2018-06-27T14:50:21Z","author_association":"OWNER","body":"Nice patch, and very thoroughly PR’d.\r\n\r\n> 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.\r\n\r\nProbably 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.\r\n\r\nI'll merge this and try it on a few projects to see if it *just works*.\r\n\r\n> Arguably, wrappers should be right next to their canonical versions for ease of maintenance. Currently they are all bundled into Dispatcher.swift.\r\n\r\nI’m cool with it this way. It's already pretty crowded in the other files.\r\n\r\n> Dispatcher.dispatch(.promise) {}\r\n\r\nThe `.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:\r\n\r\n    let p = DispatchQueue.global().async { foo() }\r\n\r\nIs ambiguous (sadly) despite the normal version returning `Void`.\r\n\r\nBut 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.\r\n\r\nPossibly 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).\r\n\r\nMainly I just worry about corner case issues that may result for our innocent and unsuspecting PMK6 users who didn't want this feature (yet)."}}
{"actor":{"display_login":"mxcl"}}