Motivation:
The various 'withMumbleContinuation' APIs are supposed to be invoked
synchronously with the caller. This assumption allows a lock to be
acquired before the call and released from the body of the
'withMumbleContinuation' after e.g. storing the continuation. However
this isn't the case and the job may be re-enqueued on the executor
meaning that this is pattern is vulnerable to deadlocks.
Modifications:
- Drop and reacquire the lock in the NIOThrowingAsyncSequenceProducer.
- Merge 'next()' and 'next(for:)' methods in the state machine into a
single func; this reduces the amount of duplicated logic across the two
functions.
Result:
Lower chance of deadlock
Motivation:
This is a follow up to 5bf841dd to handle the yield task being cancelled
between dropping the lock and re-acquiring it a moment later in the
'withContinuation' block.
Modifications:
- Add an extra cancellation check when yielding with a continuation.
Result:
Fewer issues
Motivation:
The various 'withMumbleContinuation' APIs are supposed to be invoked
synchronously with the caller. This assumption allows a lock to be
acquired before the call and released from the body of the
'withMumbleContinuation' after e.g. storing the continuation. However
this isn't the case and the job may be re-enqueued on the executor
meaning that this is pattern is vulnerable to deadlocks.
Modifications:
- Drop and reacquire the lock in the NIOAsyncWriter
Result:
Lower chance of deadlock
Fixes all warnings when `-require-explicit-sendable` flag is enabled and
enables the flag on macOS CI.
### Motivation:
We want to ensure our public API is either explicitly marked as
`Sendable` or not.
### Modifications:
Marked appropriate public types as `Sendable`, or explicitly defined
their conformance to the `Sendable` protocol as unavailable.
### Result:
We can now enable `-require-explicit-sendable` compiler flag in our
codebase.
When working on performance in PostgresNIO I noticed a few
`_swift_getGenericMetadata`s. We don't like those. I could trace them
down to `NIOThrowingAsyncSequenceProducer`. All methods on this object
are generic, since the object itself is generic.
Co-authored-by: George Barnett <gbarnett@apple.com>
Motivation:
We're continuing out Strict Concurrency journey, making sure users of
NIO can write data-race-free code.
Modifications:
- Added some missing Sendable annotations in NIOAsyncSequenceProducer
- Made BufferedStream unconditionally Sendable, and required its Element
type to also be Sendable.
The prior constraint wasn't actually correct. We always
behaved as though the element types were Sendable, by passing
them into continuations. This cleans things up.
- Made AnyAsyncSequence Sendable, which it needs to be.
- Made BufferedOrAnyStream Sendable, which it needs to be.
- Made DirectoryEntries explicitly Sendable, which it was.
- Made DirectoryEntries.Batched explicitly Sendable.
Result:
Better concurrency-safety
This fixes hitting the following preconditionFailure in NIOAsyncWriter:
"This should have already been handled by `yield()`".
It doesn't expect a yield to be suspended when the state is
`.writerFinished`, but this can definitely happen. This seemed to be the
correct solution to me, but please check it carefully, because I'm
unfamiliar with this code.
I'm not happy about the test for this. It's blocking the `didYield` call
for a while, to make sure the correct state is reached. See also the
'FIXME' line. What would be a better way to do this?
---------
Co-authored-by: Cory Benfield <lukasa@apple.com>
Motivation:
The high/low watermark backpressure strategy assumes that a yield can
never happen when there's no outstanding demand. It's reasonably easy
for this to not be a true: a handler decoding messages in a loop, for
example, may not pay attention to `read() calls and produce when there's
no outstanding demand. The channel should of course not read from the
socket, so produced values will stop being produced.
Modifications:
- Alter the high/low watermarks such that demand is only enabled when
the buffer depth is below the low watermark, and only disabled when
above the high watermark.
Result:
Fewer crashes
### Motivation:
Documentation checking catches more issues in Swift 6.0.
### Modifications:
Adopt the Swift 6.0 image and fix the errors.
### Result:
More accurate docs.
# Motivation
It was currently possible that the producer's delegate is getting called
twice with `produceMore` even if no `yield` returned a `stopProducing`.
This could happen when we expected the producer to yield elements but
the consumer went below the low watermark again. Resulting in two
subsequent calls.
# Modification
This PR stores the current demand state in the strategy which let's us
avoid flipping the `hasOustandingDemand` state of the sequence.
# Result
Correctly, ensured the call order of `produceMore` and `stopProducing`.
* 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:
NIOLockedValueBox has a 'safer' API than NIOLock as it only provides
scoped access to its boxed value. NIOLock requires users to only access
protected state while the lock is acquired. As such NIOLockedValueBox
should be preferred where possible. However, there are cases where
manual control must be used (such as storing a continuation) and users
must use a NIOLock for this.
There are two downsides to this:
1. All other access to the protected state must use the NIOLock API
putting the onus on the developer to only access the protected state
while the lock is held.
2. NIOLock can't store its protected state inline which typically
results in users storing it on a class.
Modifications:
- Add an 'unsafe' view to NIOLockedValueBox which allows users to
manually control the lock and access its protected state
- Update NIOAsyncWriter and NIOThrowingAsyncSequenceProducer to use NIOLockedValueBox
Result:
- Safer locking API is used in more places
- Fewer allocations
Motivation:
The NIOAsyncWriter uses an atomic to generate yield IDs. Each writer has
its own atomic. We can save an allocatiuon per writer but using a shared
atomic.
Each load then wrapping increment operation with relaxed ordering takes
approx 3ns on my machine. For a UInt64 this would take approx 188 years
to wrap around if run in a tight loop.
Modification:
- Share a single yield ID counter for NIOAsyncWriter
Result:
Fewer allocations
Motivation:
Even though NIOAsyncWriterError captures the file and line of the error they are internal and not printed in the
description.
Modifications:
- made the 2 properties public
- added them in the description
Result:
Users will be able to access and see the line number and file where a NIOAsyncWriterError occurred
# Motivation
We had to disable the benchmarks since they regressed without us noticing and they appear to be flaky.
# Modification
This PR fixes the allocation regression and tries to re-enable them.
* Add `withInboundOutboud` to `NIOAsyncChannel` and deprecate deinit based cleanup
# Motivation
We just released our new async NIO APIs and have already gotten quite a bunch of feedback from adopters. One of the feedback was that the deinit based closing that we have added to the `NIOAsyncChannel` has caused problems since it leads to unexpected closure of their `Channel`. Furthermore, it makes it impossible to determine how many open sockets a program has at any given time since deinit based clean up relies on the optimizer and can happen at random times.
# Modifications
This PR adds new inits to `NIOAsyncSequenceProducer` and `NIOAsyncWriter` which disable the `deinit` based clean up and instead replace them with an assertion. This allows developers to still catch these issues at debug time. Furthermore, I added a new `withInboundOutbound` scoped access to `NIOAsyncChannel` which will close the channel at the end of the scope. This still gives users a nice API while not having to care much about closing themselves.
# Result
We are no longer using deinit based clean up and bring back one of the core principles of NIO which is deterministic resource usage.
* Review comments
* Internal labels for closure arguments
* Rename to `executeThenCloseChannel`
* Actually call `sinkDeinitialized`
* Change preconditions
* Move logic to deinits
* Rename to `executeThenClose` and review nits
* Fix reordering/reentrancy bug in `NIOAsyncWriter` + `NIOAsyncChannel`
# Motivation
While testing the latest async interfaces we found a potential reordering/reentrancy bug in the `NIOAsyncWriter`. This was caused due to our latest performance changes where we fast-pathed calls in `didYield` to not hop. The problem was in the following flow:
1. Task 1: Calls `outbound.write()` -> which led an `EventLoop` enqueue with `didYield`
2. Task 1: Calls `outbound.write()` -> which led an `EventLoop` enqueue with `didYield`
3. EventLoop: While processing the write from 1. the channel became **not** writable
4. Task 1: Calls `outbound.write()` -> which lead to buffering the write in the writer's state machine since we are **not** writable
5. EventLoop: While still processing the write from 1. the channel became writable again -> We informed the `NIOAsyncWriter` about this which unbuffered the write in 4. that was stored in the state machine and call `didYield`. Since, we are on the EventLoop already we processed the write right away
The above flow show-cases a flow where we reordered the write in 2. and 4.
# Modification
This PR fixes the above issue while upholding a few constraints:
1. Produce as few context switches as possible
2. Minimize allocations
I tried different approaches but in the end decided to do the following:
1. Make sure to never call `didYield/didTerminate` from calls on the `NIOAsyncWriter.Sink`
2. Don't coalesce the elements of different writes in the `NIOAsyncWriter` but rather use the suspended tasks to retry a write after they were suspended. I choose to do this since I wanted to avoid any allocation (remember writers are `some Sequence`) and because we assume that continuous contention in a multi producer pattern is low.
3. Make sure that `Sink.finish()` is terminal and does not lead to a `didTerminate` event. This is in line with 1.
One important thing to call out, our `writer.finish()` method is not `async` we have to buffer the finish event and deliver it with the yield that got buffered before we transitioned to `writerFinished`.
# Result
No more reordering/reentrancy problems in `NIOAsyncWriter` or `NIOAsyncChannel`.
* Code review
Motivation:
Whilst investigating deadlocking code elsewhere we discovered that
resuming a continuation inside a lock can call deadlocks.
The reason is that
- `withTaskCancellationHandler` takes a sequence's
underlying runtime lock then executes the cancellation handler.
- The task cancellation handler then does work which requires taking the
NIO-level lock
- If a NIO method on a separate thread then takes the NIO-level lock and
within it resumes a continuation e.g. in `yield` we will deadlock
because the resumption attempts to obtain the underlying runtime lock.
Modifications:
Do not resume continuations within locks.
Result:
Fewer deadlock opportunities
* Call `NIOAsyncWriterSinkDelegate` outside of the lock
# Motivation
The current `NIOAsyncWriter` implementation expects that the delegate is called while holding the lock to avoid reentrancy issues. However, this prevents us from executing the delegate calls directly on the `EventLoop` if we are on it already.
# Modification
This moves all of the delegate calls outside of the locks and adds protection against reentrancy into the state machine.
# Result
Less allocations.
Clarify the reentrancy problems in docs and protect against them in the writer
* Code review
* Implement fast paths for `AsyncChannelInboundStreamChannelHandler`
* Add `_TinyArray` from `swift-certificates`
* Implement single element customization point in `NIOAsyncChannelOutboundWriterHandler`
* Call the single element optimization more often and store suspended producers in `_TinyArray`.
* Update thresholds
* Fix compiler warning
### 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>
# 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
Using the `NIOAsyncSequenceProducer` requires a bunch of knowledge around when and how the delegate is getting called to ensure a correct back-pressure implementation. We should enhance the docs a bit more to surface some of the invariants that we expect.
# Modification
Extend the docs for `NIOAsyncSequenceProducer`.
# Result
Better docs.
# Motivation
Our `NIOAsyncSequenceProducer` is a unicast `AsyncSequence`; therefor, we must ensure that only a single iterator is every created. Our failure mode in the case another iterator is created is to `fatalError`. Currently, this `fatalError` is not produced if the sequence is in the `finished` state. This results in hard to debug behaviour since the user will get a basically useless iterator.
# Modification
Ensure that the `finished` state also keeps track of if an iterator was initialised and produces the correct `fatalError`.
# Result
We are now consistent in how many iterators can be created for every state.
# Motivation
We went through a lot of changes for the API of the `NIOAsyncWriter` and some doc comments suffered from this.
# Modification
Update doc comments for the `NIOAsyncWriter`.
Small fixup for the docs of the `NIOLockedValueBox`
# Result
More accurate docs
* Implement a `NIOAsyncWriter`
# Motivation
We previously added the `NIOAsyncProducer` to bridge between the NIO channel pipeline and the asynchronous world. However, we still need something to bridge writes from the asynchronous world back to the NIO channel pipeline.
# Modification
This PR adds a new `NIOAsyncWriter` type that allows us to asynchronously `yield` elements to it. On the other side, we can register a `NIOAsyncWriterDelegate` which will get informed about any written elements. Furthermore, the synchronous side can toggle the writability of the `AsyncWriter` which allows it to implement flow control.
A main goal of this type is to be as performant as possible. To achieve this I did the following things:
- Make everything generic and inlinable
- Use a class with a lock instead of an actor
- Provide methods to yield a sequence of things which allows users to reduce the amount of times the lock gets acquired.
# Result
We now have the means to bridge writes from the asynchronous world to the synchronous
* Remove the completion struct and incorporate code review comments
* Fixup some refactoring leftovers
* More code review comments
* Move to holding the lock around the delegate and moved the delegate into the state machine
* Comment fixups
* More doc fixes
* Call finish when the sink deinits
* Refactor the writer to only yield Deques and rename the delegate to NIOAsyncWriterSinkDelegate
* Review
* Fix some warnings
* Fix benchmark sendability
* Remove Failure generic parameter and allow sending of an error through the Sink
* Call finish once the Source is deinited
# Motivation
We **MUST** call `finish()` when the `Source` deinits otherwise we can have a suspended continuation that never gets resumed.
# Modification
Introduce an internal class to both `Source`s and call `finish()` in their `deinit`s.
# Result
We are now resuming all continuations.
* Remove @unchecked
* Small changes for the `NIOAsyncSequenceProducer`
# Motivation
In the PR for the `NIOAsyncWriter`, a couple of comments around naming of `private` properties that needed to be `internal` due to inlinability and other smaller nits came up.
# Modification
This PR includes two things:
1. Fixing up of the small nits like using `_` or getting the imports inside the `#if` checks
2. Changing the public API of the `makeSequence` to be aligned across the throwing and non-throwing one.
# Result
Cleaner code and alinged APIs.
* Fix refactoring left-overs
* Add throwing version of `NIOAsyncSequenceProducer`
# Motivation
We recently introduced a `NIOAsyncSequenceProducer` to bridge a stream of elements from the NIO world into the async world. The introduced type was a non-throwing `AsyncSequence`. To support all use-cases we also need to offer a throwing variant of the type.
# Modification
- Introduce a new `NIOThrowingAsyncSequenceProducer` that is identical to the `NIOAsyncSequenceProducer` except that it has a `Failure` generic parameter and that the `next()` method is throwing.
- Extract the `StateMachine` from both `AsyncSequenceProducer`s and unify them.
- There is one modification in behaviour: `didTerminate` is now only called after `nil` or the error has been consumed from the sequence.
# Result
We now have a throwing variant of the `NIOAsyncSequenceProducer`.
* Code review and fix CI
* Remove duplicated code
Co-authored-by: Cory Benfield <lukasa@apple.com>
Motivation:
One of the `NIOAsyncSequenceProducer` extensions was missing a
availability requirements.
Modifications:
Add missing requirement.
Result:
Fewer bugs.
* Implement a back-pressure aware `AsyncSequence` source
# Motivation
We ran into multiple use-cases (https://github.com/apple/swift-nio/pull/2067, https://github.com/grpc/grpc-swift/blob/main/Sources/GRPC/AsyncAwaitSupport/PassthroughMessageSource.swift) already where we want to vend an `AsyncSequence` where elements are produced from the sync world while the consumer is in the async world. Furthermore, we need the `AsyncSequence` to properly support back-pressure.
Since we already identified that this is something fundamental for our ecosystem and that current `AsyncSequence` sources are not providing the proper semantics or performance, it would be great to find a single solution that we can use everywhere.
Before diving into the code, I think it is good to understand the goals of this `AsyncSequence`:
- The `AsyncSequence` should support a single unicast `Subscriber`
- The `AsyncSequence` should allow a pluggable back-pressure strategy
- The `AsyncSequence` should allow to yield a sequence of elements to avoid aquiring the lock for every element.
- We should make sure to do as few thread hops as possible to signal the producer to demand more elements.
# Modification
This PR introduces a new `AsyncSequence` called `NIOBackPressuredAsyncSequence`. The goal of that sequence to enable sync to async element streaming with back-pressure support.
# Result
We can now power our sync to async use-cases with this new `AsyncSequence`.
# Future work
There are couple of things left that I wanna land in a follow up PR:
1. An adaptive back-pressure strategy that grows and shrinks depending on the speed of consumption
2. A throwing version of this sequence
3. Potentially an async version that suspends on `yield()` and resumes when more elements should be demanded.
* Fix cancellation handling
* Review
* Add init helper
* Add return types to help the type checker
* Fix 5.7 CI
* Rename delegate method and update docs
* Review
* Switch to Deque, rename the type and change the behaviour of yielding an empty sequence
* Review comments from George
* Review comments by Konrad
* Review and add tests for the high low strategy
* Fix 5.4 tests
* Code review
Co-authored-by: Cory Benfield <lukasa@apple.com>
George Barnett
Cory Benfield
Rafael Cepeda
Fabian FettandGeorge Barnett
Manolo van EeandCory Benfield
Rick Newton-Rogers
Franz Busch
Stefana-Ioana Dranca
Si BeaumontandFranz Busch
Rick Newton-Rogers
Felix Schlegel
David Nadoba
Johannes Weiss