Don't use deinit to shutdown http client

[t089]

Shutting down the HTTP client in deinit might be problematic. I think, it is usually better to make this explicit by exposing the shutdown/syncShutdown methods directly on the KubernetesClient.

For example, if you were using the KubernetesClient in a NIO app and you happened to deinit an instance of the client on an event loop thread, you would be blocking this event loop which is verboten.

For example:

let eventLoop = elg.next()
        
let deployments : EventLoopFuture<apps.v1.DeploymentList> = eventLoop.flatSubmit {
    let client = KubernetesClient(provider: .shared(elg), logger: nil)!
    return client.appsV1.deployments.list()
}

print(try deployments.wait())

This hits a precondition error because the deinit and thus the syncShutdown of the client instance happens on the event loop thread.

Fatal error: BUG DETECTED: syncShutdown() must not be called when on an EventLoop.
Calling syncShutdown() on any EventLoop can lead to deadlocks.
Current eventLoop: SelectableEventLoop { selector = Selector { descriptor = 3 }, thread = NIOThread(name = NIO-ELT-0-#0) }: file AsyncHTTPClient/HTTPClient.swift, line 138
2021-01-09 22:32:06.490788+0100 [50257:6763987] Fatal error: BUG DETECTED: syncShutdown() must not be called when on an EventLoop.
Calling syncShutdown() on any EventLoop can lead to deadlocks.
Current eventLoop: SelectableEventLoop { selector = Selector { descriptor = 3 }, thread = NIOThread(name = NIO-ELT-0-#0) }: file AsyncHTTPClient/HTTPClient.swift, line 138

[t089]

Thinking about it, there is probably also a problem with the init method. In the example above, the init of the client happens on the event loop thread. And because the init will try to load the config / credentials from the filesystem, this is also a hidden blocking IO call. The NIO way of doing things would be to use NonBlockingFileIO to access the local filesystem.

I see two options:

1. Make the init package internal and provide a public static makeAutoConfiguredClient factory method, that returns an EventLoopFuture<KubernetesClient>.
2. Alternatively, factor the auto-configuration part out of the client. The KubernetesClientConfig type itself could have an async static factory method, that does the auto configuration from the filesystem using NonBlockingFileIO, and then returns an EventLoopFuture<KubernetesClientConfig?> which can be used to create a client instance.

[iabudiab]

An explicit syncShutdown instead of deinit is definitely a better approach.

The init is another story. I haven't really thought about creating KubernetesClient instances on an EventLoop on demand, but rather initialised once on App start and then reused throughout the whole lifecycle.

Something like the Vapor example: https://github.com/swiftkube/examples/tree/main/swiftkubedash

The app is configured:
https://github.com/swiftkube/examples/blob/7a97f89eb3fbbc4d513abc037de7d8b91ac6b241/swiftkubedash/Sources/Run/main.swift#L7-L10

And the KubernetesClient is initialised on the main thread once:
https://github.com/swiftkube/examples/blob/7a97f89eb3fbbc4d513abc037de7d8b91ac6b241/swiftkubedash/Sources/App/configure.swift#L48-L52

then it can be (re)used on any EventLoop. This should be however mentioned or better explained in the docs.

I am quite frankly not sure yet, if the async init of the client the right way to go. I would like to hear more input/feedback from a wider audience first 😉. Hopefully we'll get more enthusiastic users soon 🤞

[t089]

An explicit syncShutdown instead of deinit is definitely a better approach.

I really think you need both sync and async shutdown.

The init is another story. I haven't really thought about creating KubernetesClient instances on an EventLoop on demand, but rather initialised once on App start and then reused throughout the whole lifecycle.

Yes this is fair. I guess this would be the most common use case. For context, I’m doing it on the event loop because I need to recreate the client once my credentials expire.

then it can be (re)used on any EventLoop. This should be however mentioned or better explained in the docs.

documenting it would be a good solution I think

I am quite frankly not sure yet, if the async init of the client the right way to go. I would like to hear more input/feedback from a wider audience first 😉. Hopefully we'll get more enthusiastic users soon 🤞

The client works really well 👍 at least in my very limited use case 🙂