URLSessionTask retain cycle and memory leak fix - all Linuxies

[johnno1962]

HI Apple,

I’ve noticed a memory leak using NSURLSessionTasks to fetch a URL in Swift 4 that wasn’t present in Swift 3.1.1. This is for Vanilla Linux as well as Android. This PR seems to resolve it.

There was a retain cycle between the URLSessionTask and its protocol handler resulting in it never being deinit’d and the internal data container never being freed up. This PR nils out the pointer to the protocol handler when the task completes, breaking the retain cycle and allowing both to deinit.

Fixes SR-5472

[johnno1962]

Sorry, I couldn’t resist fixing the many unnesssesary files created in /tmp for data tasks which will eventually slow a server using apis down. I don’t think this warranted another PR as I was looking at URLSession anyway and it was such a minor change to do so. Tested using a foundation test run.

[parkera]

Seems like we could leave it as the IUO, causing a crash if the protocol is ever unset in the other places (which seems like a precondition failure).

[itaiferber]

@swift-ci Please test

[johnno1962]

@parkera are you sure you want _protocol as a IUO?? It effectively makes cancel, suspend, resume unusable as they will crash now that _protocol can be nil if the request happens to have completed.

[parkera]

I'm not sure about the behavior on Darwin here, but that's what we should attempt to match.

The reason I was asking about removing the IUO here was that this leak fix also results in a behavior change to a silent failure if _protocol is nil, whereas as the IUO it would previously crash, right?

[johnno1962]

The leak fix doesn't change behaviour relative to darwin in itself. After the patch and using an IUO it does introduce a race condition where if you start a request but decide to cancel/suspend/resume after the request has already completed (and _protocol nil'd out) there will be a crash as opposed to just ignoring the cancel which is I image what the Darwin implementation would do.

[johnno1962]

Just checked. Darwin behaviour on resume for invalid protocol is call completion handler with error:
2017-09-01 19:54:15.646020+0100 zproto[13718:4235341] Fetch -1 nil Optional(Error Domain=NSURLErrorDomain Code=-1002 "unsupported URL" UserInfo={NSUnderlyingError=0x608000248ac0 {Error Domain=kCFErrorDomainCFNetwork Code=-1002 "(null)"}, NSErrorFailingURLStringKey=john://www.bbc.co.uk/news, NSErrorFailingURLKey=john://www.bbc.co.uk/news, NSLocalizedDescription=unsupported URL})

[johnno1962]

I think I’ve converged. Over to you @parkera

[alblue]

@swift-ci please test

[pushkarnk]

The retain cycle was expected to be broken as done in this PR. Looks like we missed this during the refactoring in #968 The creation of temp files only when required (by download tasks) also looks good to me.

[pushkarnk]

@swift-ci please test

[parkera]

@pushkarnk if you're happy with this, go ahead and merge

[pushkarnk]

@swift-ci test and merge

[pushkarnk]

@swift-ci please test and merge

[johnno1962]

Thanks @pushkarnk 👍 Did this fix make Swit-4.0?

[ianpartridge]

No, it'll need a separate PR against swift-4.0-branch.

[johnno1962]

I can do a PR but Is it still open?

[ianpartridge]

@parkera knows for sure. Even if 4.0 is closed though, I think it is quite possible there will be a 4.0.1 release soon after, cut from the same branch. There was a 3.0.1 and 3.0.2 last year.