dylanshine / openai-kit

A community Swift package used to interact with the OpenAI API
https://platform.openai.com/docs/api-reference
MIT License
703 stars 109 forks source link

Shutdown HTTPClient in deinit of Client #31

Closed dylanshine closed 1 year ago

dylanshine commented 1 year ago

@ronaldmannak In an effort to clean up the API, and abstract the use of HTTPClient for basic use cases, I thought providing a default HTTPClient would simplify things. Curious on your thoughts.

ronaldmannak commented 1 year ago

This is excellent. In the rare instance where you want to use your custom HTTPClient, you can still override it.

Developers must note that if they use a different HTTPClient type that doesn't respond to syncShutdown(), they need to shut down the HTTPClient manually.

Additionally, if syncShutdown is called on a custom HTTPClient, invoking syncShutdown in deinit may intentionally crash a debug build. To prevent this, consider adding the following to deinit: guard MultiThreadedEventLoopGroup.currentEventLoop == nil else { return } (I haven't checked it but that must work, right?)

HTTPClient.swift line 176:

    internal func syncShutdown(requiresCleanClose: Bool) throws {
        if let eventLoop = MultiThreadedEventLoopGroup.currentEventLoop {
            preconditionFailure("""
            BUG DETECTED: syncShutdown() must not be called when on an EventLoop.
            Calling syncShutdown() on any EventLoop can lead to deadlocks.
            Current eventLoop: \(eventLoop)
            """)
        }
        ... 
ronaldmannak commented 1 year ago

Looks great!