Motivation
URL parsing has changed in macOS Sonoma and associated releases. Our
tests were reliant on the old behaviour. The behaviour is controlled
by the OS on which the program was linked, which makes it impossible
for us to programmatically work out which result we should see.
The affected tests are not actually useful, we don't really care how
the URLs parse, so we can safely just remove them.
Modifications
Remove the affected tests.
Result
Tests pass!
Motivation:
Sometimes it can be helpful to limit the number of times a connection
can be used before discarding it. AHC has no such support for this at
the moment.
Modifications:
- Add a `maximumUsesPerConnection` configuration option which defaults
to `nil` (i.e. no limit).
- For HTTP1 we count down uses in the state machine and close the
connection if it hits zero.
- For HTTP2, each use maps to a stream so we count down remaining uses
in the state machine which we combine with max concurrent streams to
limit how many streams are available per connection. We also count
remaining uses in the HTTP2 idle handler: we treat no remaining uses
as receiving a GOAWAY frame and notify the pool which then drains the
streams and replaces the connection.
Result:
Users can control how many times each connection can be used.
* Reproducer
* Refactor test case
* Refactor tests
* Remove debugging artefacts
* Fix typo
* Fix formatting
* Remove `promise?.succeed(())`
* Add test for HTTP2 request with large header
Motivation
We currently don't handle large headers well which trigger a channel writability change event.
Modification
Add failing (but currently skipped) tests which reproduces the issue
Result
We can reliably reproduce the large request header issue in an integration and unit test.
Note that the actual fix is not included to make reviewing easier and will come in a follow up PR.
* Remove logging
* Fix crash for large HTTP request headers
Fix crash for when sending HTTP request headers result in a channel writability change event
* Formatting and linux tests
* Formatting and linux tests
* Generate linux tests
* Use previous default max concurrent streams value of 10
* Fix crash if request is canceled after request header is send
* generate linux tests and run swift format
---------
Co-authored-by: Cory Benfield <lukasa@apple.com>
Motivation
We currently don't handle large headers well which trigger a channel writability change event.
Modification
Add failing (but currently skipped) tests which reproduces the issue
Result
We can reliably reproduce the large request header issue in an integration and unit test.
Note that the actual fix is not included to make reviewing easier and will come in a follow up PR.
* Handle ResponseAccumulator not being able to buffer large response in memory
Check content length header for early exit
* Add test which currently hangs indefinitely
* Run `generate_linux_tests.rb` and `SwiftFormat`
* Print type and value if assert fails
* Run `generate_linux_tests.rb` and `SwiftFormat`
* Remove duplicate test due too merge conflict
* Validate that maxBodySize is positive
* Address review comments
Motivation
When receiving certain patterns of response body parts, we can end up
recursing almost indefinitely to deliver them to the application. This
can lead to crashes, so we might politely describe it as "sub-optimal".
Modifications
Keep track of our stack depth and avoid creating too many stack frames.
Added some unit tests.
Result
We no longer explode when handling bodies with lots of tiny parts.
Co-authored-by: David Nadoba <d_nadoba@apple.com>
Motivation
When users stream their bodies they may still want to send Connection:
close headers and terminate the connection early. This should work
properly.
Unfortunately it became clear that we didn't correctly pass the
information that the connection needed to be closed. As a result, we'd
inappropriately re-use the connection, potentially causing unnecessary
HTTP errors.
Modifications
Signal whether the connection needs to be closed when the final
connection action is to send .end.
Results
We behave better with streaming uploads.
Motivation
In some cases, the last thing that happens in a request-response pair is
that we send our HTTP/1.1 .end. This can happen when the peer has sent
an early response, before we have finished uploading our body. When it
does, we need to be diligent about cleaning up our connection state.
Unfortunately, there was an edge in the HTTP1ConnectionStateMachine that
processed .succeedRequest but that did not transition the state into
either .idle or .closing. That was an error, and needed to be fixed.
Modifications
Transition to .idle when we're returning
.succeedRequest(.sendRequestEnd).
Result
Fewer crashes
Motivation
It's totally acceptable for a HTTP server to respond before a request
upload has completed. If the response is an error, we should abort the
upload (and we do), but if the response is a 2xx we should probably just
finish the upload.
In this case it turns out we'll actually hit a crash when we attempt to
deliver an empty body message. his is no good!
Once that bug was fixed it revealed another: while we'd attempted to
account for this case, we hadn't tested it, and so it turns out that
shutdown would hang. As a result, I've also cleaned that up.
Modifications
- Tolerate empty circular buffers of bytes when streaming an upload.
- Notify the connection that the task is complete when we're done.
Result
Fewer crashes and hangs.
Motivation
Currently error reporting with NIO Transport Services is often sub-par.
This occurs because the Network.framework connections may enter the
waiting state until the network connectivity state changes. We were not
watching for the user event that contains the error in that state, so if
we timed out in that state we'd just give a generic timeout error,
instead of telling the user anything more detailed.
Additionally, several of our tests assume that failure will be fast, but
in NIO Transport Services we will enter that .waiting state. This is
reasonable, as changed network connections may make a connection that
was not succeeding suddenly viable. However, it's inconvenient for
testing, where we're mostly interested in confirming that the error path
works as expected.
Modifications
- Add an observer of the WaitingForConnectivity event that records it
into our state machine for later reporting.
- Add support for disabling waiting for connectivity for testing
purposes.
- Add annotations to several tests to stop them waiting for
connectivity.
Results
Faster tests, better coverage, better errors for our users.
Co-authored-by: David Nadoba <dnadoba@gmail.com>
* refactor RedirectHandler
- `redirectState` is no longer a property of `HTTPClient.Request`. RedirectHandler now stores this state directly and therefore no longer optional.
- we no longer count the number of allowed redirects down. Instead the number of redirects is dervied from `self.visited.count` and we compare it to the maxRedirect to check if we git the limit.
* `HTTPClient.Configuration.RedirectConfiguration.Configuration` is now called `HTTPClient.Configuration.RedirectConfiguration.Mode`
only two `Configuration`s left in the type name
* add redirect logger test
### Motivation
Our current swiftformat version does not support async/await. Since we want to add support for async/await we must update swiftformat or disable it. I tried my very best to keep the number of changes as small as possible. I assume we want to stick with the new 0.48.8 for some time.
### Changes
- Update swiftformat to 0.48.8
### Result
We can land async/await code.
### Motivation
Fixes#238 and #231.
### Changes
- Extracted the unclean shutdown test from `HTTPClientTests` into their own file `HTTPClientUncleanSSLConnectionShutdownTests`
- Copy and pasted @weissi great explanation from #238 into the test file
- Removed property `ignoreUncleanSSLShutdown` everywhere
### Result
`ignoreUncleanSSLShutdown` on `HTTPClient.Configuration` is deprecated and ignored.
Co-authored-by: Johannes Weiss <johannesweiss@apple.com>
- a new `RequestOptions` struct was created, that can be used to set request specific options. It is required by the `HTTPExecutableRequest`
- Added support for `ignoreUncleanSSLShutdown` in the `HTTPRequestStateMachine` and the `HTTP1ConnectionStateMachine`. In http/2 `ignoreUncleanSSLShutdown` is always off.
### Motivation
If we call
```
context.write(ByteBuffer(string: "foo"), promise: nil)
context.flush()
context.close()
```
there is no guarantee that `"foo"` will be written to the network. Currently the test `testNoBytesSentOverBodyLimit` assumes this however.
### Changes
- `testNoBytesSentOverBodyLimit` does not assume anymore, that bytes will be sent before a close of the connection.
- Use `XCTAssertEqual` to compare `Equatable` errors.
- The connection creation logic has been refactored into a number of smaller methods that can be combined
- Connection creation now has a logical home. It is moved from `Utils.swift` into a `ConnectionFactory`
- There are explicit `ChannelHandlers` that are used for connection creation:
- `TLSEventsHandler` got its own file and unit tests
- `HTTP1ProxyConnectHandler` got its own file and unit tests
- `SOCKSEventsHandler` got its own file and unit tests
- Some small things are already part of this pr that will get their context later. For example:
- `HTTPConnectionPool` is added as a namespace to not cause major renames in follow up PRs
- `HTTPConnectionPool.Connection.ID` and its generator were added now. (This will be used later to identify a connection during its lifetime)
- the file `HTTPConnectionPool+Manager` was added to give `HTTPConnectionPool.Connection.ID.Generator` already its final destination.
- Add `swift-nio-http2` as a dependency to the Package.swift (only used in the test target so far).
- Remove unused initialization options for `HTTPBin`
- The initialization options `ssl: Bool = false, compress: Bool = false, refusesConnections: Bool = false` move into a `Mode` enum. The mode might be `.http1_1(ssl: Bool, compress: Bool)`, `.http2(compress: Bool)`, or `.refuse`
- Added support for `http/2` (not used in any tests yet, however I verified the support with curl locally.)
- Nearly all channel pipeline modifications in `HTTPBin` are synchronous now.
- HTTPBin's request handler is configurable. This allows testing of more esoteric cases without having to create a new server every time. The default `HTTPBin` continues to use the `HTTPBinHandler`.
Updated:
NIO
NIOSSL
NIO Extras
NIOTS
Also fix TLSConfiguration.forClient() warnings by converting to TLSConfiguration.makeClientConfiguration(). Also the same for forServer().
### Motivation
- The `testSSLHandshakeErrorPropagation` has a very short connect timeout (100ms). The reason for this is, that a failing TLS handshake manifest in NIOTS as a connect timeout and we don’t want to wait for the timeout for to long in our tests.
- Using a the NIOSSL however we expect a proper NIOSSLError. Sometimes though the connection is not setup fast enough (because of the very short 100ms connect timeout), that we get a connectionTimeout error instead.
### Modifications
- Setting the short connect timeout to 100ms when using NIOTS only. This will make sure we get the proper NIOSSLError when using NIOSSL
Currently the DelayOnHeadDelegate test utility depends on correct timings. Right now, if a request is slower than 50ms in `testErrorAfterCloseWhileBackpressureExerted` the test will fail, since the backpressure promise is failed, before a head was received. This pr fixes this, by giving the user a callback, when the head was received.
Motivation:
At the moment, AHC assumes that creating a `NIOSSLContext` is both cheap
and doesn't block.
Neither of these two assumptions are true.
To create a `NIOSSLContext`, BoringSSL will have to read a lot of
certificates in the trust store (on disk) which require a lot of ASN1
parsing and much much more.
On my Ubuntu test machine, creating one `NIOSSLContext` is about 27,000
allocations!!! To make it worse, AHC allocates a fresh `NIOSSLContext`
for _every single connection_, whether HTTP or HTTPS. Yes, correct.
Modification:
- Cache NIOSSLContexts per TLSConfiguration in a LRU cache
- Don't get an NIOSSLContext for HTTP (plain text) connections
Result:
New connections should be _much_ faster in general assuming that you're
not using a different TLSConfiguration for every connection.
Motivation:
Users of the HTTPClientResponseDelegate expect that the event loop
futures returned from didReceiveHead and didReceiveBodyPart can be used
to exert backpressure. To be fair to them, they somewhat can. However,
the TaskHandler has a bit of a misunderstanding about how NIO
backpressure works, and does not correctly manage the buffer of inbound
data.
The result of this misunderstanding is that multiple calls to
didReceiveBodyPart and didReceiveHead can be outstanding at once. This
would likely lead to severe bugs in most delegates, as they do not
expect it.
We should make things work the way delegate implementers believe it
works.
Modifications:
- Added a buffer to the TaskHandler to avoid delivering data that the
delegate is not ready for.
- Added a new "pending close" state that keeps track of a state where
the TaskHandler has received .end but not yet delivered it to the
delegate. This allows better error management.
- Added some more tests.
- Documented our backpressure commitments.
Result:
Better respect for backpressure.
Resolves#348
Motivation:
AsyncHTTPClient attempts to avoid the problem of Happy Eyeballs making
it hard to know which Channel will be returned by only inserting the
TLSEventsHandler upon completion of the connect promise. Unfortunately,
as this may involve event loop hops, there are some awkward timing
windows in play where the connect may complete before this handler gets
added.
We should remove that timing window by ensuring that all channels always
have this handler in place, and instead of trying to wait until we know
which Channel will win, we can find the TLSEventsHandler that belongs to
the winning channel after the fact.
Modifications:
- TLSEventsHandler no longer removes itself from the pipeline or throws
away its promise.
- makeHTTP1Channel now searches for the TLSEventsHandler from the
pipeline that was created and is also responsible for removing it.
- Better sanity checking that the proxy TLS case does not overlap with
the connection-level TLS case.
Results:
Further shrinking windows for pipeline management issues.
Johannes WeissandJohannes Weiss
Cory Benfield
David Nadoba
George Barnett
carolinacass
Karl
Fabian Fett
David Evans
Mads Odgaard
David Evans