Motivation:
The tests shouldn't be making sendability violations.
Modifications:
Fix the warnings
Result:
- No warnings
- Strict concurrency is adopted!
Motivation:
Delegates can be passed from any thread and are executed on an arbitrary
event loop. That means they need to be Sendable. Rather than making them
all Sendable in one go, we'll do the larger ones separately.
Modifications:
- Make FileDownloadDelegate sendable
Result:
Safe to pass FileDownloadDelegate across isolation domains
Enable MemberImportVisibility check on all targets. Use a standard
string header and footer to bracket the new block for ease of updating
in the future with scripts.
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:
- 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.
* 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
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>
* 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
- 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`.
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.
* 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 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.
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.
Motivation:
AHC's tests were split in HTTPClientTests (which only use the public
API) and HTTPClientInternalTests (which can use `internal` API by using
`@testable`). At some point, the public API tests had `@testable` added
to the AHC import which breaks this idea.
Modification:
Restore the intent by removing `@testable` and moving the 2 tests that
needed it over.
Result:
Cleaner test suite.
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.
* Close idle pool connections
Motivation: Pooled connections should close at some point (see #168)
Changes:
- Add new poolingTimeout property to HTTPClient.Configuration, it's
default value is .seconds(60), it can be set to nil if one wishes to
disable this timeout.
- Add relevant unit test
Co-authored-by: Johannes Weiss <johannesweiss@apple.com>
* Remove activity assertion in prepareForClose
motivation: assertion on activity sometimes fails but it turns out it is overly restrictive
changes:
- remove assertion and accept any given activity in prepareForClose
* test closing connections whilst syncShutdown
Co-authored-by: Johannes Weiss <johannesweiss@apple.com>
Motivation:
I wasn't sure if we actually properly observed an idle connection
suddenly closing. Ideally, if an idle connection just goes away, we just
want to remove it from the connection pool (which is what happens).
Modification:
Add a test that verifies idle connections that get closed from the
server don't get reused in the pool.
Result:
Better test coverage for more complicated scenarios.
motivation: Better performance thanks to connection reuse
changes:
- Added a connection pool for HTTP/1.1
- All requests automatically use the connection pool
- Up to 8 parallel connections per (scheme, host, port)
- Multiple additional unit tests
Previously, UNIX Domain Sockets would only work if the URL also had a
"base URL". If it didn't have a base URL, it would try to connect to the
empty string which would fail :).
Now, we support both cases:
- URLs with a baseURL (the path to the UDS) and a path (actual path)
- URLs that just have an actual path (path to the UDS) where we'll just
use "/" as the URL's path
motivation: the Swift Server Workgroup is not a legal entity and cannot hold copyrights. with this change, code authors continue and retain their copyrights under the apache license and previous copyrights note, but Apple steps up instead of the workgroup which has no legal status
changes:
* update header files to say "Apple Inc. and the AsyncHTTPClient project authors" instead of "Swift Server Workgroup and the AsyncHTTPClient project authors"
* update validation scripts to check for the correct header
* add CONTRIBUTING.md file to explain how to make contributions and include a legal notice about licensing the contribution to Apple and the project
* regenerate CONTRIBUTORS.md to reflect most recent contributions
Motivation:
Right now, HTTPClient only asserts that it's shut down if it was started with its own EventLoopGroup.
That however is weird because it's lifecycle model depends on the parameters you pass to `init`.
Modifications:
Always validate the lifecycle (in debug mode).
Result:
API makes more sense.
* Ignore uncleanShutdown error when state is head or body
* Add ignoreNIOSSLUncleanShutdownError to Configuration
* Revert old HTTPClient.init founctions
* Run generate_linux_tests.rb
* Rename ignoreNIOSSLUncleanShutdownError to ignoreUncleanSSLShutdown
* Make tests compatible with swift 5.0
George Barnett
Rick Newton-Rogers
aryan-25
David Nadoba
Cory Benfield
Karl
Fabian Fett
Artem Redkin
Dimitri Bouniol
Johannes Weiss
Adam Fowler
Artem Redkin
Iain Smith
Trevör
tomer doron
vkill