Swift log now has an InMemoryLogHandler. Lets depend on that instead of
having our own `CollectEverythingLogHandler`.
I've added an extension on top, to make it easier to create the logger
too
Result: less code
Motivation:
The various 'withMumbleContinuation' APIs are supposed to be invoked
synchronously with the caller. This assumption allows a lock to be
acquired before the call and released from the body of the
'withMumbleContinuation' after e.g. storing the continuation. However
this isn't the case and the job may be re-enqueued on the executor
meaning that this is pattern is vulnerable to deadlocks.
Modifications:
- Rework the test helpers to avoid holding a lock when a continuation is
created.
- Switch to using NIOLockedValue box
Result:
Lower chance of deadlock
### Motivation
When creating a connection, we wrongfully assumed that
`failedToCreateNewConnection` will always be called before
`http*ConnectionClosed` in the `HTTPConnectionPoolStateMachine`. However
this is far from correct. In NIO Futures are fulfilled before
`ChannelHandler` callbacks. Ordering in futures should not be assumed in
such a complex project.
### Change
We change the `http*ConnectionClosed` methods to be noops, if the
connection is in the starting state. We instead wait for the
`failedToCreateNewConnection` to create backoff timers and friends.
rdar://164674912
---------
Co-authored-by: George Barnett <gbarnett@apple.com>
Motivation:
The various 'withMumbleContinuation' APIs are supposed to be invoked
synchronously with the caller. This assumption allows a lock to be
acquired before the call and released from the body of the
'withMumbleContinuation' after e.g. storing the continuation. However
this isn't the case and the job may be re-enqueued on the executor
meaning that this is pattern is vulnerable to deadlocks.
Modifications:
- Drop and reacquire the lock in Transaction
Result:
Lower chance of deadlock
Motivation:
* More secure GitHub Actions workflows
Modifications:
Add explicit 'contents: read' permissions to workflows that did not have
explicit permissions defined. This follows GitHub Actions security best
practices by limiting the default GITHUB_TOKEN permissions.
Result:
An extra layer of security.
Motivation
Right now, we insert HTTP/2 handlers in a callback on a future that is
done very late. The result of this is that an entire ALPN negotiaton
_can_ complete before this callback is attached. That can in rare cases
cause the HTTP/2 handler to miss the server preamble, because it gets
added too late.
Modifications
This patch refactors the existing code to close that window. It does so
by passing a promise into the connection path and completing that
promise _on_ the event loop where we add the ALPN handlers, which should
ensure this will execute immediately when the ALPN negotiation
completes. Immportantly, we attach our promise callbacks to that promise
_before_ we hand it off, making sure the timing windows go away.
Results
Timing window is closed
Motivation:
Swift 6.2 has been released, we should add it to our CI coverage.
Modifications:
Add additional Swift 6.2 jobs where appropriate in main.yml,
pull_request.yml
Result:
Improved test coverage.
Motivation
This patch adds support for HTTP/1 connection pre-warming. This allows
the user to request that the HTTP/1 connection pool create and maintain
extra connections, above-and-beyond those strictly needed to run the
pool. This pool can be used to absorb small spikes in request traffic
without increasing latency to account for connection creation.
Modifications
- Added new configuration properties for pre-warmed connections.
- Amended the HTTP/1 state machine to create new connections where
necessary.
- Added state machine tests.
Results
Pre-warmed connections are available.
Motivation:
The trailing space is visible in log message metadata, and depending
upon the log handler in use, will sometimes be visible due to quoting.
Modifications:
Just remove the trailing space.
Result:
There won't be a trailing space in the key anymore. This has no
functional impact whatsoever as far as I was able to determine.
Motivation:
testSimpleGetRequest asserts right after submitting the request for
execution that no streams have been closed. While unlikely there's
nothing to prevent this assertion from failing (and indeed it has
failed): the request may have executed immediately and the stream closed
before checking the delegate.
Modifications:
- Remove the assertion
Result:
Tests are less flaky
### Motivation:
Some errors do not show up in debug builds. Enabling release mode builds
improves the CI coverage.
### Modifications:
Enable release mode builds for pull requests and scheduled builds on
main.
### Result:
Improved CI coverage.
Fixes#847.
Motivation:
On 32-bit systems, using .randomElement on a range larger than what can
fit in Int32 (Int) causes a crash. After only 26 or 27 retries of a
request using HTTPClient, the calculateBackoff method would run into
this and crash consistently on an armv7 (32-bit) device.
Modifications:
A one-line fix to opt to using Int64.random on the same jitterRange
instead of .randomElement, which works as expected without crashing on
32-bit systems.
Result:
The HTTPClient now works as expected and can perform as many retries as
needed without crashing.
I tested this on my armv7 board doing the retries, and ran up to several
hundred repetitions after a few hours with no crashes as was happening
before.
@Lukasa
Motivation:
An oversight in 373862a meant that 5.9 wasn't actually dropped which
means that the use of 'nonisolated(unsafe)' in 0e715a27 broke users of
5.9.
Modifications:
- Add back a 5.9 path
Result:
- Builds on 5.9
- Resolves#843
Motivation:
Now that strict concurrency has been adopted the AHC should avoid
regressing by treating all warnings as errors in CI.
Modifications:
- Treat warnings as errors in CI
Result:
Stricter CI
Motivation:
The tests shouldn't be making sendability violations.
Modifications:
Fix the warnings
Result:
- No warnings
- Strict concurrency is adopted!
Motivation:
There are a couple of sendability warnings leftover in Sources.
Transaction moves a closure into a task. The closure isn't Sendable (and
shouldn't be). However, higher up the stack there's a closure which
generates the non-sendable closure which can be sendable.
Modifications:
- Pass the sendable closure generating closure down rather
- Add a few more explicit sendable annotations
Result:
Fewer warnings
Motivation
We have some Task error handling functions that are generic for no
apparent reason. They're also typically called from contexts where they
also report the error to the delegate, but one of the call sites doesn't
do that. So add a test for that as well.
Modifications
- Rewrite Task.fail(with:delegate:) to be non-generic.
- Add a call to the delegate error handler on the path that is missing
it.
- Add a test for that call
Results
Cleaner, easier to follow code
Motivation:
The response accumulator is a delegate which must be sendable as it's
passed across isolation domains.
Modifications:
- Make delegates have a sendable requirement
- Make the response accumulator sendable
Result:
Delegates, and the response accumulator, are sendable
Motivation:
RequestBag conforms to HTTPExecutableRequest to must be Sendable.
Modifications:
- Move event-loop bound state to a loop-bound box
- Remove redundant event-loop checks (they are performed by the loop
bound box)
Result:
Fewer warnings
Motivation:
Delegates can be passed from any thread and are executed on an arbitrary
event loop. That means they need to be Sendable. Rather than making them
all Sendable in one go, we'll do the larger ones separately.
Modifications:
- Make FileDownloadDelegate sendable
Result:
Safe to pass FileDownloadDelegate across isolation domains
Motivation:
The body stream writer can be sent across isolation domains so should be
sendable.
Modifications:
- Make it explicitly sendable
- Add appropriate preconcurrency annotations
- Wrap an iterator from swift-algorithms as it hasn't yet annotated its
types with Sendable
Result:
Body stream writer is sendable
Motivation:
The connection pool holds much of the low level logic in AHC. We should
fix its sendability issues before moving to higher levels.
Modifications:
- Make HTTP1ConnectionDelegate and HTTP2Delegate sendable, this requires
passing IDs rather than connections to their methods
- Make HTTPConnectionRequester sendable and have its methods take
Sendable views of the HTTP1Connection and HTTP2Connection types
- Add sendable views to HTTP1Connection and HTTP2Connection
- Mark HTTP1Connection and HTTP2Connection as not sendable
- Make HTTPRequestExecutor and HTTPExecutableRequest sendable
- Update tests
Result:
Connection pool has stricter sendability requirements
Motivation:
We're about to go on a sendability journey. Let's pick some low hanging
fruit to get started.
Modifications:
- Add a few assume-isolated calls
- Stop using static var
- Use a dispatch group instead of a work item to wait for work to be
done.
Result:
Fewer warnings
Motivation:
As part of adopting strict concurrency all public types should be
explicit about whether they are sendable or not.
Modifications:
- Add explicit sendability annotations to a number of types
Result:
Sendability is explicit
Motivation:
As requested in #596, it can be handy to have a lower-level access to
channels (HTTP/1 connection, HTTP/2 connection, or HTTP/2 stream) to
enable a more fine-grained interaction for, say, observability, testing,
etc.
Modifications:
- Add 3 new properties (`http1_1ConnectionDebugInitializer`,
`http2ConnectionDebugInitializer` and
`http2StreamChannelDebugInitializer`) to `HTTPClient.Configuration` with
access to the respective channels. These properties are of `Optional`
type `@Sendable (Channel) -> EventLoopFuture<Void>` and are called when
creating a connection/stream.
Result:
Provides APIs for a lower-level access to channels.
---------
Co-authored-by: Cory Benfield <lukasa@apple.com>
Co-authored-by: David Nadoba <d_nadoba@apple.com>
Co-authored-by: George Barnett <gbarnett@apple.com>
Motivation:
Now that Swift 6.1 has been released, Swift 5.9 has dropped out of the
support window.
Modifications:
- Bumpt tools versions to 5.9
- Disable 5.9 workflows
Result:
5.9 is no longer supported
Motivation:
Swift 6.1 has been released, we should add it to our CI coverage.
Modifications:
Add additional Swift 6.1 jobs where appropriate in main.yml,
pull_request.yml
Result:
Improved test coverage.
Trying to pave the way for closing
https://github.com/swift-server/async-http-client/issues/790 with some
direction from @Lukasa.
I have no idea where is best to insert this new delegate method. I'm
currently doing it first thing in `receiveResponseHead0`, and not using
an EventLoopFuture for back pressure management. The state machine seems
pretty fragile and I don't want to leave too much of an imprint. Trying
to be a part of an EventLoopFuture chain seems really complicated and
would really leave a mark on the codebase, so I'm wondering if it's
possible to just warn the user "do not block"?
Anyway, just a jumping-off point and happy to take direction!
I needed a way to use a `FileDownloadDelegate` task to fish out the
recommended file name.
```swift
let response = try await downloadTask.get()
// access content-disposition
response.head.headers.first(name: "Content-Disposition")
```
The `head` property is an explicitly unwrapped optional because there is
no "default value" to set it to, and it won't be accessed by the user
until it's already been set anyway. This is a little inelegant, so I
could change it to something like below where I fill in bogus init data,
but that seems worse for some reason.
```swift
public struct Progress: Sendable {
public var totalBytes: Int?
public var receivedBytes: Int
public var head: HTTPResponseHead
}
private var progress = Progress(
totalBytes: nil,
receivedBytes: 0,
head: .init(
version: .init(major: 0, minor: 0),
status: .badRequest
)
)
```
Motivation
Foundation has reverted several of the changes of behaviour in the URL
type, leaving 6.0 and 6.1 with a different behaviour on non-Apple
platforms than all other versions.
We should tolerate that.
Modifications
Update the tests to understand the difference.
Result
Tests pass
Motivation
EmbeddedEventLoop is not thread-safe, which means that outside of very
rare use-cases it's not safe to use it in Swift Concurrency.
Modifications
Replace invalid uses of EmbeddedEventLoop with NIOAsyncTestingEventLoop
Result
Better safety
Specifically Swift 5.10 _on Intel on Ubuntu Noble (24.04)_ has a crazy
bug which leads to compilation failures in a `#if compiler(>=6.0)`
block: https://github.com/swiftlang/swift/issues/79285 .
This workaround fixes the compilation by _changing the whitespace_.
Thanks @gwynne for finding this workaround!
---------
Co-authored-by: Johannes Weiss <johannes@jweiss.io>
At the moment, `HTTPClient`'s entire API surface violates Structured
Concurrency. Both the creation & shutdown of a HTTP client as well as
making requests (#807) doesn't follow Structured Concurrency. Some of
the problems are:
1. Upon return of methods, resources are still in active use in other
threads/tasks
2. Cancellation doesn't always work
This PR is baby steps towards a Structured Concurrency API, starting
with start/shutdown of the HTTP client.
Co-authored-by: Johannes Weiss <johannes@jweiss.io>
hamzahrmalik
George Barnett
Fabian FettandGeorge Barnett
Cory Benfield
Rick Newton-Rogers
Konrad `ktoso` Malawski
Konrad `ktoso` Malawski
Gwynne Raskind
Johannes Weiss
Raphael
jessezamora
Gus Cairo
Greg Cotten
Allan Shortlidge