improve threading correctness in URLSession #1186
Conversation
| @@ -555,7 +517,9 @@ extension _ProtocolClient : URLProtocolClient { | |||
| session.delegateQueue.addOperation { | |||
| delegate.urlSession(session, task: task, didCompleteWithError: nil) | |||
| task.state = .completed | |||
| session.taskRegistry.remove(task) | |||
| task.workQueue.async { | |||
There was a problem hiding this comment.
I think this will fix an extremely intermittent crash a few of us have seen.
|
@swift-ci please test |
| @@ -62,7 +59,8 @@ open class URLSessionTask : NSObject, NSCopying { | |||
| } | |||
| internal init(session: URLSession, request: URLRequest, taskIdentifier: Int, body: _Body) { | |||
| self.session = session | |||
| self.workQueue = session.workQueue | |||
| /* make sure we're actually having a serial queue as it's used for synchronization */ | |||
| self.workQueue = DispatchQueue.init(label: "org.swift.URLSessionTask.WorkQueue", target: session.workQueue) | |||
There was a problem hiding this comment.
Makes sense. However, I recall we had replaced the use of DispatchQueues with semaphores in attempts to work around a weird and intermittent hang in DispatchQueue.async - something we could reproduce only in a couple of local environments.
There was a problem hiding this comment.
@pushkarnk well, if that's the case I want to know and I'll fix the issue.
| @@ -171,12 +171,13 @@ | |||
| import CoreFoundation | |||
| import Dispatch | |||
|
|
|||
|
|
|||
| fileprivate let globalVarSyncQ = DispatchQueue(label: "org.swift.Foundation.URLSession.GlobalVarSyncQ") | |||
There was a problem hiding this comment.
Thanks! This was long pending and ignored :(
|
@swift-ci please test |
1 similar comment
|
@swift-ci please test |
|
@weissi thank you for exploring this rabbit hole 😄 Really hope you continue to improve this area of the codebase 👍 |
|
tests all passed and approved, so merging. |
this is really only the beginning to prevent everything from breaking with the simplest test that does use more than one thread. Thread-safely seems to be large absent in this part of foundation 😢.
Unfortunately, I seem to be going down a rabbit hole here but it's a start.