{"action":"created","issue":{"url":"https://api.github.com/repos/thoughtbot/Superb/issues/39","repository_url":"https://api.github.com/repos/thoughtbot/Superb","labels_url":"https://api.github.com/repos/thoughtbot/Superb/issues/39/labels{/name}","comments_url":"https://api.github.com/repos/thoughtbot/Superb/issues/39/comments","events_url":"https://api.github.com/repos/thoughtbot/Superb/issues/39/events","html_url":"https://github.com/thoughtbot/Superb/pull/39","id":252030172,"number":39,"title":"Overhaul and simplify RequestAuthorizer's concurrency model","user":{"login":"sharplet","id":147686,"avatar_url":"https://avatars3.githubusercontent.com/u/147686?v=4","gravatar_id":"","url":"https://api.github.com/users/sharplet","html_url":"https://github.com/sharplet","followers_url":"https://api.github.com/users/sharplet/followers","following_url":"https://api.github.com/users/sharplet/following{/other_user}","gists_url":"https://api.github.com/users/sharplet/gists{/gist_id}","starred_url":"https://api.github.com/users/sharplet/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/sharplet/subscriptions","organizations_url":"https://api.github.com/users/sharplet/orgs","repos_url":"https://api.github.com/users/sharplet/repos","events_url":"https://api.github.com/users/sharplet/events{/privacy}","received_events_url":"https://api.github.com/users/sharplet/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":2,"created_at":"2017-08-22T17:20:41Z","updated_at":"2017-08-24T15:02:07Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/thoughtbot/Superb/pulls/39","html_url":"https://github.com/thoughtbot/Superb/pull/39","diff_url":"https://github.com/thoughtbot/Superb/pull/39.diff","patch_url":"https://github.com/thoughtbot/Superb/pull/39.patch"},"body":"This test runs somewhat slowly on CI, so the timeout is increased to 3 seconds to reduce intermittent test failures."},"comment":{"url":"https://api.github.com/repos/thoughtbot/Superb/issues/comments/324661916","html_url":"https://github.com/thoughtbot/Superb/pull/39#issuecomment-324661916","issue_url":"https://api.github.com/repos/thoughtbot/Superb/issues/39","id":324661916,"user":{"login":"sharplet","id":147686,"avatar_url":"https://avatars3.githubusercontent.com/u/147686?v=4","gravatar_id":"","url":"https://api.github.com/users/sharplet","html_url":"https://github.com/sharplet","followers_url":"https://api.github.com/users/sharplet/followers","following_url":"https://api.github.com/users/sharplet/following{/other_user}","gists_url":"https://api.github.com/users/sharplet/gists{/gist_id}","starred_url":"https://api.github.com/users/sharplet/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/sharplet/subscriptions","organizations_url":"https://api.github.com/users/sharplet/orgs","repos_url":"https://api.github.com/users/sharplet/repos","events_url":"https://api.github.com/users/sharplet/events{/privacy}","received_events_url":"https://api.github.com/users/sharplet/received_events","type":"User","site_admin":false},"created_at":"2017-08-24T15:02:07Z","updated_at":"2017-08-24T15:02:07Z","body":"This is ready for review again.\r\n\r\n**What was the problem?**\r\n\r\nWhile investigating the race condition in the tests, I was able to determine that `RequestAuthorizer` is still thread-safe, but it was actually the test itself that had the race condition. Here is where the problem occurred:\r\n\r\n```swift\r\n      let examples = (1...5000).map { \"/example\\($0)\" }\r\n      examples.forEach { _ in group.enter() } // (1)\r\n\r\n      DispatchQueue.concurrentPerform(iterations: examples.count) { i in\r\n        let example = examples[i]\r\n        let request = testRequest(path: example)\r\n\r\n        authorizer.performAuthorized(request) { result in\r\n          DispatchQueue.main.async {\r\n            completedRequests.insert(example)\r\n            if let error = result.error {\r\n              errors.append(error)\r\n            }\r\n          }\r\n        }\r\n\r\n        // (2)\r\n        group.leave()\r\n      }\r\n\r\n      group.wait()\r\n\r\n      // (3)\r\n      testProvider.complete(with: .cancelled)\r\n```\r\n\r\nAt point (1) we set up a `DispatchGroup`, whose purpose is to allow us to wait for all the pending requests to be enqueued before completing authentication at (3). Implicitly assumed here is that whenever we hit (2), the request that was performed is completely enqueued and waiting for authentication to complete. **It turns out that was false!** Because the internal call to `performAuthorized(_:reauthenticate:completionHandler:)` was dispatched onto a global concurrent queue, it's possible for the dispatch group to complete before all of those globally scheduled blocks have completed.\r\n\r\nThe simple workaround (in ec55052) was to remove the unnecessary queue shift and enqueue the request as quickly as possible. Too much concurrency is not a good thing.\r\n\r\n**What else changed?**\r\n\r\nAll this investigation made me realise that a couple of the internal abstractions used by `RequestAuthorizer` were unnecessary, and probably making things much _slower_ than they needed to be. To that end, the explicit `Actor` and `Channel` classes are no more, and `RequestAuthorizer` now explicitly manages an internal queue of pending requests as a plain old array.\r\n\r\nHow is this better? Well, `RequestAuthorizer` is still actually modelled as an actor: it has private state that it only manages on a private, serial `messageQueue`. We no longer use `DispatchQueue.sync`, favouring `DispatchQueue.async`, and explicitly dispatch completion handlers onto a configuration `completionQueue`.\r\n\r\nI believe that removing these blocking and synchronisation based primitives should _increase_ performance by allowing GCD to minimise context switches and spawn fewer threads to meet the same workload."}}
{"actor":{"display_login":"sharplet"}}