* save progress
* Replace `TransactionBody` with `NIOAsyncSequenceProducer`
* test
* revert unnesscary changes
* Add end-to-end test
which currently fails because of https://github.com/apple/swift-nio/issues/2398
* soundness
* Use latest swift-nio release
* throw CancellationError on task cancelation
* Fix Swift 5.5 & 5.6
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.
Motivation:
not accumulate too many bytes
Modifications:
Implementing collect function to use NIOCore version to prevent overflowing
Co-authored-by: Cory Benfield <lukasa@apple.com>
Sometimes it can be useful to connect to one host e.g. `x.example.com` but request and validate the certificate chain as if we would connect to `y.example.com`. This is what this PR adds support for by adding a `dnsOverride` configuration to `HTTPClient.Configuration`. This is similar to curls `—resolve-to` option but only allows overriding host and not ports for now.
Motivation
Task.wait() is a convenience method to avoid needing to wait for the
response future. This has had the effect of "laundering" a noasync
warning from EventLoopFuture.wait(), hiding it within a purely sync call
that may itself be used in an async context.
We should discourage using these and prefer using .get() instead.
Modifications
Mark Task.wait() noasync.
Add Task.get() for backward compatibility.
Result
Safer migration to Swift concurrency
* 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.
* Add Swift 5.8 CI
* Update nightly CI to Ubuntu 22.04
* Fix warnings in test target with latest NIO release
Replace calls to `.wait()` with calls to `.get()`
Motivation
Allowing arbitrary data in outbound header field values allows for the
possibility that users of AHC will accidentally pass untrusted data into
those values. That untrusted data can substantially alter the parsing
and content of the HTTP requests, which is extremely dangerous. The
result of this is vulnerability to CRLF injection.
Modifications
Add validation of outbound header field values.
Result
No longer vulnerable to CRLF injection
* Tolerate shutdown message after channel is closed
### Motivation
A channel can close unexpectedly if something goes wrong. We may in the meantime have scheduled the connection for graceful shutdown but the connection has not yet seen the message. We need to still accept the shutdown message and just ignore it if we are already closed.
### Modification
- ignore calls to shutdown if the channel is already closed
- add a test which would previously crash because we have transition from the closed state to the closing state and we hit the deinit precondition
- include the current state in preconditions if we are in the wrong state
### Result
We don’t hit the precondition in the deinit in the scenario described above and have more descriptive crashes if something still goes wrong.
* 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
SwiftNIO 2.42.0 has deprecated Lock and replaced it with a new NIOLock. This commit removes all uses of Lock and replaces them with NIOLock. Further, now, we must require SwiftNIO 2.42.0
* 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
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
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
David Nadoba
George Barnett
carolinacassandCory Benfield
Fabian Fett
iMike
Johannes WeissandJohannes Weiss
Franz BuschandFabian Fett