Clarify semantics of `shutdown`

[t089]

We should clarify what the semantics of shutdown are.

I think the following definition could make sense:

When a shutdown signal is received by the client

• It keeps processing already inflight tasks (following redirect, downloading / uploading, etc ...)
• It cancels tasks that are waiting to be processed
• It errors new tasks that are being scheduled after the signal was received
• Once all tasks have either failed or finished, if it owns the event loop it stops the event loop.

I think at the moment there is no queue of pending tasks, so point two is maybe not relevant yet.

[t089]

Maybe it would make sense to have a separate (invalidateAndCancel ) method similar to URLSession that also aborts inflight tasks.

[weissi]

Agreed! I think it should cancel them all with a well-known error: something like .requestCancelledBecauseClientShutDown or something.

[t089]

So actually point 2,3 could be the same "cancel error"

[PopFlamingo]

@t089 @weissi The currently tagged version (1.1.0) doesn't actually stop existing tasks when it's called, note it might stop them from executing if the client has been created with eventLoopGroupProvider: .createNew as the call will also shutdown the backing EventLoop.

There are changes in the pooling commit: existing tasks are now failed with HTTPClientError.cancelled when calling syncShutdown(), although I'm not sure of wether or not that completely fixes the possibility of futures never calling their callbacks if this stops the event loop too early.

Do we actually want to make syncShutdown() wait on currently running tasks or should this be the user responsibility?

Note there is exists the internal syncShutdown(requiresCleanClose: Bool) which will throw an error if attempting to shutdown while there are still active tasks, would this ever make sense to make it public, maybe with a different name?

[weissi]

@adtrevor if you fail all promises, then everything should run before the EventLoop can shutdown. Of course, if someone triggers something asynchronous from within those callbacks, that might again race against EventLoop shutdown. There's not much that NIO can do here (apart from crashing more often to tell you that you shut the EventLoop too early which we're adding in https://github.com/apple/swift-nio/pull/1395 )