Fix NSUrlSession memory leak by finishing session

[bherbst]

Subsystem
iOS client engine

Motivation
#1420 describes a memory leak in the iOS client. I have posted more details in that issue, but the gist of it is that when opening an NSUrlSession using sessionWithConfiguration(), the delegate is leaked until the app exits unless we call invalidateAndCancel() or finishTasksAndInvalidate() at some point. See the documentation for the delegate parameter on developer.apple.com for more information.

In effect, this meant that the engine, the session, and the coroutine used for executing the request are all leaked until the app dies.

Solution
After adding the data task to NSUrlSession, we immediate call finishTasksAndInvalidate(), which blocks creation of new tasks but allows the existing data task to complete. Once that data task completes, NSUrlSession will then break references to the delegate and callback objects, allowing those objects to become garbage collected.

[cy6erGn0m]

Do you know, if dataTaskWithRequest produce additional helper tasks? Shouldn't we invoke the finish function somewhere in the delegates callback instead just before resuming the continuation?

[dmitrievanthony]

HI @bherbst, thanks for the PR, seems like it fixes a significant issue. Speaking about the change, do I understand correctly that NSURLSession usage should be followed by invalidateAndCancel() or finishTasksAndInvalidate() in all cases? If it's right I think it makes sense to wrap this usage into try-finally block or, maybe even better, add fun NSURLSession.use { ... } extension with such a block. What do you think?

[cy6erGn0m]

The other alternative would be setting invalidateAndCancel on the call context completion if we are unsure about the exact time when to terminate.

[bherbst]

I'll start by saying I'm certainly no iOS networking expert, but here's my understanding:

Do you know, if dataTaskWithRequest produce additional helper tasks? Shouldn't we invoke the finish function somewhere in the delegates callback instead just before resuming the continuation?

The docs for dataTaskWithRequest seem to indicate only one is spawned, but finishTasksAndInvalidate will wait for all tasks to finish before invalidating, so that shouldn't be an issue. I put it directly after the resume() call because my assumption is that the task isn't yet running until we call resume() and thus if we tried to finishTasksAndInvalidate we would immediately invalidate the session since no tasks are running.

do I understand correctly that NSURLSession usage should be followed by invalidateAndCancel() or finishTasksAndInvalidate() in all cases? If it's right I think it makes sense to wrap this usage into try-finally block or, maybe even better, add fun NSURLSession.use { ... } extension with such a block.

Not necessarily- after talking to some iOS developers my belief is that most iOS developers use NSURLSession a bit differently than KTOR does. Instead of creating a new NSURLSession for each network request, they typically have a singleton session that they reuse for all requests. Since it is being reused, the session can't be invalidated. A use{ ... } extension method could be useful though! Would it be more appropriate to finishTasksAndInvalidate() or invalidateAndCancel?

[matarasso]

isn't this the same issue that was resolved here? #1342

[bherbst]

@matarasso Indeed it looks to be the same! Good find. Unfortunately that PR was only ever merged into ktorio:e5l/develop.

I've updated this PR to move the finishTasksAndInvalidate() call to invokeOnCompletion() as suggested in the other PR as well, so the two are now identical.

[matarasso]

A shame that this couldn't make it to 1.3.0, I've been using my own fork for months because this is a show stopper for an app that downloads hundreds of files such as ours.

[e5l]

Thanks. LGTM
Sorry for the delay, it took some time to clarify details