We have a number of copies of `UnsafeTransfer` and two copies of
`UnsafeMutableTransferBox` in our code base. Before introducing more of
those, lets centralize to just one using a `package` access modifier.
`NIOTypedHTTPClientUpgradeHandler.handlerAdded` will write upgrade
request if the channel is already active
### Motivation:
This has been added to make it easier to implement a websocket client
that supports proxies. If the upgrade handler is added after the proxy
connect has been processed previously nothing would happen.
### Modifications:
- Added new function
`NIOTypedHTTPClientUpgradeHandler.writeUpgradeRequest()` function. Call
this from `channelActive` and also from `handlerAdded` if the channel is
already active.
- Added test `testUpgradeHappensAfterHandlerAdded`
### Result:
NIOTypedHTTPClientUpgradeHandler now also works if it is added to a
channel after the channel is active
---------
Co-authored-by: Cory Benfield <lukasa@apple.com>
On filesystems without symlink support for Windows, the symlinked test
helper at is materialized as a plain text file containing the relative
path, which breaks compilation with errors like:
```
expressions are not allowed at the top level
cannot find 'assertNoThrowWithValue' in scope
```
This change inlines the contents, restoring the intended shared test
utilities for the NIOHTTP1 test target. This helps Windows compile the
Test target
Motivation:
NIOHTTPResponseHeadersValidator drops head/end response parts if they
contain invalid header fields. If a head part is dropped and the server
allows pipelining then the subsequent body/end part will reach the
pipelining handler and result in an assertion failure in debug builds.
In release builds, some parts may be incorrectly written out leading to
protocol violations.
Modifications:
- Drop all response parts after an invalid response part
Result:
- Fewer bugs
- Resolves#3326
Co-authored-by: Cory Benfield <lukasa@apple.com>
Motivation
With the introduction of isolated conformances, it has become necessary
to start managing the use of metatypes for some of our protocols. In
general, we don't want to force the relevant protocols to only be
conformed in non-isolated forms. Instead, we just want to make the
specific APIs non-usable.
Modifications
- Add shims for SendableMetatype that only use it when it is available.
- Require SendableMetatype where needed, gated by @preconcurrency.
Result
We continue to be safe.
Motivation:
Swift 5.9 is no longer supported, we should bump the tools version and
remove it from our CI.
Modifications:
* Bump the Swift tools version to Swift 5.10
* Remove Swift 5.9 jobs where appropriate in main.yml, pull_request.yml
Result:
Code reflects our support window.
### Motivation
C++ interop support in Xcode has started to dislike the header files
from llhttp. While a bug has been filed upstream at
https://github.com/nodejs/llparse/issues/71 it doesn't seem as though
there is any movement on that fix upstream. As a result, we need to
apply a patch.
To keep the code maintainable, we'd like to apply that patch in an
automated fashion, so we should do so by amending the "update and patch"
script. Along the way, we should update LLHTTP to the latest code.
### Modifications
The modifications are split into separate commits.
1. Modify the script to fix some bugs that have crept in.
2. Run the script to update LLHTTP to latest, without the patch.
3. Fix up the tests to account for the fact that we get some different
errors from LLHTTP now.
4. Apply the patch suggested by @postmechanical in #3093
5. Re-run the script, which only applies the patch
### Result
C++ interop should be working again. Resolves#3093.
---------
Co-authored-by: George Barnett <gbarnett@apple.com>
To handle errors fired by previous ChannelHandlers. eg
NIOSSLClientHandler
### Motivation:
Otherwise if a ChannelHandler previous to the upgrade ChannelHandler
fails the error returned is `inappropriateOperationForState` and the
error fired by the original ChannelHandler is lost.
Added `testReturnsErrorsFromPriorChannelHandlers`. Also had to
reorganise `setUpClientChannel` slightly so I could get the result of
the upgrade.
---------
Co-authored-by: Cory Benfield <lukasa@apple.com>
### Motivation:
To ensure NIOHTTP1 concurrency safety.
### Modifications:
* Enable strict concurrency checking in the package manifest.
* Mark several objects `Sendable` with `@preconcurrency` annotations
where they are returned in futures which may execute in arbitrary
concurrency domains.
* `NIOTypedHTTPClientProtocolUpgrader`
* `NIOTypedHTTPClientUpgradeConfiguration`
* `NIOUpgradableHTTPServerPipelineConfiguration`
* `NIOUpgradableHTTPClientPipelineConfiguration`
* `NIOTypedHTTPServerProtocolUpgrader`
* `NIOTypedHTTPServerUpgradeConfiguration`
* Mark handlers as explicitly not sendable
* `NIOTypedHTTPClientUpgradeHandler`
* `NIOTypedHTTPServerUpgradeHandler`
* Added new Sendable type aliases:
* `NIOHTTPClientUpgradeSendableConfiguration`
* `NIOHTTPServerUpgradeSendableConfiguration`
### Result:
No more concurrency warnings. Builds will warn and CI will fail if
regressions are introduced.
### Motivation:
As outlined in this
[issue](https://github.com/apple/swift-nio/issues/2930) by @weissi, the
current performance of calling `description` on `HTTPHeaders` is
undermined by the dynamism of Array.
### Modifications:
The proposed solution replaces the implementation of `description` to
iterate over the items and print them out manually. This provides a
faster solution since we bypass the cost of calling into `description`
of an Array.
### Result:
A more performant implementation of `HTTPHeaders` description.
---------
Co-authored-by: Johannes Weiss <johannesweiss@apple.com>
Co-authored-by: Cory Benfield <lukasa@apple.com>
Motivation:
6.0.3 added new warnings about Sendable issues, so let's fix those to
keep things warnings free.
Modifications:
A bunch of Sendable usage fixes.
Result:
Warnings free builds again!
### Motivation:
Opening the `swift-nio` repository made me warning blind because there
were always so many trivially fixable warnings about things that were
correct but cannot be understood by the compiler.
### Modifications:
Fix all the sendable warnings that popped up, except for one test where
`NIOLockedValueBox<Thread?>` isn't sendable because `Foundation.Thread`
seemingly isn't `Sendable` which is odd. Guessing that'll be fixed on
their end.
### Result:
- Fewer warnings
- Less warning-blindness
- More checks
Motivation:
`IOData` is a legacy but alas also core type that needs to be
`Sendable`. Before this PR however it can't be `Sendable` because it
holds a `FileRegion` which holds a `NIOFileDescriptor`. So let's make
all of these `Sendable` but let's also start the deprecation journey for
the following types:
- `IOData`, now soft-deprecated (no warnings) because on its reliance on
`FileRegion`
- `FileRegion`, now soft-deprecated (no warnings) because on its
reliance on `NIOFileHandle`
- `NIOFileHandle`, now soft-deprecated (warnings on the
`NIOFileHandle(descriptor:)` constructor but with a
`NIOFileHandle(_deprecatedTakingOwnershipOfDescriptor:)` alternative
- `NonBlockingFileIO`, now soft-deprecated (warnings on the `openFile`
functions (but with `_deprecated` alternatives) because of their
reliance on `NIOFileHandle)
Modification:
- Make `NIOFileDescriptor`, `FileRegion` and `IOData` `Sendable` by
tracking the fd number and the usage state in an atomic
- Enforce singular access by making the `withFileDescriptor { fd ... }`
function atomically exchange the fd number for a "I'm busy" sentinel
value
- Start deprecating `IOData`, `NIOFileHandle`, `NonBlockingFileIO`,
`FileRegion`
Result:
- `NIOFileDescriptor`, `FileRegion` and `IOData` can be `Sendable`
Check for half closure during server upgrade and close channel if client
closes the channel
### Motivation:
This is to fix#2742
### Modifications:
Add `userInboundEventTriggered` function to
`NIOTypedHTTPServerProtocolUpgrader` which checks for
`ChannelEvent.inputClosed`
### Result:
Negotiation future now errors when client closes the connection instead
of never completing
---------
Co-authored-by: Franz Busch <privat@franz-busch.de>
Motivation:
The ChannelInvoker protocols are an awkward beast. They aren't really
something that people can do generic programming against. Instead, they
were designed to do API sharing. Of course, they didn't do that very
well, and the strict concurrency checking world has revealed this.
Much of the API surface on ChannelInvoker is confused. There are
NIOAnys, which aren't Sendable. We allow sending user events without
requiring Sendable. And our two main conforming types are
ChannelPipeline and ChannelHandlerContext, two types with wildly
differing thread-safety semantics.
This PR aims to clean that up.
Modifications:
- Deprecated all API surface on ChannelInvoker protocols that uses
NIOAny.
ChannelInvoker has to be assumed to be a cross-thread protocol,
and that requires that it only use Sendable types. NIOAny isn't,
so these methods are no longer sound.
- Re-add non-deprecated versions on ChannelHandlerContext.
While it's not safe to use the NIOAny methods on Channel or
ChannelPipeline, it's totally safe to use them on
ChannelHandlerContext. So we keep those available and
undeprecated.
- Provide typed generic replacements on ChannelPipeline and on Channel
To replace the NIOAny methods on ChannelPipeline and Channel
we can use some typed generic ones instead. These are not
defined on ChannelInvoker, as the methods are useless on
ChannelHandlerContext. This begins the acknowledgement that
ChannelHandlerContext should not have conformed to these
protocols at all.
- Add Sendable constraints to the user event witnesses on ChannelInvoker
Again, these were missing, but must be there for Channel and
ChannelPipeline.
- Provide non-Sendable overloads on ChannelHandlerContext
ChannelHandlerContext is thread-bound, and so may safely pass
non-Sendable user events.
Result:
One step closer to strict concurrency cleanliness for NIOCore.
Motivation:
RemovableChannelHandlers have a large API surface in NIOCore. That API
surface is a bit awkward with regard to strict concurrency, and needs
some cleanup.
Modifications:
This patch adds some new API that is necessary to safely work with
RemovableChannelHandlers, deprecates some API that cannot plausibly be
used, and cleans up some other parts of the API.
Result:
Easier to work with RemovableChannelHandlers
Motivation:
Since Swift 5.5 and [SE-0299](https://github.com/swiftlang/swift-evolution/blob/main/proposals/0299-extend-generic-static-member-lookup.md) it is possible to add static members to protocols which are discoverable through the shorthand dot syntax.
This change reduces type repetition and improves call-site legibility.
Modifications:
Added extensions for ChannelOption with static members where Self is bound to a concrete type.
Result:
ChannelOption types can be used with the leading dot syntax. For eg:
```
//before
.channelOption(ChannelOptions.explicitCongestionNotification, value: true)
.channelOption(ChannelOptions.socketOption(.so_reuseaddr), value: 1)
//after
.channelOption(.explicitCongestionNotification, value: true)
.channelOption(.socketOption(.so_reuseaddr), value: 1)
```
* Apply formatting
* Apply no block comments rule
* Apply OmitExplicitReturns
* Apple OnlyOneTrailingClosureArgument
* Apply NoAssignmentInExpressions
* Fix up DontRepeatTypeInStaticProperties lint errors
* Apply `OrderedImports`
* Apply `ReplaceForEachWithForLoop`
* format file
* Enable the formatting pipeline
* Adopt `AmbiguousTrailingClosureOverload`
* Fix license header
* Fix format check
* Fix `EndOfLineComment`
* Fix CI
* Adapt CI script to check if changes when running formatting
* Separate lint and format into to steps
* Fix format
* Adopt `UseEarlyExits`
* Revert "Adopt `UseEarlyExits`"
This reverts commit d1ac5bbe12.
* Fixed an issue where HTTP/2 connections would disconnect if response compression were used and a 304 Not Modified response was returned
* Added HTTP encoder tests for 304 Not Modified responses
# Motivation
Adding a handler via the regular `pipeline.addHandler` APIs can lead to transferring the handler over isolation regions. To avoid running into `Sendable` warnings here we can use the `syncOperations` of the pipeline to avoid transferring the handlers.
# Modification
This PR is mostly changing code to avoid transferring handlers to and from the event loops in tests. This should be a no-op mostly.
Co-authored-by: Cory Benfield <lukasa@apple.com>
* Revert "Back out new typed HTTP protocol upgrader (#2579)"
# Motivation
We have reverted the typed HTTP protocol upgrader pieces since adopters were running into a compiler bug (https://github.com/apple/swift/pull/69459) that caused the compiler to emit strong references to `swift_getExtendedExistentialTypeMetadata`. The problem is that `swift_getExtendedExistentialTypeMetadata` is not available on older runtimes before constrained existentials have been introduced. This caused adopters to run into runtime crashes when loading any library compiled with this NIO code.
# Modifications
This PR reverts the revert and guard all new code in a compiler guard that checks that we are either on non-Darwin platforms or on a new enough Swift compiler that contains the fix.
# Result
We can offer the typed HTTP upgrade code to our adopters again.
* Add compiler guards
# Motivation
We got reports in https://github.com/apple/swift-nio/issues/2574 that our new typed HTTP upgrader are hitting a Swift compiler bug which manifests in a runtime crash on older iOS/macOS/etc.
# Modification
This PR backs out the new typed HTTP protocol upgrader APIs so that we can unblock our users until the Swift compiler bug is fixed.
# Result
No more crashes for our users.
* Avoid terminating when a precondition is not met in HTTPServerPipelineHandler
* Address review comments
* rename precondition to assertion
---------
Co-authored-by: Cory Benfield <lukasa@apple.com>
# Motivation
Over the past months, we have been working on new async bridges to make using NIO's `Channel` from Swift Concurrency possible. Since this work was far reaching we have opted to land all of it as SPI. Now the time has come and we feel confident enough to make the SPI official API. This comes after testing the new APIs in various scenarios such as HTTP 1&2, HTTP upgrades, protocol negotiation and in benchmarks.
# Modification
This PR removes the SPI from the `NIOAsyncChannel`, the bootstrap methods, protocol negotiation and HTTP upgrade.
# Result
Everyone can use the our new APIs🚀
# Motivation
This test has been flaky since we only waited for the untyped handler to be removed but we also execute this test for the typed test subclass.
# Modification
This PR fixes the test by also waiting for the typed handler to no longer be present in the pipeline.
# Result
Less flaky tests.
* Introduce new typed `HTTPClientUpgrader` and `WebSocketClientUpgrader`
# Motivation
In my previous PR https://github.com/apple/swift-nio/pull/2517, I added a new typed `HTTPServerUpgrader` and corresponding implementation for the `WebSocketServerUpgrader`. The goal of those is to carry type information across HTTP upgrades which allows us to build fully typed pipelines.
# Modification
This PR adds a few things:
1. A new `NIOTypedHttpClientUpgradeHandler` + `NIOTypedHttpClientProtocolUpgrader`. I also moved the state handling to a separate state machine. Similar to the server PR I did not unify the state machine between the newly typed and untyped upgrade handlers since they differ in logic.
2. A new `NIOTypedWebSocketClientUpgrader`
3. An overhauled WebSocket client example.
# Result
This is the last missing piece of dynamic pipeline changing where we did not carry around the type information. After this PR lands, we can finalize the `AsyncChannel` and async typed NIO pieces.
* Remove availability on the protocols
* Tolerate empty HTTP response body parts
### Motivation
Empty HTTP response body parts currently eagerly end a response. This is unexpected and against NIOs documented behaviour.
### Modification
- add test which send an empty response body part and would previously fail
- skip empty response body parts
### Result
Users can send empty body parts without ending the response.
* add comment explaining why we do it here instead of fixing it at the source
* move empty body part logic to `writeChunk`
* Introduce new typed `HTTPServerUpgrader` and `WebSocketServerUpgrader`
# Motivation
With our new `NIOAsyncChannel` and typed bootstrap APIs we want to be able to let users spell out their pipeline in a typed way. Pipelines can contain handlers that have to make a forking decision such as HTTP upgrading. Our current `HTTPServerUpgradeHandler` is one of those handlers but it lacks strict typing. To interact nicely with our new typed APIs we need to have a new variant of the `HTTPServerUpgradeHandler` that can carry type information.
# Modification
This PR adds a few things:
1. A new `NIOTypedHTTPServerUpgradeHandler` + `NIOTypedHTTPServeProtocolUpgrader`. I also moved the state handling logic to a separate state machine. I thought about unifying the state machines of the _old_ handler and the new one but they differ in behaviour which makes the state machine more complicated.
2. A new `NIOTypedWebSocketServerUpgrader` that conforms to `NIOTypedHTTPServerProtocolUpgrader`
3. An overhauled WebSocket server example that fully uses Concurrency.
# Result
We now have a way to fully type the server side of HTTP protocol upgrading.
Code review
Update parameter names for new API and fix example
* Introduce new configuration struct and rename to `UpgradablePipeline`
* Review comments
* Bump minimum Swift version to 5.7
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 integration test script
* Update docs
Result:
Remove support for Swift 5.6, add 5.10
* fix indentation issues
* 5.9 docker image use release image
Motivation:
Right now we always set the framing headers in HTTP requests and
responses that we send. This is a good practice for most users, but
it can cause issues, most notably in responses to CONNECT requests
which requires that we do not set framing headers.
Unfortunately, in NIO's current HTTP/1.1 design it is not possible
for us to suppress these headers. This is because the HTTP encoders
come _earlier_ in the pipeline than the decoders, so the HTTP
encoders do not know structurally what requests they are responding
to.
While we could merge the encoders/decoders, that's a fairly
substantial change. A better short-term change is to offer users
the ability to turn off the NIO header manipulation feature. In
this circumstance, users take on full responsibility for
appropriately framing their HTTP messages.
Modifications
- Add config to HTTPRequestEncoder and HTTPResponseEncoder.
- Plumb this config through.
- Add a bunch of tests.
Result
Users have way more control of HTTP/1.1 messages.
Motivation
We shouldn't crash on somewhat likely user error.
Modifications
Pass on writes after close(mode: .output) instead of crashing.
Result
User code is more robust to weird edge cases.
Motivation:
Currently the server pipeline handler ignores close. This generally works,
except in the rare case that the user calls close(mode: .output). In this
instance they have signalled that they'll never write again, and they're
likely expecting a final close shortly after.
However, it is possible that the pipeline handler has suspended reads
at the same time. On Linux this isn't an issue, because we'll still be told
about the eventual socket close. However, on Apple platforms we won't: we've
masked off the reads, and we can't listen to EVFILT_EXCEPT due to some
other issues. This means that on Apple platforms the server pipeline handler
can accidentally wedge the Channel open and prevent it from closing.
We should take this opportunity to have the server pipeline handler be smart
about close(mode: .output). What _should_ happen here is that the pipeline
handler should immediately refuse to deliver further requests on the Channel.
If one is in-flight, it can continue, but everything else should be dropped.
This is because the server cannot possibly respond to further requests.
Modifications:
- Add new states to the server pipeline handler
- Drop buffered requests and new data after close(mode: .output)
- Add tests
Result:
Server pipeline handler behaves way better.
Motivation:
#fileID introduced in Swift 5.3, so no longer need to use #file anywhere
Modifications:
Changed #file to #filePath or #fileID depending on the situation
Motivation:
The `HTTPServerUpgradeHandler` removes itself from the pipeline after an
upgrade and unbuffers any unconsumed reads. If an upgrade starts but
does not complete successfully then the pipeline may be left in an
unknown state.
If, for example, the failure occurs before the user provided upgrade
handler is run then unbuffered writes may be unwrapped by the wrong
channel handler as the wrong type leading to a crash.
Modifications:
- Only remove the upgrade handler and forward buffered writes if all
parts of the upgrade complete successfully.
- If part of the upgrade fails then fire an error into the channel
pipeline without removing the server upgrade handler.
- Remove a few unnecessary `map`s.
- Make `httpEncoder` non-optional since and remove associated dead code
since it can never be `nil`.
Result:
Upgrade handling is safer.
Motivation
When we receive a HEAD response, it's possible that the response
contains a content-length. llhttp has a bug
(https://github.com/nodejs/llhttp/issues/202) that prevents it from
properly managing that issue, which causes us to incorrectly parse
responses.
Modifications
Forcibly set llhttp's content-length value to 0.
Result
Correctly handle HTTP framing around llhttp's issues.
Motivation
HTTP headers are prevented from containing certain characters that can
potentially affect parsing or interpretation. Inadequately policing this
can lead to vulnerabilities in web applications, most notably HTTP
Response Splitting.
NIO was insufficiently policing the correctness of the header fields we
emit in HTTP/1.1. We've therefore added a new handler that is
automatically added to channel pipelines that will police the validity
of header fields.
For projects that are already running the validation themselves, this
can be easily disabled. Note that by default NIO does not validate
content length is correctly calculated, so applications can have their
framing fall out of sync unless they appropriately calculate this
themselves or use chunked transfer encoding.
Modifications
- Add thorough unit testing to confirm we will not emit invalid header
fields.
- Error if a user attempts to send an invalid header field.
Result
NIO applications are no longer vulnerable to response splitting by CRLF
injection by default.
Motivation:
We should better tolerate LLHTTP status codes we don't yet know about.
Modifications:
- Added support for the status codes that currently exist
- Add a fallback to the RAW case for the future.
Result:
Better management of LLHTTP status codes
Motivation:
The node.js HTTP parser library that we use has been unmaintained for some time. We should move to the maintained replacement, which is llhttp. This patch will update our dependency and bring us over to the new library, as well as make any changes we need.
Modifications:
This patch comes in 4 parts, each contained in a separate commit in the PR.
The first commit drops the existing http_parser code and updates some of the repo state for using llhttp.
The second commit rewrites the update script to bring in llhttp instead of http_parser.
The third runs the actual script. You can skip reviewing this except to sanity check the outcome.
The fourth commit updates the NIO code and the tests to get everything working.
In general the substance of the product modifications was minimal. The logic around keeping track of where we are in the buffer and how upgrades work has changed a bit, so that required some fiddling. I also had to add an error reporting path for the delegates to be able to throw specific errors that llhttp no longer checks for. Finally, I removed two tests that were a little overzealous and that llhttp does not police.
Result:
Back on the supported path.
Workaround for https://github.com/apple/swift/issues/59911
```swift
/src/Tests/NIOHTTP1Tests/HTTPTest.swift:148:21: warning: 'buf' mutated after capture by sendable closure
buf.writeString("\(c)")
```
The variable `buf` is mutated but only before being capture by the sendable closure.
All other warnings/errors are fixed with the latest Swift 5.7 toolchain
Fabian Fett
Adam FowlerandCory Benfield
Zamderax
George BarnettandCory Benfield
Max Desiatov
Rick Newton-Rogers
Ayush Garg
Franz Busch
Dimitri Bouniol
hamzahrmalik
hamzahrmalik
Rick Newton-RogersandFranz Busch
David Nadoba
carolinacass
