- 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>
- The connection creation logic has been refactored into a number of smaller methods that can be combined
- Connection creation now has a logical home. It is moved from `Utils.swift` into a `ConnectionFactory`
- There are explicit `ChannelHandlers` that are used for connection creation:
- `TLSEventsHandler` got its own file and unit tests
- `HTTP1ProxyConnectHandler` got its own file and unit tests
- `SOCKSEventsHandler` got its own file and unit tests
- Some small things are already part of this pr that will get their context later. For example:
- `HTTPConnectionPool` is added as a namespace to not cause major renames in follow up PRs
- `HTTPConnectionPool.Connection.ID` and its generator were added now. (This will be used later to identify a connection during its lifetime)
- the file `HTTPConnectionPool+Manager` was added to give `HTTPConnectionPool.Connection.ID.Generator` already its final destination.
Updated:
NIO
NIOSSL
NIO Extras
NIOTS
Also fix TLSConfiguration.forClient() warnings by converting to TLSConfiguration.makeClientConfiguration(). Also the same for forServer().
Motivation:
At the moment, AHC assumes that creating a `NIOSSLContext` is both cheap
and doesn't block.
Neither of these two assumptions are true.
To create a `NIOSSLContext`, BoringSSL will have to read a lot of
certificates in the trust store (on disk) which require a lot of ASN1
parsing and much much more.
On my Ubuntu test machine, creating one `NIOSSLContext` is about 27,000
allocations!!! To make it worse, AHC allocates a fresh `NIOSSLContext`
for _every single connection_, whether HTTP or HTTPS. Yes, correct.
Modification:
- Cache NIOSSLContexts per TLSConfiguration in a LRU cache
- Don't get an NIOSSLContext for HTTP (plain text) connections
Result:
New connections should be _much_ faster in general assuming that you're
not using a different TLSConfiguration for every connection.
Motivation:
AsyncHTTPClient attempts to avoid the problem of Happy Eyeballs making
it hard to know which Channel will be returned by only inserting the
TLSEventsHandler upon completion of the connect promise. Unfortunately,
as this may involve event loop hops, there are some awkward timing
windows in play where the connect may complete before this handler gets
added.
We should remove that timing window by ensuring that all channels always
have this handler in place, and instead of trying to wait until we know
which Channel will win, we can find the TLSEventsHandler that belongs to
the winning channel after the fact.
Modifications:
- TLSEventsHandler no longer removes itself from the pipeline or throws
away its promise.
- makeHTTP1Channel now searches for the TLSEventsHandler from the
pipeline that was created and is also responsible for removing it.
- Better sanity checking that the proxy TLS case does not overlap with
the connection-level TLS case.
Results:
Further shrinking windows for pipeline management issues.
Motivation:
There is an awkward timing window in the TLSEventsHandler flow where it
is possible for the NIOSSLClientHandler to fail the handshake on
handlerAdded. If this happens, the TLSEventsHandler will not be in the
pipeline, and so the handshake failure error will be lost and we'll get
a generic one instead.
This window can be resolved without performance penalty if we use the
new synchronous pipeline operations view to add the two handlers
backwards. If this is done then we can ensure that the TLSEventsHandler
is always in the pipeline before the NIOSSLClientHandler, and so there
is no risk of event loss.
While I'm here, AHC does a lot of pipeline modification. This has led to
lengthy future chains with lots of event loop hops for no particularly
good reason. I've therefore replaced all pipeline operations with their
synchronous counterparts. All but one sequence was happening on the
correct event loop, and for the one that may not I've added a fast-path
dispatch that should tolerate being on the wrong one. The result is
cleaner, more linear code that also reduces the allocations and event
loop hops.
Modifications:
- Use synchronous pipeline operations everywhere
- Change the order of adding TLSEventsHandler and NIOSSLClientHandler
Result:
Faster, safer, fewer timing windows.
Motivation:
Right now documentation states that timrout defaults to no timeout, this
is no actually true, if timeout is not set NIO bootstrap defaults to 10
seconds connect timeout.
Modifications:
Updates documentation comment.
Result:
Closes#118
Motivation:
Right now we only handle one type of SSL error: `.handshakeFailed`,
but in reality a multitude of errors can happen, for example, remote
party might just close connection that will in turn raise
`.uncleanShutdown` error, that will be dropped on the floor and
users will only get non-descriptive `NoResult` error.
Modifications:
Handle all types of SSL errors during handshake instead of just one.
Adds a test.
Result:
Closes#313
Co-authored-by: Cory Benfield <lukasa@apple.com>
Motivation:
Streams length parameter is optional to allow cases were stream length is not known in advance, but we do not support this in request validation. This PR aims to address that.
Modifications:
Modifies request validation to default to chunked encoding if body length is zero or to passed in content-length header
Adds a test
Result:
Closes#218
`redirectConfiguration` can't default to `false` as it's not a boolean value, and the default value is `RedirectConfiguration()`.
Co-authored-by: Johannes Weiss <johannesweiss@apple.com>
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.
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>
motivation:
TLS handshake promise was leaked in some cases of failure (see #179)
changes:
- Avoid leaking promise
- Clearer completion flow for related futures
- Add testAvoidLeakingTLSHandshakeCompletionPromise test
* 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>
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
* Bugfix HTTPS SNI and IP Address
Motivation:
Solving the SNI Bug
Modifications:
Added an internal extension on String for checking if the hostname is an IP Address -- see the private extension on SNI. Additionally using the IPv4Address and IPv6Address Function from Network above 10.14 as protecting with #availabe.
Adding the test for HTTPS and IP in as hostname
Result:
We get results with an IP as Hostname
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.
* refactor proxy configuration
motivation: make proxy configuration follow same convention as other configuration
changes:
* nest Proxy under HTTPClient.Configuration instad of top level HTTPClient
* make host and port public and mutable, following convention of other configuration objects in this library
* add some API docs
* fixup
* add NIO event loop as an argument for execute
* review fix: add to np-delegate method as well
* Resolve confict
* add missing linux test
* fix formatting
* missing self
* review fix: add event loop preference argument instead of eventloop
* formatting
* review fix: spelling
* fix compilation error
* review fixes: make preference argument not explicit and add precondition that EL must be part of ELG
* 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
Fabian Fett
David Evans
Johannes Weiss
Cory Benfield
Artem Redkin
Adam Fowler
Max Desiatov
Dimitri Bouniol
Artem Redkin
Fabian Fett
Trevör
Marcin Krzyzanowski
Andreas Kostuch
tomer doron
vkill
Ludovic Dewailly