URLSession's delegateQueue should be serial as per the spec. #983
Conversation
| @@ -226,6 +227,7 @@ open class URLSession : NSObject { | |||
| self.workQueue = DispatchQueue(label: "URLSession<\(identifier)>") | |||
| self.taskAttributesIsolation = DispatchQueue(label: "URLSession<\(identifier)>.taskAttributes", attributes: DispatchQueue.Attributes.concurrent) | |||
| self.delegateQueue = queue ?? OperationQueue() | |||
| self.delegateQueue.maxConcurrentOperationCount = 1 | |||
There was a problem hiding this comment.
This changes the user's OperationQueue to be serial if it isn't already. Does Darwin do that?
There was a problem hiding this comment.
The spec. says "The queue should be a serial queue, in order to ensure the correct ordering of callbacks". But yes, it is not mandatory and user can send in a concurrent queue as well. Tested with a small test-case on Darwin and I see that user's OperationQueue is not converted to serial if it isn't already. Thanks @ianpartridge for pointing this out.
Modified the changes for the same.
mamabusi
force-pushed
the
urlSesSerialDelegQueue
branch
from
b322388 to
620fcbb
Compare
|
@swift-ci test |
|
@mamabusi I'd suggest we have a comment making it clear that we will NOT make a user supplied OperationQueue serial. |
| @@ -225,7 +226,9 @@ open class URLSession : NSObject { | |||
| identifier = nextSessionIdentifier() | |||
| self.workQueue = DispatchQueue(label: "URLSession<\(identifier)>") | |||
| self.taskAttributesIsolation = DispatchQueue(label: "URLSession<\(identifier)>.taskAttributes", attributes: DispatchQueue.Attributes.concurrent) | |||
| self.delegateQueue = queue ?? OperationQueue() | |||
| let operationQueue = OperationQueue() | |||
There was a problem hiding this comment.
We shouldn't create an OperationQueue unless we need to. This creates one in all cases. How about this:
if queue != nil {
self.delegateQueue = queue
} else {
self.delegateQueue = OperationQueue()
self.delegateQueue.maxConcurrentOperationCount = 1
}
mamabusi
force-pushed
the
urlSesSerialDelegQueue
branch
from
620fcbb to
6f7ea2c
Compare
|
@pushkarnk @ianpartridge Review changes and suggestions are addressed. Thanks! |
|
Thanks @mamabusi. @pushkarnk - can you re-run the CI? |
|
Looks good to me. Thanks! |
|
Just for the records, here's what @ianpartridge found: https://www.malhal.com/2017/01/07/nsoperationqueue-maxconcurrentoperationcount-1-serial-queue-problem/ (though it is unlikely that a user will supply a delegate method that will block indefinitely) |
|
@swift-ci test |
|
@swift-ci test and merge |
Refer to the API document that mentions URLSession's delegateQueue should be serial:
https://developer.apple.com/reference/foundation/urlsession/1411597-init