AsyncHTTPClient support for linux#143
AsyncHTTPClient support for linux#143mattt merged 4 commits intohuggingface:mattt/gate-linux-urlsessionfrom
Conversation
|
Any updates on this perhaps? |
|
Btw, as it stands now, the increased Internally, we use modified timeouts along with another shared singleton session, but this is not ideal (AsyncHTTPClient does not expose the means of creating a proper singleton that cannot be shut down as they do with their own .shared singleton), thus not included in the PR. If you have any suggestions regarding default timeouts etc., I'd be happy to improve the PR in that direction. |
There was a problem hiding this comment.
Pull request overview
Adds an optional AsyncHTTPClient-based transport path (via SwiftPM traits) to avoid Linux URLSession/FoundationNetworking concurrency issues, while keeping the existing URLSession-based behavior when the trait is not enabled.
Changes:
- Introduces
SessionType+makeDefaultSession()to abstract overURLSessionvsAsyncHTTPClient.HTTPClient. - Updates the network session types in model implementations to use
SessionType. - Adds
HTTPClientconvenience extensions for JSON fetch + streaming/SSE, and wires conditional dependencies/traits inPackage.swift.
Reviewed changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| Sources/AnyLanguageModel/Transport.swift | Adds conditional SessionType + default session factory for URLSession vs AsyncHTTPClient. |
| Sources/AnyLanguageModel/Models/OpenResponsesLanguageModel.swift | Switches stored session type and init default to SessionType. |
| Sources/AnyLanguageModel/Models/OpenAILanguageModel.swift | Switches stored session type and init default to SessionType. |
| Sources/AnyLanguageModel/Models/OllamaLanguageModel.swift | Switches stored session type and init default to SessionType. |
| Sources/AnyLanguageModel/Models/GeminiLanguageModel.swift | Switches stored session type and init defaults to SessionType (both initializers). |
| Sources/AnyLanguageModel/Models/AnthropicLanguageModel.swift | Switches stored session type and init default to SessionType. |
| Sources/AnyLanguageModel/Extensions/HTTPClient+Extensions.swift | Adds AsyncHTTPClient-backed fetch, newline-stream decoding, and SSE decoding helpers. |
| Package.swift | Adds AsyncHTTPClient trait + conditional dependencies, and forwards the trait to EventSource. |
| Package.resolved | Locks AsyncHTTPClient + transitive NIO dependencies and bumps EventSource resolution. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let asyncBytes = AsyncStream<UInt8> { byteContinuation in | ||
| SwiftTask { | ||
| do { | ||
| for try await buffer in response.body { | ||
| for byte in buffer.readableBytesView { |
There was a problem hiding this comment.
In fetchEventStream, the AsyncStream<UInt8> is produced by launching an inner Task (the one iterating response.body), but that task is not stored or cancelled when the outer AsyncThrowingStream terminates. This can leave a request/body-consumption task running after the consumer cancels, keeping the connection and resources alive. Consider keeping a reference to that inner task and cancelling it (and finishing the byte stream) in an onTermination handler (or otherwise wiring cancellation through so cancelling the stream reliably stops reading the body).
| #if canImport(FoundationNetworking) | ||
| import FoundationNetworking | ||
| #endif | ||
| import JSONSchema |
There was a problem hiding this comment.
FoundationNetworking and JSONSchema appear to be unused in this file, which will trigger unused-import warnings (and may fail CI if warnings are treated as errors). Please remove unused imports (or add uses if they’re intentional).
| #if canImport(FoundationNetworking) | |
| import FoundationNetworking | |
| #endif | |
| import JSONSchema |
| enum HTTPClientError: Error, CustomStringConvertible { | ||
| case invalidResponse | ||
| case httpError(statusCode: Int, detail: String) | ||
| case decodingError(detail: String) | ||
|
|
||
| var description: String { | ||
| switch self { | ||
| case .invalidResponse: | ||
| return "Invalid response" |
There was a problem hiding this comment.
New HTTPClientError mirrors URLSessionError, but there are no tests covering its description formatting or basic error cases under the AsyncHTTPClient trait. Since the repo already tests URLSessionError descriptions, it would be good to add equivalent tests guarded by #if canImport(AsyncHTTPClient) to prevent regressions when the trait is enabled.
| private let urlSession: SessionType | ||
|
|
||
| /// Creates an OpenAI language model. | ||
| /// |
There was a problem hiding this comment.
With the introduction of SessionType (URLSession vs AsyncHTTPClient.HTTPClient), the private property name urlSession is now misleading when the AsyncHTTPClient trait is enabled, and it makes the initializer docs (“URL session”) inaccurate. Consider renaming the stored property to something neutral like session (and updating the parameter docs accordingly) to avoid confusion.
|
Thanks so much for your work on this, @JonasProgrammer @KotlinFactory I have some follow-up changes, but it doesn't look like I can push directly to this branch, so I'll make that post-merge. |
056dccd
into
huggingface:mattt/gate-linux-urlsession
| @@ -0,0 +1,213 @@ | |||
| #if canImport(AsyncHTTPClient) | |||
| // AsyncHTTPClient.HTTPHandler introduces a Task type that clashes | |||
| typealias SwiftTask = Task | |||
There was a problem hiding this comment.
It's really too bad that Task is shadowed by AsyncHTTPClient and requires an internal typealias. Worse still that the alternative is referring to its fully-qualified _Concurrency.Task.
| } | ||
| } | ||
|
|
||
| enum HTTPClientError: Error, CustomStringConvertible { |
There was a problem hiding this comment.
This should probably conform to LocalizedError. I also generally like to make thrown error types public, unless this is wrapped elsewhere.
| public let model: String | ||
|
|
||
| private let urlSession: URLSession | ||
| private let urlSession: SessionType |
There was a problem hiding this comment.
bikeshed: I think this should be spelled HTTPSession
… race (#134) * Serialize Linux URLSession request paths to mitigate _MultiHandle race * Incorporate feedback from review * Replace withLock instance method with top-level withLinuxRequestLock helper * Fix Linux compiler bug around generic returning lock helper * More workarounds for Linux compiler bugs * Incorporate feedback from review * AsyncHTTPClient support for linux (#143) * deps: add trait-based import of AsyncHTTPClient * feat: implement transparent AsyncHTTPClient wrapper * deps: conditionally include trait in EventSource * Increase HTTP request timeout from 60 to 180 seconds --------- Co-authored-by: Leonhard Solbach <49833472+KotlinFactory@users.noreply.github.com> * Incorporate feedback from review * swift format -i -r . --------- Co-authored-by: Jonas Stoehr <jonass@dev.jsje.de> Co-authored-by: Leonhard Solbach <49833472+KotlinFactory@users.noreply.github.com>
As discussed in #127, the current workaround is not really feasible for async applications.
This PR includes an AsyncHTTPClient trait that does the following:
When untraited, the library should work as-is. With the trait active, the interface is changed to consume an HTTPClient as the session type.
I tried to change the business logic as little as possible, mostly implementing URLSession's extensions for HttpClient.