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!
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>
fixes#784
`writeChunks` had 3 bugs:
1. An actually wrong `UnsafeMutableTransferBox` -> removed that type
which should never be created
2. A loooong future chain (instead of one final promise) -> implemented
3. Potentially infinite recursion which lead to the crash in #784) ->
fixed too
Migrate CI to use GitHub Actions.
### Motivation:
To migrate to GitHub actions and centralised infrastructure.
### Modifications:
Changes of note:
* Adopt swift-format using rules from SwiftNIO.
* Remove scripts and docker files which are no longer needed.
* Disabled warnings-as-errors on Swift 6.0 CI pipelines for now.
### Result:
Feature parity with old CI.
Motivation:
As an HTTP library, async-http-client should have authentication
support.
Modifications:
This adds a `setBasicAuth()` method to both HTTPClientRequest and
`HTTPClient.Request` and their related unit tests.
Result:
Library users will be able to leverage this method to use basic
authentication on their requests without implementing this in their own
projects.
Note: I also ran the tests (`swift test`) with the
`docker.io/library/swift:6.0-focal` and
`docker.io/library/swift:5.10.1-focal` to ensure linux compatibility.
Signed-off-by: Agam Dua <agam_dua@apple.com>
### Motivation:
- The properties that store the request body length and the cumulative number of bytes sent as part of a request are of type `Int`.
- On 32-bit devices, when sending requests larger than `Int32.max`, these properties overflow and cause a crash.
- To solve this problem, the properties should use the explicit `Int64` type.
### Modifications:
- Changed the type of the `known` field of the `RequestBodyLength` enum to `Int64`.
- Changed the type of `expectedBodyLength` and `sentBodyBytes` in `HTTPRequestStateMachine` to `Int64?` and `Int64` respectively.
- Deprecated the `public var length: Int?` property of `HTTPClient.Body` and backed it with a new property: `contentLength: Int64?`
- Added a new initializer and "overloaded" the `stream` function in `HTTPClient.Body` to work with the new `contentLength` property.
- **Note:** The newly added `stream` function has different parameter names (`length` -> `contentLength` and `stream` -> `bodyStream`) to avoid ambiguity problems.
- Added a test case that streams a 3GB request -- verified this fails with the types of the properties set explicitly to `Int32`.
### Result:
- 32-bit devices can send requests larger than 2GB without integer overflow issues.
Motivation:
Now that Swift 5.9 is GM we should update the supported versions and
remove 5.6
Modifications:
* Update `Package.swift`
* Remove `#if swift(>=5.7)` guards
* Delete the 5.6 docker compose file and make a 5.10 one
* Update docs
Result:
Remove support for Swift 5.6, add 5.10
* ChunkedCollection
* Use swift-algorithms
* SwiftFormat
* test chunking
* add documentation
* SwiftFormat
* fix old swift versions
* fix older swift versions
second attempt
* fix old swift versions
third attempt
* fix old swift versions
fourth attempt
* update documentation
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
Motivation:
This change was proposed in issue
[#389](https://github.com/swift-server/async-http-client/issues/389).
Users writing their own `HttpClientResponseDelegate` implementation
might want to emit log messages to the `task`'s `logger`.
Modifications:
Changed the access level of `Task`'s `logger` property from `internal`
to `public`.
Result:
Users can access the `logger` property of a `Task` outside of the
`async-http-client`.
* 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>
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
* 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
* make `Scheme` a type
* introduce new Endpoint type
* use endpoint as storage in `HTTPClient.Request`
* fix merge conflicts
* rename Endpoint to DeconstructedURL
* swift-format
* make `DeconstructedURL` properties `var`'s
* move scheme into global namespace
- rename `useTLS` to `usesTLS` where posible without breaking public API
- only import Foundation.URL
* fix review comments
- Refactored code from `TaskHandler` into new method `HTTPClient.Request.createRequestHead` that creates an `HTTPRequestHead` and matching `RequestFramingMetadata`
- Added required property `requestFramingMetadata` to `HTTPExecutingRequest`
- Added property `requestFramingMetadata` to `RequestBag`
Co-authored-by: Cory Benfield <lukasa@apple.com>
- Adding four new protocols:
- `HTTPExecutingRequest` a protocol representing an HTTP task that is executed on a ChannelHandler.
- `HTTPScheduledRequest` a protocol representing an HTTP task that is scheduled for execution, but in a waiting state (example: Waiting for an idle connection in the connection pool).
- `HTTPRequestExecutor` a protocol that can be used from the `HTTPExecutingRequest` abstracting away functionality that will normally be implemented by a `ChannelHandler`
- `HTTPRequestScheduler` a protocol that can be used from the `HTTPScheduledRequest` abstracting away functionality that will normally be implemented by a `ConnectionPool`
- An implementation of the `HTTPExecutingTask` and `HTTPScheduledRequest` called `RequestBag`. It implements our current API using the new protocols
Co-authored-by: George Barnett <gbarnett@apple.com>
Co-authored-by: Cory Benfield <lukasa@apple.com>
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:
HTTPResponseAggregator attempts to build a single, complete response
object. This necessarily means it loads the entire response payload into
memory. It wants to provide this payload as a single contiguous buffer
of data, and it does so by aggregating the data into a single contiguous
buffer as it goes.
Because ByteBuffer does exponential reallocation, the cost of doing this
should be amortised constant-time, even though we do have to copy some
data sometimes. However, if this operation triggers a copy-on-write then
the operation will become quadratic. For large buffers this will rapidly
come to dominate the runtime.
Unfortunately in at least Swift 5.3 Swift cannot safely see that during
the body stanza the state variable is dead. Swift is not necessarily
wrong about this: there's a cross-module call to ByteBuffer.writeBuffer
in place and Swift cannot easily prove that that call will not lead to a
re-entrant access of the `HTTPResponseAggregator` object. For this
reason, during the call to `didReceiveBodyPart` there will be two copies
of the body buffer alive, and so the write will CoW.
This quadratic behaviour is a nasty performance trap that can become
highly apparent even at quite small body sizes.
Modifications:
While Swift can't prove that the `self.state` variable is dead, we can!
To that end, we temporarily set it to a different value that does not
store the buffer in question. This will force Swift to drop the ref on
the buffer, making it uniquely owned and avoiding the CoW.
Sadly, it's extremely difficult to test for "does not CoW", so this
patch does not currently come with any tests. I have experimentally
verified the behaviour.
Result:
No copy-on-write in the HTTPResponseAggregator during body aggregation.
Motivation:
When we stream request body, current implementation expects that body
will finish streaming _before_ we start to receive response body parts.
This is not correct, reponse body parts can start to arrive before we
finish sending the request.
Modifications:
- Simplifies state machine, we only case about request being fully sent
to prevent sending body parts after .end, but response state machine
is mostly ignored and correct flow will be handled by NIOHTTP1
pipeline
- Adds HTTPEchoHandler, that replies to each response body part
- Adds bi-directional streaming test
Result:
Closes#327
* fail if we get part when state is endOrError
* Prevent TaskHandler state change after `.endOrError`
Motivation:
Right now if task handler encounters an error, it changes state to
`.endOrError`. We gate on that state to make sure that we do not
process errors in the pipeline twice. Unfortunately, that state
can be reset when we upload body or receive response parts.
Modifications:
Adds state validation before state is updated to a new value
Adds a test
Result:
Fixes#297
Motivation:
TaskHandler unconditionally releases it's connection on error,
this can lead to double release. This issue actually indicates
a more general issue where handler continues to handle errors
even after its state is `.endOrError`. We need to fix this by
ignoring all subsequent errors.
Modifications:
1. Check state before calling out delegate and pool
2. Replace all error callouts with call to `errorCaught`
Result:
Fixes#294
* Added tests that ensure redirects to unix socket paths from a regular HTTP server are disallowed.
Motivation:
Currently, redirects to any supported URL scheme will always be allowed, despite code being in place to seemingly prevent it. See #230.
Modifications:
- Added a method to HTTPBin to redirect to the specified target.
- Added failing tests that perform redirects from a regular server to a socket-based server and vice versa.
Result:
Failing tests that show that the existing redirect checks were inadequate.
* Fixed an issue where redirects to socket path-based servers from any server was always allowed.
Motivation:
An arbitrary HTTP(S) server should not be able to trigger redirects, and thus activity, to a local socket-path based server, though the opposite may be a valid scenario. Currently, requests in either direction are allowed since the checks don't actually check the destination scheme.
Modifications:
- Refactored `hostSchemes`/`unixSchemes` to `hostRestrictedSchemes`/`allSupportedSchemes`, which better describes what they do.
- Refactored `Request.supports()` to `Request.supportsRedirects(to:)` since it is only used by Redirects now.
- Check the destination URL's scheme rather than the current URL's scheme when validating a redirect.
Result:
Closes#230
Co-authored-by: Artem Redkin <artem@redkin.me>
Greg Cotten
Johannes Weiss
Rick Newton-Rogers
Agam Dua
aryan-25
Rick Newton-Rogers
David Nadoba
Cory Benfield
Fabian Fett
Felix Schlegel
Karl
David Evans
Mads Odgaard
Artem Redkin
Tim Condon
Dimitri Bouniol