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.
### Motivation:
Channels based on `BaseSocketChannel` throw in both `getOption` and
`setOption` if the channel has been closed, since the `setsockopt` will
fail. However, the current behavior of `EmbeddedChannel` is options
remain writable and readable on closed channels.
There are situations where we'd like to be able to model the runtime
behaviour of the real channel in tests, e.g. to test this fix:
https://github.com/apple/swift-nio-extras/pull/304.
### Modifications:
- Add API to enable throwing in `EmbeddedChannel.getOption` and
`.setOption` if channel is closed.
- Add a test for this new behavior.
### Result:
- New API to enable throwing in `EmbeddedChannel.getOption` and
`.setOption` if channel is closed.
- No observable change for existing users of `EmbeddedChannel`.
### Motivation:
From
https://github.com/apple/swift-nio/actions/runs/20197120190/job/57982705638
onwards, the Integration Test benchmark runs have been failing for
`Linux (nightly-main)` due to allocations for seven tests falling below
the defined thresholds.
### Modifications:
Updated thresholds for the tests where the number of allocations were
out-of-threshold (seven in total).
### Result:
The Integration Test run for `Linux (nightly-main)` no longer fails.
### Motivation:
`NIOAsyncTestingChannel` stored its `localAddress` and `remoteAddress`
in a locked storage on itself for thread safety, however in doing so
left us open to bugs because a handler grabbing the addresses of the
context had no visibility of the values.
### Modifications:
Reach into `EmbeddedChannelCore` for the addresses instead of storing
them on the `NIOAsyncTestinghannel`. I also considered a delegate
approach where the `EmbeddedChannelCore` could offload the
responsibility for storing the values back to the
`NIOAsyncTestingChannel` but it was complicated and of questionable
value.
### Result:
* The correct address values are seen no matter how they are obtained.
* We probably take a performance hit locking the values in this way but
this is testing code so probably not the end of the world.
Motivation:
The compiler is better at optimising closures wrapped in nominal types
than raw closures when used as generic parameters. CallbackList is used
a lot and stores an optional closure as well as on optional array of
closures. Wrapping these internally should save some allocations.
Modifications:
- Wrap the element stored in the `CallbackList`.
Result:
Fewer allocs
---------
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.
Add and enable Swift 6.1 workflows
### Motivation:
Swift 6.1 has been released, we should add it to our CI coverage.
### Modifications:
* Update `appe/swift-nio/scripts/generate_matrix.sh`
* Update reusable adopters of `swift_test_matrix.yml`
* Update end-user adopter workflows of `swift_test_matrix.yml`
* Copy over test flags from Swift 6.0 jobs
### Result:
NIO tests against Swift 6.1 in CI and downstream repositories can
opt-in.
(Successful CI run with the workflows modified to use the changes on
this branch
https://github.com/apple/swift-nio/actions/runs/14400598096?pr=3196)
Following on from https://github.com/apple/swift-nio/pull/3126 delete
`Benchmarks/Thresholds/nightly-6.1` and
`IntegrationTests/tests_04_performance/Thresholds/nightly-6.1.json`
which is no longer needed now that the shared benchmarks workflow has
been updated.
Use nightly_next as swift version
see https://github.com/apple/swift-nio/pull/3122
Motivation:
To not have to rename threshold directories when the nightly branch
changes.
Modifications:
* Use nightly_next as swift version in the matrix generation script
which
is picked up by the benchmark script.
* Move nightly-next thresholds and add legacy symlink
Result:
Benchmark thresholds will attempt to find directories named
nightly_next, not nightly_6_1.
Motivation:
As pointed out in #3071, the `flatScheduleTask` implementations can be
improved.
Modifications:
- Refactor the `flatScheduleTask` implementations to skip `flatMap`
calls, which avoids creating an extra promise.
- As there is now a lower number of allocations, reduce the necessary
thresholds for the allocation tests.
Result:
Reduction in the number of allocations in the package.
---------
Co-authored-by: George Barnett <gbarnett@apple.com>
### Motivation:
Some changes were missed in #3076, passing through parameters for the
new 6.1 version.
### Modifications:
Pass through more parameters, clean up misleading comments.
### Result:
6.1 nightly runs will be more like 6.0 nightly runs were
Motivation:
Users of SelectableEventLoop deserve to have their code that uses the
isolated event loop view perform at least as well as the code that
doesn't.
Modifications:
- Adopt the extra protocol witnesses.
- Refactor out common code.
Result:
Better performance
---------
Co-authored-by: George Barnett <gbarnett@apple.com>
Motivation
Right now the isolated view on the EventLoop has to implement its
interface in terms of UnsafeTransfer, because there are no non-Sendable
operations it can use.
That hurts us because we have to allocate an extra closure for all these
operations, making them slower than they need to be. This is unavoidable
in some contexts, but most EventLoops would be able to offer a fast-path
that avoids this extra allocation.
To achieve that, we need to move this logic into a customisation point
on the EventLoop protocol. Our fallback default implementation can be
the same as we do today.
Modifications
- Add customisation points for EventLoop for each of its operations that
has a protocol witness already.
- Add default implementations of these, using the implementation from
EventLoop.Isolated.
Results
No behavioural changes, but the code has moved.
---------
Co-authored-by: George Barnett <gbarnett@apple.com>
Motivation:
As with the ELF operations before them, the isolated EL operations
currently incur overhead above-and-beyond the overhead of their
non-isolated counterparts. That's not what we want to see. However,
before we "fix" them, we need to add regression testing to confirm our
fix actually worked.
Modifications:
- Add isolated variations of all current EL scheduling tests.
- Where isolated methods didn't have alloc counter tests, add new ones
for the non-isolated versions too.
Result:
More tests.
### Motivation:
The bootstraps are the next most obvious target for cleaning up
Sendability issues. These issues are mostly just missing annotations,
but there are a few places where we had actual latent threading bugs
that were missed. A lot more of the control flow is made more explicit
in this patch, and in general it should get a lot easier to be confident
that the code is correct.
### Modifications:
- Add necessary `@Sendable` annotations
- Clean up some incorrect `self` captures of bootstraps, which are not
`Sendable`, and which could lead to real threading bugs
- Make a few methods `static` to avoid needing to capture `self` at all.
- Make a few threading assumptions clear by using the isolated views or
the sync operations
- Add some missing `Sendable` constraints on interior generic functions.
### Result:
Better safety, better correctness in the bootstraps.
Motivation:
Unfortunately, closure composition is really expensive: closures that
capture closures always heap allocate. To make ELF.Isolated perform
well, then, we need to inline the method bodies directly.
Modifications:
- Add some isolated functions into ELF for enqueueing callbacks.
- Inline the implementation of the ELF methods into the isolated view.
Result:
Allocation counts match between isolated/nonisolated.
Motivation:
We've added isolated views onto EventLoopFutures. These are great, but
as initially implemented they add a bunch of overhead to the nonisolated
versions. That's not optimal, but before we change the code we should
make sure we have a benchmark baseline.
Modifications:
- Extend the test_future_lots_of_callbacks alloc test to add all the
callbacks that the isolated views have.
- Duplicate that test to one that adds isolated wrappers so we can
compare.
Result:
Two regression tests whose allocation counts should be equal, but
aren't.
Motivation:
NIOEmbedded is used all over NIO-land for testing various pieces of the
infrastructure, and so requires a substantial audit for strict
concurrency.
Modifications:
- Mark a few things Sendable.
- Fix the tests, which actually did have some nasty bugs
Result:
Sendable-clean NIOEmbedded
Motivation:
When converting an Array that holds existentials, it is necessary for
Swift to allocate a new Array and copy the elements into it, so that it
can fix up their existential boxes. We forced this to happen with taking
a `[ChannelHandler]` to `[ChannelHandler & Sendable]`, without rewriting
the other functions.
The result of that was that the other functions needed to convert the
channel handler types, causing extra allocations in this path.
Modifications:
Propagate the constraint down to the point where we iterate the Array.
Result:
Allocations reduced.
Motivation:
Unfortunately the current script has a bug meaning it can never fail.
Modifications:
Change bracketing for output search.
Result:
Allocation tests will flag when failing
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)
```
# Motivation
We have quite a lot of shell scripts in our repo and want to make sure that they all pass `shellcheck`.
# Modification
This PR adds a GH action workflow to the soundness script for `shellcheck` and fixes up all errors and warnings.
# Result
No more shell/bash discussions
### Motivation:
The two testing event loops—`EmbeddedEventLoop` and `NIOAsyncTestingEventLoop`—have different semantics for outstanding work during shutdown, which are both different from the production `SelectableEventLoop`, specifically with newly scheduled tasks that result from running existing scheduled work at the time of shutdown.
There are three axes to consider:
1. Scheduled tasks that are at or past their deadline.
2. Scheduled tasks with a deadline in the future.
3. Newly scheduled work that result from either running or cancelling (1) and (2).
| | (1) | (2) | (3) |
|-|-|-|-|
| `SelectableEventLoop` | Run | Cancel | Quiesce over 1000 ticks, repeatedly run resulting (1) and cancel resulting (2) |
| `EmbeddedEventLoop` | Run | Cancel | Cancel resulting (1) and resulting (2) |
| `NIOAsyncTestingEventLoop` | Run | Run | Run resulting (1) and resulting (2) |
Note that `NIOAsyncTestingEventLoop` may never terminate because of this.
### Modifications:
This PR aligns `EmbeddedEventLoop` and `NIOTestingEventLoop` and makes them more similar to `SelectableEventLoop` and less surprising semantics.
### Result:
| | (1) | (2) | (3) |
|-|-|-|-|
| `SelectableEventLoop` | Run | Cancel | Quiesce over 1000 ticks, repeatedly run resulting (1) and cancel resulting (2) |
| `EmbeddedEventLoop` | Run | Cancel | Cancel resulting (1) and resulting (2) |
| `NIOAsyncTestingEventLoop` | Run | Cancel | Cancel resulting (1) and resulting (2) |
### Future work:
Given both the `EmbeddedEventLoop` and `NIOAsyncTestingEventLoop` are used in tests of NIO applications that run with `SelectableEventLoop` in production, it might be worth extending both to also support quiescing to give a more representative shutdown behaviour in tests.
* 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.
Motivation:
The Swift 6 compiler emits a number of warnings about sendability which
we haven't dealt with yet. The CI failing on warnings may mask other
compilation issues or test failures.
Modifications:
- Disable warnings as errors on Swift 6 and main
Result:
Swift 6 and main CI should pass
# Motivation
Another reusable check is to make sure that all library products of a package are successfully building when consumed from a module that has Cxx interoperability enabled. Another check that's missing is running the integration tests.
# Modification
This PR adds two new checks to the reusable workflow. One to check for Cxx interoperability compatibility and another one to run the integration tests. I also fixed a misalgined name for the nightly benchmarks.
# Result
This should be one of the last reusable workflow checks.
# Motivation
Our 5.9 and nightly CI has been failing for some time now due to flaky allocation tests.
# Modification
This PR slightly changes some shared infra for how we ran some of our allocation tests. We are now waiting for the client and server channel to close so that allocations are more stable.
# Results
Green CI again
# Motivation
Our 5.9 and nightly CI has been failing for some time now due to flaky allocation tests.
# Modification
This PR slightly changes some shared infra for how we ran some of our allocation tests. We are now waiting for the client and server channel to close so that allocations are more stable.
# Results
Green CI again
* Extend the integration test harness to track FDs
Motivation
This patch extends the NIO integration test harness to track
file descriptors, in particular to search for leaks. This
change has been validated on Linux and Darwin, and in both cases
correctly diagnoses FD leaks.
The goal is to enable us to regression test for things like
Modifications
- Add support for hooking socket and close calls.
- Wire up this support into the test harness.
- Extend the test harness to handle the logging.
- Add new regression test for #2047.
Results
We can write regression tests for FD leaks.
* Disable FD checking in most builds.
I'm doing this for speed reasons
* Always print the leaked fds number
# Motivation
We landed the async bridge types a while back but never added allocation and performance tests. Since we expect these types to be used performance critical paths we really should cover those with tests.
# Modification
Extends the allocation counter scaffolding to support async tests. Furthermore, add allocations tests for both the writer and producer. Lastly, I a also added a performance test for the producer.
# Result
We now have baseline tests for the `NIOAsyncWriter` and `NIOAsyncSequenceProducer`
Co-authored-by: Cory Benfield <lukasa@apple.com>
Motivation:
Protocols like Sequence have "private" hooks that can be implemented to
provide fast-paths for some operations. We missed a few on BBV, and I'd
like to add them. This is one use-case where they can help.
Modifications:
- Add an allocation-counter benchmark and a runtime benchmark for
copying BBV to Array.
Result:
We have some benchmarks.
### Motivation:
In issue https://github.com/apple/swift-nio/issues/1316, we see a large number of allocations to happen when scheduling tasks. This can definitely be optimized. This PR adds a number of baseline allocation and performance tests for both `scheduleTask` and `execute`. In the next PRs, I am going to try a few optimizations to reduce the number of allocations.
### Modifications:
Added baseline performance and allocation tests for `scheduleTask` and `execute`
Motivation:
To justify performance changes we need to measure the code being
changed. We believe that `HTTPHeaders.subscript(canonicalForm:)` is a
little slow.
Modifications:
- Add allocation and performance tests for fetching header values in
their canonical form
Results:
More benchmarks!
Motivation:
The remaining NIO code really conceptually belongs in a module called
NIOPosix, and NIOCore should really be called NIO. We can't really do
that last step, but we can prepare by pushing the bulk of the remaining
code into a module called NIOPosix.
Modifications:
- Move NIO to NIOPosix
- Make NIO an umbrella module.
Result:
NIOPosix exists.
Motivation:
Peter thinks that result-erasing maps should not allocate, and we have
special code paths in the code to try to make Void -> Void maps not
allocate. Sadly, both code paths currently do allocate.
Per our rules for not trying to make optimizations without data, we
should start measuing these closures so we can make optimizations.
Modifications:
- Added an alloc couter test for result-erasing maps.
Result:
Alloc counter test suitable for any fix of #1697.
Motivation:
Allocation counter tests are good, and we aren't measuring this today.
Modifications:
- Wrote some add/remove tests that use different remove functions.
Result:
Better insight into performance.
* Add synchronous channel options
Motivation:
The functions for getting and setting channel options are currently
asynchronous. This ensures that options are set and retrieved safely.
However, in some cases the caller knows they are on the correct event
loop but still has to pay the cost of allocating a future to either get
or set an option.
Modifications:
- Add a 'NIOSynchronousChannelOptions' protocol for getting and setting
options
- Add a customisation point to 'Channel' to return 'NIOSynchronousChannelOptions'.
- Default implementation returns nil so as to not break API.
- Add implementations for 'EmbeddedChannel' and 'BaseSocketChannel'
- Allocation tests for getting and setting autoRead
Results:
Options can be get and set synchronously.
Motivation:
`ChannelPipeline` is explicitly thread-safe, any of the operations may
be called from outside of the channel's event loop. However, there are
often cases where it is known that the caller be on the right event
loop, and an asynchronous API is unnecessary.
In some cases -- such as when a pipeline is configured dynamically and
handlers are added from the 'channelRead' implementation of one handler
-- it forces the caller to write code that they might not actually need:
such as buffering events which may happen before the future completes.
This is unnecessary complexity when the caller knows that they must
already be on an event loop.
Modifications:
- Add a 'SynchronousOperations' view to the 'ChannelPipeline' which is
available to callers via 'syncOperations'.
- Supported operations include: adding a handler, adding multiple
handlers, retrieving a context via various predicates and retrieving a
handler of a given type.
- Some of the operations in 'ChannelPipeline' were refactored to have an
explicitly synchronous version, asynchronous versions complete their
promise based on the result of these calls.
- Various minor documentation fixes and addition of 'self' where it was
not used explicitly.
Result:
Users can perform synchronous operations on the 'ChannelPipeline' if
they know they are on the right event loop.
Motivation:
I'm sick of typing `.init(major: 1, minor: 1)`.
Modifications:
- Added static vars for common HTTP versions.
Result:
Maybe I'll never type `.init(major: 1, minor: 1)` ever again.
* Add allocation test for adding multiple handlers
Motivation:
I believe there is at least 1 avoidable allocation in this area.
Even if there isn't, making sure we don't increase allocations is good.
Modifications:
Add a test of allocations when adding multiple handlers.
Set limits for docker images.
Result:
Allocations when adding multiple handlers are now checked.
* Remove an allocation from addHandlers
Motivation:
Fewer allocations should improve performance.
Modifications:
Split out a sub function from addHandlers.
I originally thought I'd have to change the part of this
function which reads `var handlers = handlers` as there was
a surprising allocation at the beginning of this function.
It seems that breaking out some of the logic is sufficient
to remove an allocation.
Result:
1 fewer allocation.
* Fix up alloc tests.
Fabian Fett
Si Beaumont
aryan-25
Rick Newton-Rogers
George BarnettandCory Benfield
Gus Cairo
Clinton NkwochaandGeorge Barnett
Peter Adams
Ayush Garg
Franz Busch
Johannes Weiss