Fix thread leak in FileDownloadDelegate

[dnadoba]

Fixes #364

Modification

• remember if we own the thread pool or not.
• shutdown the thread pool if we own it after we are done with it

Result

we no longer leak one thread per download

[dnadoba]

API breakage failure is a false positive

16:14:03 1 breaking change detected in AsyncHTTPClient:
16:14:03   💔 API breakage: constructor FileDownloadDelegate.init(path:pool:reportHead:reportProgress:) has removed default argument from parameter 1

The change in question can be simplified to:

struct Foo {
    // old init
    init(bar: Int = 0) {}
    // new inits
    init() {}
    init(bar: Int) {}
}

replacing a default argument with two inits, one with the argument and one without, is an ABI breaking change but not an API breaking change.

[weissi]

we shouldn't destroy this in deinit, you don't even have a real guarantee that deinit isn't running in a thread owned by the pool (which would make it deadlock).

I'd suggest to add a lifecycle hook (shutdown or so) to the delegate and have AHC invoke that. That new shutdown method can have an empty default implementation to not break the API.

Separately, I think AHC should provide the thread pool or else we spin up/down a thread per download and that kinda makes it pointless to do asynchronous I/O if we need a new thread each time.

[Lukasa]

Is a lifecycle hook compatible with AHC providing the thread pool?

I'm inclined to say that if the user doesn't provide a thread pool we should just use a global static. Users can override the thread pool for testing if they need to shut stuff down, but the default case is that they don't care and the threads can be cleaned up at termination.

How do you feel about that?

[weissi]

actually you could add two delegate methods (with default empty impls):

• provideThreadPool(ioPool) where AHC provides a running thread pool to the delegate
• shutdown() where AHC tells the delegate that it its job is done

After that, we can change the default here and have the delegate not even have the option to accept an io pool from the user.

[dnadoba]

If AHC provides the default shared thread pool we don't need the shutdown() lifecycle hook because the delegate doesn't need to shutdown the thread pool anymore but the AHC client.

[weissi]

that's true

[dnadoba]

I have added a shared file IO thread pool to the HTTPClient which is passed to the delegate through a call to provideSharedThreadPool(fileIOPool:)
1afc151

[weissi]

awesome, thank you!

[dnadoba]

I have changed the implementation because we would otherwise only ever use one thread for all file IO. This is not good because file IO can block the thread and there is no easy way of making it non-blocking. We therefore initialize the thread pool with ProcessInfo.processInfo.processorCount threads. As this is potentially much more expensive operation compared to just creating one thread we also delay initialization until the first actual write.

[Lukasa]

Generally looks good, thanks, but we'll want some tests of the functionality I think.

[dnadoba]

I just realized that we actually don't need to make this public. @Lukasa any objections to me making it internal?

[Lukasa]

None at all.

[dnadoba]

Done in c3c68b2

[dnadoba]

@Lukasa test added in 9ee5c82