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:
Follow up to #2412 which changed the async channel bootstrap tests to
bind to port 0 instead of a fixed port. A few instances were missed,
this fixes up the remainder.
Modifications:
- Bind to port 0
Result:
Tests should run in parallel without failures.
# Motivation
The `AsyncChannelBootstrapTests` were failing when running parallel since they were all using the same port.
# Modification
This PR uses port `0` to let the system assign a port instead. It also runs the formatter on this file.
# Result
No more tests failures.
* 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
* Add `AsyncChannel` based `ServerBootstrap.bind()` methods
# Motivation
In my previous PR, we added a new async bridge from a NIO `Channel` to Swift Concurrency primitives in the from of the `NIOAsyncChannel`. This type alone is already helpful in bridging `Channel`s to Concurrency; however, it is hard to use since it requires to wrap the `Channel` at the right time otherwise we will drop reads. Furthermore, in the case of protocol negotiation this becomes even trickier since we need to wait until it finishes and then wrap the `Channel`.
# Modification
This PR introduces a few things:
1. New methods on the `ServerBootstrap` which allow the creation of `NIOAsyncChannel` based channels. This can be used in all cases where no protocol negotiation is involved.
2. A new protocol and type called `NIOProtocolNegotiationHandler` and `NIOProtocolNegotiationResult` which is used to identify channel handlers that are doing protocol negotiation.
3. New methods on the `ServerBootstrap` that are aware of protocol negotiation.
# Result
We can now easily and safely create new `AsyncChannel`s from the `ServerBootstrap`
* Code review
* Fix typo
* Fix up tests
* Stop finishing the writer when an error is caught
* Code review
* Fix up writer tests
* Introduce shared protocol negotiation handler state machine
* Correctly handle multi threaded event loops
* Adapt test to assert the channel was closed correctly.
* Code review
Motivation:
The fix provided in #2407 was subtly wrong. ignoreSIGPIPE, which throws
the error in question, closes the FD on error _except_ on EINVAL from
fcntl, where it instead does not. This inconsistent behaviour is the
source of the bug. Because this behaviour is inconsistent, the fix from
PR #2407 is also inconsistent and can in some cases double-close the
socket.
The actual issue is not as old as I expected: the code can be observed
by reviewing the change in #1598, which incorrectly inserted the error
transformation before the call to close.
Modifications:
- Revert the change from #2407.
- Move the close in ignoreSIGPIPE to before the error check, rather than
after, so we unconditionally execute it.
Result:
More resilient fix.
Motivation:
When an error is hit during a read loop, a channel is able to tolerate
that error without closing. This is done for a number of reasons, but
the most important one is accepting sockets for already-closed
connections, which can trigger all kinds of errors on the read path.
Unfortunately, there was an edge-case in the code for handling this
case. If one or more reads in the loop had succeeded before the error
was caught, the inner code would be expecting a call to readIfNeeded,
but the outer code wouldn't make it. This would lead to autoRead
channels being wedged open.
Modifications:
This patch extends the Syscall Abstraction Layer to add support for
server sockets. It adds two tests: one for the basic accept flow, and
then one for the case discussed above.
This patch also refactors the code in BaseSocketChannel.readable0 to
more clearly show the path through the error case. There were a number
of early returns and partial conditionals that led to us checking the
same condition in a number of places. This refactor makes it clearer
that it is possible to exit this code in the happy path, with a
tolerated error, which should be considered the same as reading
_something_.
Result:
Harder to wedge a channel open.
Motivation:
In some circumstances we can accept a socket that is already closed. In
those cases, creating the underlying Socket type will fail, as
attempting to ignore SIGPIPE will fail. On Apple platforms, this causes
us to leak the accepted socket, and can lead to file descriptor
exhaustion.
Modifications:
- Close the accepted socket if we fail to create a Socket class
Result:
No FD leaks
# Motivation
I spotted a bug in the ALPNHandler where it doesn't properly unbuffer reentrant reads. This can lead to dropped reads.
# Modification
Instead of buffering into an array we are now buffering into a Deque and unbuffer as long as there are reads in the Deque.
# Result
No more dropped reads.
### Motivation:
Follow up PR for https://github.com/apple/swift-nio/pull/2399
We currently still return `nil` if the current `Task` is canceled before the first call to `NIOThrowingAsyncSequenceProducer.AsyncIterator.next()` but it should throw `CancellationError` too.
In addition, the generic `Failure` type turns out to be a problem. Just throwing a `CancellationError` without checking that `Failure` type is `any Swift.Error` or `CancellationError` introduced a type safety violation as we throw an unrelated type.
### Modifications:
- throw `CancellationError` on eager cancellation
- deprecates the generic `Failure` type of `NIOThrowingAsyncSequenceProducer`. It now must always be `any Swift.Error`. For backward compatibility we will still return nil if `Failure` is not `any Swift.Error` or `CancellationError`.
### Result:
`CancellationError` is now correctly thrown instead of returning `nil` on eager cancelation. Generic `Failure` type is deprecated.
Motivation:
Up until recently, it has not been possible to regression check our
documentation. However, in recent releases of the DocC plugin it has
become possible to make warnings into errors, making it possible for us
to CI our docs.
This patch adds support for doing that, and also cleans up our
documentation so that it successfully passes the check.
Along the way I accidentally wrote an `index.md` for `NIOCore` so I
figure we may as well keep it.
Modifications:
- Structure the documentation for NIOCore
- Fix up DocC issues
- Add `check-docs.sh` script to check the docs cleanly build
- Wire things up to our docker-compose scripts.
Result:
We can CI our docs.
Co-authored-by: George Barnett <gbarnett@apple.com>
* Land `NIOAsyncChannel` as SPI
# Motivation
We want to provide bridges from NIO `Channel`s to Swift Concurrency. In previous PRs, we already landed the building blocks namely `NIOAsyncSequenceProducer` and `NIOAsyncWriter`. These two types are highly performant bridges between synchronous and asynchronous code that respect back-pressure.
The next step is to build convenience methods that wrap a `Channel` with these two types.
# Modification
This PR adds a new type called `NIOAsyncChannel` that is capable of wrapping a `Channel`. This is done by adding two handlers to the channel pipeline that are bridging to the `NIOAsyncSequenceProducer` and `NIOAsyncWriter`.
The new `NIOAsyncChannel` type exposes three properties. The underlying `Channel`, a `NIOAsyncChannelInboundStream` and a `NIOAsyncChannelOutboundWriter`. Using these three types the user a able to read/write into the channel using `async` methods.
Importantly, we are landing all of this behind the `@_spi(AsyncChannel`. This allows us to merge PRs while we are still working on the remaining parts such as protocol negotiation.
# Result
We have the first part necessary for our async bridges. Follow up PRs will include the following things:
1. Bootstrap support
2. Protocol negotiation support
3. Example with documentation
* Add AsyncSequence bridge to NIOAsyncChannelOutboundWriter
* Code review
* Prefix temporary spi public method
* Rename writeAndFlush to write
Motivation:
NIOLock uses a storage object constructed from a ManagedBuffer. In
general this is fine, but it's a tricky API to use safely and we want to
avoid violating any of its guarantees.
Modifications:
- Store the value in the header and the lock in the elements
- Add debug assertions on alignment
Result:
We'll be a bit more confident of the use of NIOLock
Motivation:
SwiftPM has changed its default layout for packages in
apple/swift-package-manager#6144. This breaks our CI, which assumes the
prior layout. We should work around this.
Modifications:
Enhance the code to tolerate both layouts.
Result:
Integration tests run on all platforms
Motivation:
The recently added UDP GRO tests fail on some older Linux Kernel
versions. We believe that UDP GRO support on loopback was limited in
early versions so we should tolerate those failures.
However, we've verified that on 5.15 and newer that GRO is supported so
we should not tolerate failure in those cases.
Modifications:
- Add shims to CNIOLinux to get system info via uname
- Verify GRO works before running GRO tests and if it doesn't then
validate the kernel version isn't greater than 5.15.
Results:
Less flaky tests.
* Pool buffers for messages and addresses.
* Revert changes related to controlMessageStorage
* Cosmetic fix.
---------
Co-authored-by: Cory Benfield <lukasa@apple.com>
Motivation:
Support was added for UDP_SEGMENT in #2372 which allows for large UDP
datagrams to be written to a socket by letting the kernel or NIC segment
the data across multiple datagrams. This reduces traversals across the
network stack which can lead to performance improvements. UDP_GRO is the
receive-side counterpart allowing the kernel/NIC to aggregate datagrams
and reduce network stack traversals.
Modifications:
- Add a function in CNIOLinux to check whether UDP_GRO is supported
- Add the relevant socket and channel options
- Add tests
Result:
- UDP_GRO can be enabled where supported and applications may receive
large buffers.
mark syncShutdownGracefully noasync
Motivation:
The code as-is blocks the calling thread.
Modifications:
* mark `EventLoopGroup.syncShutdownGracefully()` and `NIOThreadPool.syncShutdownGracefully()` noasync on Swift > 5.7
* offer NIOThreadPool.shutdownGracefully()
* add renamed to syncShutdownGracefully()
Motivation:
EventLoopTest has an availability annotation for the various Darwin
platforms. This availability check is repeated in a few tests which
yields warnings.
Modifications:
Remove the class level availability guard.
Result:
Warning free.
Co-authored-by: Cory Benfield <lukasa@apple.com>
Motivation:
I (foolishly) didn't validate the test fix in #2382, instead I validated
that the original test passed with 61 segments (rather than 64). The
channel needs to be recreated first.
Modifications:
- Rebuild the channel before trying again if 64 segments is too many.
Result:
Test passes.
Motivation:
On older kernel versions testWriteBufferAtGSOSegmentCountLimit fails
because the write fails with EINVAL. This appears to be a kernel bug as
it passes on more recent versions.
Modifications:
- Try again with a lower segment limit if the write fails with EINVAL.
Result:
Less flaky test.
setOption forms a raw pointer to a generic argument. The compiler will
warn on this as of:
[proposal] Constrain implicit raw pointer conversion... #1963https://github.com/apple/swift-evolution/pull/1963
/Sources/NIOPosix/BaseSocket.swift:286:31: warning: forming
'UnsafeRawPointer' to a variable of type 'T'; this is likely incorrect
because 'T' may contain an object reference.
option_value: &val,
^
Ideally, this would be fixed by adding a BitwiseCopyable constraint to
the 'value' parameter of 'BaseSocker.setOption'. That would not only
eliminate the warning, but would make the API safer. But
BitwiseCopyable isn't quite ready for public use. In the meantime,
this is a reasonable workaround.
Co-authored-by: Cory Benfield <lukasa@apple.com>
Motivation:
On Linux, the UDP_SEGMENT socket option allows for large buffers to be
written to the kernel and segmented by the kernel (or in some cases the
NIC) into smaller datagrams. This can substantially decrease the number
of syscalls.
This can be set on a per message basis on a per socket basis. This
change adds per socket configuration.
Modifications:
- Add a CNIOLinux function to check whether UDP_SEGMENT is supported on
that particular Linux.
- Add a helper to `System` to check whether UDP_SEGMENT is supported on
the current platform.
- On Linux only:
- add the udp socket option level
- add the udp_segment socket option
- Add the `DatagramSegmentSize` channel option.
- Get/Set the option in `DatagramChannel`
Results:
UDP GSO is supported on Linux.
Co-authored-by: Cory Benfield <lukasa@apple.com>
Motivation:
swift- nio was failing builds that should pass
Modifications:
Adding available to the necessary sections
* Updating test OnToRunClosure
Motivation:
testCancelledScheduledTasksDoNotHoldOnToRunClosure() was not allowed enough time and timing off at moments, causing it to fail occasionally
Modifications:
Added a ConditionLock throughout the code to make sure it only unlocks when the code has waited enough time for it to not hit the precondition failure
Motivation:
Our time types are trivial, and they should be fully transparent. This
produces minor performance improvements in code handling time types, but
is mostly useful in terms of allowing the compiler to observe that these
functions have no side effects, thereby eliding some ARC traffic.
Modifications:
Make our time types inlinable.
Result:
Better performance.
Motivation:
Channels can read `ChannelOptions.maxMessagesPerRead` times from a
socket in each read cycle. They typically re-use the same buffer for
each read and rely on it CoWing if necessary. If we read more than once
in a cycle then we may CoW the buffer. Instead of reusing one buffer we
can reuse a pool of buffers limited by `maxMessagesPerRead` and cycle
through each, reducing the chance of CoWing the buffers.
Modifications:
- Extend `RecvByteBufferAllocator` to provide the size of the next
buffer with a default implementation returning `nil`.
- Add an recv buffer pool which lazily grows up to a fixed size and
attempts to reuse buffers where possible if doing so avoids CoWing.
Results:
Fewer allocations
Motivation:
To know when we next need to wake up, we keep track of what the next
deadline will be. This works great, but in order to keep track of this
UInt64 we save off an entire ScheduledTask. This object is quite wide (6
pointers wide), and two of those pointers require ARC traffic, so doing
this saving produces unnecessary overhead.
Worse, saving this task plays poorly with task cancellation. If the
saved task is cancelled, this has the effect of "retaining" that task
until the next event loop tick. This is unlikely to produce catastrophic
bugs in real programs, where the loop does tick, but it violates our
tests which rigorously assume that we will always drop a task when it is
cancelled. In specific manufactured cases it's possible to produce leaks
of non-trivial duration.
Modifications:
- Wrote a weirdly complex test.
- Moved the implementation of Task.readyIn to a method on NIODeadline
- Saved a NIODeadline instead of a ScheduledTask
Result:
Minor performance improvement in the core event loop processing, minor
correctness improvement.
Motivation:
- Uses Concurrency but missing availability requirements; does not build
on macOS.
Modifications:
- Add availability requirements/guards
Result:
Compiles on macOS
# Motivation
We are currently always allocating a new `Deque` when we get a single element write in the `NIOAsyncWriter`
# Modification
Provide a fast path method on the `NIOAsyncWriterSinkDelegate` protocol which will be called when we receive a single element write and are currently writable.
# Result
Performance win for the single write cases.
Motivation:
PooledBuffer is an inherently unsafe type, but its original incarnation
was less safe than it needed to be. In particular, we can rewrite it to
ensure that it is compatible with automatic reference counting.
Modifications:
- Rewrite PooledBuffer to use ManagedBuffer
- Clean up alignment math
- Use scoped accessors
- Add hooks for future non-scoped access
Result:
Safer, clearer code
* Pool buffers for ivecs and storage refs in the event loop.
* Introduce PoolElement for poolable objects and add some bounds checks for the pooled buffers.
* Some polishes.
* Fix build failure with Swift 5.5/5.6
* User raw pointers instead of typed.
Motivation:
The `PendingDatagramWritesManager` unconditionally creates an array and
reserves capacity... only to never use it.
Modifications:
Remove the unsued code.
Result:
Fewer allocations.
# Motivation
Currently `testTaskCancel_whenStreaming_andNotSuspended` is flaky since `didTerminate` can be called after the iterator is dropped. Fixes https://github.com/apple/swift-nio/issues/2354
# Modification
Let's modify that slightly so we hight the condition we want to hit.
# Result
No more flaky tests.
Motivation:
The NIO docs are now published on the Swift Package Index but the README
still refers to GitHub pages.
Modifications:
- Update README and other docs to point to Swift Package Index.
Result:
Documentation links work
Motivation:
Our allocation counter tests still build for 5.0 by default. This isn't
great, but we've been getting away with it because building for 5.7
requires changing the way we express our dependencies. Doing that is a
big breaking pain in the neck that requires changes in multiple repos.
As a shorter-term goal, however, to enable testing in at least one more
repo (swift-nio-ssh), this PR lifts the version to 5.1, which is the
last release compatible with the Package.swift dependency structure we
use but that supports the Platforms we need to express.
Modifications:
- Move allocation counter to 5.1
- Add platforms from swift-nio-ssh
Result:
We can write allocation counter tests for swift-nio-ssh.