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.
The Darwin module is slowly being split up, and as it gets further
along, it will stop importing some of the split-out modules like the one
for locale.h that provides newlocale() and other locale API. However,
there's a wrinkle that on platforms with xlocale, it's xlocale.h that
provides most of the POSIX locale.h functions and not locale.h, so
prefer the xlocale module when available.
Motivation:
We would like to make this work for Musl so that we can build fully
statically linked binaries that use AsyncHTTPClient.
Modifications:
Define `_GNU_SOURCE` as a compiler argument; doing it in a source file
doesn't work properly with modular headers.
Add imports of `Musl` in appropriate places.
`Musl` doesn't have `strptime_l`, so avoid using that.
Result:
async-http-client will build for Musl.
Motivation:
The above test has been seen to fail with port already in use.
The test assumes that a port can be bound to twice in a row.
It's possible that another process takes the port between the two
binds - this can be stopped by binding a second time before stopping
the first.
Modifications:
Allow port reuse and reorder the test to bind a second time before
releasing the first.
Result:
Test should no longer be flaky.
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
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
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
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
To land support for async/await, we need test utilities. We already have a `MockRequestExecutor`. Let's improve this to better handle on and off `EventLoop` request processing.
### Changes
- Move `MockRequestExecutor` into its own file
- Add blocking APIs to `MockRequestExecutor` when called from another thread
### 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>
- 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().
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:
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:
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
Motivation:
Currently when either we or the server send Connection: close, we
correctly do not return that connection to the pool. However, we rely on
the server actually performing the connection closure: we never call
close() ourselves. This is unnecessarily optimistic: a server may
absolutely fail to close this connection. To protect our own file
descriptors, we should make sure that any connection we do not return
the pool is closed.
Modifications:
If we think a connection is closing when we release it, we now call
close() on it defensively.
Result:
We no longer leak connections when the server fails to close them.
Fixes#324.
* Add FileDownloadDelegate for simple file downloads
* Add testFileDownload to the allTests array
* Fix formatting
* Fix compatibility with Swift 5.0
* Add doc comments, update README.md
* Refine FileDownloadDelegate description in README
* Bump NIO version, remove weak self, cleanup test
* Fix formatting issues in a doc comment
* Create separate Progress struct, async open file
* Create an ad-hoc EventLoopGroup for opening a file
* Move file opening code to `didReceiveBodyPart`
* Fix linter error in FileDownloadDelegate.swift
* Fix wrong future assignment in FileDownloadDelegate
* Fix Swift 5.0 return statement compatibility
* Fix linter warning
* Fix Swift 5.0 return statement compatibility
* Remove redundant `write` function
* Add negative test case and separate testing endpoint
* Add missing testFileDownloadError to the manifest
Motivation:
Build warnings fail our builds.
Modifications:
- Update minimum NIOSSL version
- Remove the use of the now-unnecessary `try` statement.
Result:
No warnings!
* 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>
* Added additional tests for socketPath-based requests
Motivation:
While going through the existing tests, I identified a few more instances where we could add some testing.
Modifications:
Added one test that verifies Requests are being decoded correctly, and improved three others to check for path parsing, error throwing, and schema casing respectively.
Result:
Tests that continue to pass, but that will also catch any incompatible changes in the future.
* Added some convenience initializers to URL and methods to Request for making requests to socket paths
Motivation:
Creating URLs for connecting to servers bound to socket paths currently requires some additional code to get exactly right. It would be nice to have convenience methods on both URL and Request to assist here.
Modifications:
- Refactored the get/post/patch/put/delete methods so they all call into a one line execute() method.
- Added variations on the above methods so they can be called with socket paths (both over HTTP and HTTPS).
- Added public convenience initializers to URL to support the above, and so socket path URLs can be easily created in other situations.
- Added unit tests for creating socket path URLs, and testing the new suite of convenience execute methods (that, er, test `HTTPMETHOD`s). (patch, put, and delete are now also tested as a result of these tests)
- Updated the read me with basic usage instructions.
Result:
New methods that allow for easily creating requests to socket paths, and passing tests to go with them.
* Removed some of the new public methods added for creating a socket-path based request
Motivation:
I previously added too much new public API that will most likely not be necessary, and can be better accessed using a generic execute method.
Modifications:
Removed the get/post/patch/put/delete methods that were specific to socket paths.
Result:
Less new public API.
* Renamed execute(url:) methods such that the HTTP method is the first argument in the parameter list
Motivation:
If these are intended to be general methods for building simple requests, then it makes sense to have the method be the first parameter in the list.
Modifications:
Moved the `method: HTTPMethod` parameter to the front of the list for all `execute([...] url: [...])` methods, and made it default to .GET. I also changed the url parameter to be `urlPath` for the two socketPath based execute methods.
Result:
A cleaner public interface for users of the API.
* Fixed some minor issues introduces with logging
Motivation:
Some of the convenience request methods weren't properly adapted for logging.
Modifications:
- Removed a doc comment from patch() that incorrectly referenced a logger.
- Fixed an issue where patch() would call into post().
- Added a doc comment to delete() that references the logger.
- Tests for the above come in the next commit...
Result:
Correct documentation and functionality for the patch() and delete() methods.
* Updated logging tests to also check the new execute methods
Motivation:
The logging tests previously didn't check for socket path-based requests.
Modifications:
Updated the `testAllMethodsLog()` and `testAllMethodsLog()` tests to include checks for each of the new `execute()` methods.
Result:
Two more tests that pass.
Motivation:
AsyncHTTPClient is not a simple piece of software and nowadays also
quite stateful. To debug issues, the user may want logging.
Modification:
Support passing a logger to the request methods.
Result:
Debugging simplified.
* Added tests for http+unix and https+unix url schemes
Motivation:
Using a base URL as the socket path only works when the URL object is maintained as long as possible through the stack. Additionally, it doesn't currently provide a way to use TLS over UNIX sockets.
Modifications:
Added two tests to test out the to-be supported URL schemes, http+unix, and https+unix, which encode the socket path as a %-escaped hostname, as some existing services already do.
Result:
Better UNIX domain socket support.
Motivation:
Previously, we'd only use the server's connection header to determine if
we should close the connection or not. That's wrong because if we set
`connection: close` ourselves, we must not reuse again.
Modification:
Set `TaskHandler.closing = false` if we send a close header.
Result:
More HTTP correctness.
This is the continuation of the good work of @Yasumoto and @weissi in #135
The following code adds support for NIO Transport services. When the ConnectionPool asks for a connection bootstrap it is returned a NIOClientTCPBootstrap which wraps either a NIOTSConnectionBootstrap or a ClientBootstrap depending on whether the EventLoop we are running on is NIOTSEventLoop.
If you initialize an HTTPClient with eventLoopGroupProvider set to .createNew then if you are running on iOS, macOS 10.14 or later it will provide a NIOTSEventLoopGroup instead of a EventLoopGroup.
Currently a number of tests are failing. 4 of these are related to the NIOSSLUncleanShutdown error the others all seem related to various race conditions which are being dealt with on other PRs. I have tested this code with aws-sdk-swift and it is working on both macOS and iOS.
Things look into:
The aws-sdk-swift NIOTS HTTP client had issues with on Mojave. We should check if this is the case for async-http-client as well.
Co-authored-by: Joe Smith <yasumoto7@gmail.com>
Co-authored-by: Johannes Weiss <johannesweiss@apple.com>
* Enable clients to call URLs that include %2F as an escaped backslash
Previously `percentEncodedPath` was using `path.addingPercentEncoding(withAllowedCharacters: .urlPathAllowed)`
which converts %2F to a literal '/'. This prevented users fetching URLs like https://api.travis-ci.org/repo/github/rails%2Frails
which use %2F as part of a path segment.
Migrating to `URLComponents(url: self, resolvingAgainstBaseURL: false)?.percentEncodedPath` has the desired behaviour
for the couple of test cases that exist.
Updated the test server to switch on the `percentEncodedPath` so it's easier
to understand the desired behaviour.
Rick Newton-Rogers
Ian Anderson
Gustavo Cairo
Alastair Houghton
Peter Adams
brenno
carolinacass
David Nadoba
Fabian Fett
Cory Benfield
Karl
David Evans
Artem Redkin
Max Desiatov
Dimitri Bouniol
Johannes Weiss
Artem Redkin
Adam Fowler
Iain Smith