microsoft / WinObjC

Objective-C for Windows
MIT License
6.24k stars 808 forks source link

NSURLProtocol and NSURLSession do not guarantee delegate dispatch order and thread #2521

Closed jaredhms closed 7 years ago

jaredhms commented 7 years ago

My last CI build failed in the test execution phase; I've seen this several times over the past few months.

2017-04-14T17:18:29.7225794Z Non-passing Tests: 2017-04-14T17:18:29.7225794Z 2017-04-14T17:18:29.7225794Z ##[error]: NSURLSessionTests::DataTaskWithURL_WithCompletionHandler [Failed] 2017-04-14T17:18:29.7225794Z 2017-04-14T17:18:29.7225794Z Summary: Total=112, Passed=111, Failed=1, Blocked=0, Not Run=0, Skipped=0 2017-04-14T17:18:29.8913424Z ##[error]Process completed with exit code 1.

pradipd commented 7 years ago

Another hit on 4/17 daily build release configuration.

rajsesh commented 7 years ago

rather than disable the tests, we want to understand why the delegate callback ordering is unreliable.

DHowett-MSFT commented 7 years ago

I'm renaming this bug to cover what's going on.

  1. NSURLSession should kick off its delegate callbacks and wait for them to complete
  2. NSURLProtocol_WinHTTP is violating the suggested threading rules set out on the reference platform. It should kick off its client callbacks on the client thread.

To quote the reference documentation,

In addition, an NSURLProtocol subclass is expected to call the various methods of the NSURLProtocolClient protocol from the client thread, including all of the following: -URLProtocol:wasRedirectedToRequest:redirectResponse: -URLProtocol:didReceiveResponse:cacheStoragePolicy: -URLProtocol:didLoadData: -URLProtocolDidFinishLoading: -URLProtocol:didFailWithError: -URLProtocol:didReceiveAuthenticationChallenge: -URLProtocol:didCancelAuthenticationChallenge:

We are not doing so (2), so in combination with (1) we are seeing inconsistent delegate reporting order.

DHowett-MSFT commented 7 years ago

Since @ms-jihua is taking over the NSURLProtocol_WinHTTP implementation in 1705, I'm re-assigning this to her.

DHowett-MSFT commented 7 years ago

The tests have been disabled in the meantime.