* Fix thread leak in `FileDownloadDelegate`
* `SwiftFormat`
* Add a shared file IO thread pool per HTTPClient
* User bigger thread pool and initlize lazily during first file write
* thread pool is actually not used in tests
* Update documentation
* fix review comments
* make `fileIOThreadPool` internal
* Add test to verify that we actually share the same thread pool across all delegates for a given HTTPClient
Co-authored-by: Cory Benfield <lukasa@apple.com>
* Tollerate more data after request body is cancelled
* wait is not needed if we shutdown the server first
* Remove test that depends on external resources
* Remove unused conformance to Equatable
* SwiftFormat
* run generate_linux_tests.rb
* Increase timeout for CI
Motivation
We should apply the connect timeout to the complete set of connection
attempts, rather than the request deadline. This allows users
fine-grained control over how long we attempt to connect for. This is
also the behaviour of our old-school interface.
Modifications
- Changed the connect deadline calculation for async/await to match that
of the future-based code.
- Added a connect timeout test.
Result
Connect timeouts are properly handled
Motivation
Documentation is nice, and we can help support users by providing useful
clear docs.
Modifications
Add Docc to 5.6 and later builds
Make sure symbol references work
Add overview docs
Result
Nice rendering docs
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
Warnings aren't great, and NIOAtomic is deprecated.
Modifications
Replace the last use of NIOAtomic with ManagedAtomic.
Result
Fewer warnings
Fixes#606
Motivation
If we receive an early HTTP response, the last action on a HTTP/1.1
connection is to send the .end message. While we had an error handling
path in the code, it wasn't tested, and when executed it would end up
leaking the connection by failing to close it _or_ return it to the
pool.
This patch fixes the issue by appropriately terminating the connection
and adding a test.
Modifications
Add a test
Terminate the connection if sendEnd fails
Result
Fewer connection leaks
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>
### Motivation
Today `didSendRequestPart` is called after a request body part has been passed to the executor. However, this does not mean that the write hit the socket. Users may depend on this behavior to implement back-pressure. For this reason, we should only call this `didSendRequestPart` once the write was successful.
### Modification
Pass a promise to the actual channel write and only call the delegate once that promise succeeds.
### Result
The delegate method `didSendRequestPart` is only called after the write was successful. Fixes#565.
Co-authored-by: Fabian Fett <fabianfett@apple.com>
### Motivation
Currently, we don’t consume the response body of redirect responses. Because of this requests, that receive a redirect response with response body, may hang indefinitely.
### Changes
- Consume redirect response body if less than 3kb
- Cancel redirect response if larger than 3kb
### Result
Redirect responses are consumed. Fixes#574
Same fix for HTTP/1 that landed for HTTP/2 in #558.
### Motivation
`HTTP1ClientChannelHandler` currently does not tolerate immediate write errors.
### Changes
Make `HTTP1ClientChannelHandler` resilient to failing writes.
### Result
Less crashes in AHC HTTP/1.
Motivation
The RequestBag intermediates between two different threads. This means
it can get requests that were reasonable when they were made but have
been superseded with newer information since then. These generally have
to be tolerated.
Unfortunately if we received a request to resume the request body stream
_after_ the need for that stream has been invalidated, we could hit a
crash. That's unnecessary, and we should tolerate it better.
Modifications
Tolerated receiving requests to resume body streaming in the finished
state.
Result
Fewer crashes
Fixes#576
### Motivation
If we follow a redirect which changes the origin e.g. from `127.0.0.1` to `localhost` we didn't change the `Host` header to the appropriate new origin and port combination.
### Changes
Use the original request which does not include the host instead of the prepared request to form a new request to the redirect URL.
### Alternatives
If the user defines a `Host` header themselves on the original `HTTPClientRequest` we currently never touch it, even in the redirect case. Maybe we should change our strategy and do one of the following:
1. We could always override the user defined `Host` header
2. We could only remove the user defined `Host` header on redirect and set it to the new origin and port combination
Swift tools version 5.3 and higher (the version that is specified at very top of a Package.swift file) excludes folders with a dot in the name by default. It luckily produces a warning "found 1 file(s) which are unhandled; explicitly declare them as resources or exclude from the target". However, this issue is buried under a lot of missing types Errors because of the 3 excluded files.
I run into this issue and it took me some time to figure out what the actual problem was. As we will eventually move from 5.2 to 5.3 we can already save the next person some time by resolving this issue now.
### Motivation
With Xcode 13.2, and therefore Swift 5.5.2, Swift Concurrecy is supported on older Apple OSs. async/await suport will no longer be available on Swift before `5.5.2` but this isn't a breaking change because we have not yet made anything of it public.
### Changes
- replace all `#if compiler(>=5.5) && canImport(_Concurrency)` with `#if compiler(>=5.5.2) && canImport(_Concurrency)`
- replace all `available(macOS 12.0, iOS 15.0, watchOS 8.0, tvOS 15.0, *)` with `available(macOS 10.15, iOS 13.0, watchOS 6.0, tvOS 13.0, *)`
* Fix HTTP1 to HTTP2 migration while shutdown is in progress
### Motivation
Calling `HTTPClient.shutdown()` may never return if connections are still starting and one new established connection results in a state migration (i.e. from HTTP1 to HTTP2 or vice versa). We forgot to migrate the shutdown state. This could result in a large dealy until `.shutdown()` returns because we wait until connections are closed because of idle timeout. Worse, it could also never return if more requests are queued because the connections would not be idle and therefore not close itself.
###Changes
- Mirgrate shutdown state too
- add tests for this specific case
* simplify testMigrationFromHTTP1ToHTTP2WhileShuttingDown
* add http2 to http1 migration test
* 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
David NadobaandGitHub
KarlandGitHub
Christian PriebeandGitHub
Johannes WeissandGitHub
