Implement a `NIOAsyncWriter` #2251

FranzBusch · 2022-08-26T15:59:25Z

Motivation

We previously added the NIOAsyncSequenceProducer 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

glbrntt

Only just got to the meat of this PR; will pick up the rest of the review later. Leaving my comments so far.

Sources/NIOCore/AsyncSequences/NIOAsyncWriter.swift

fabianfett

Great start. Left some broader questions for now.

Sources/NIOCore/AsyncSequences/NIOAsyncWriter.swift

fabianfett · 2022-08-30T16:28:40Z

Sources/NIOCore/AsyncSequences/NIOAsyncWriter.swift

+            var yieldID: UInt64
+            /// The yield's produced sequence of elements.
+            @usableFromInline
+            var elements: AnySequence<Element>


Should the AsyncWriter be generic over the Sequence type as well? How often do user's want to send different Sequence types? For the SequenceOfOne/Two case users could always opt for an enum that implements Sequence.

Yeah, I didn't like the point of holding an AnySequence here. If we do that, then we should get rid of the yielding of a single element.

If we do that, then we should get rid of the yielding of a single element.

I think we must and I consider this a good thing :P

Should the AsyncWriter be generic over the Sequence type as well? How often do user's want to send different Sequence types?

I'm not sure that's a good idea in practice. I think the typical usage pattern would be for a framework to create a writer and pass it to a caller. That would mean the framework would be responsible for choosing what the sequence type is or the caller has to ask the framework for a writer with a given sequence type (which is a pretty awkward API).

For the SequenceOfOne/Two case users could always opt for an enum that implements Sequence.

Not offering a yield-one API and requiring users to implement an enum like this makes for a really user hostile API IMHO.

FWIW I also don't like AnySequence here but I think we should keep it for the time being (it is an implementation detail and can be changed later).

One alternative would be to always store [Element] and special case the API to avoid copies when an array is provided and copy into an array when anything else is provided.

Taking that further we could be to pull a 'NIOAny' trick and have an enum of array, one, and anySequence.

That said, I think we should focus on getting the API right first and then compare perf.

I agree with what George said. Making the whole thing generic of the sequence that is yielded severely limits the usage of the type and makes it very awkward to hold. I would stick with AnySequence for now and as George said we can always make it faster by applying some of our tricks.

As discussed offline, I think we should change this to remove the AnySequence.

fabianfett · 2022-08-30T16:32:45Z

Sources/NIOCore/AsyncSequences/NIOAsyncWriter.swift

+            let yieldID = self._yieldIDCounter.loadThenWrappingIncrement(ordering: .relaxed)
+
+            try await withTaskCancellationHandler {
+                // We are manually locking here to hold the lock across the withCheckedContinuation call
+                self._lock.lock()
+
+                let action = self._stateMachine.yield(yieldID: yieldID)


What's the benefit of having the yieldIDCounter as an atomic, if we acquire a lock afterwards anyway?

We acquire the lock inside the cancellation handler and inside the operation. However, we need to get the ID outside of both of them. We have two options:

Aquire the lock before once to get an id and then acquire the lock inside again

Use an atomic for the id

I went with the atomic because that should perform better than the lock.

Sources/NIOCore/AsyncSequences/NIOAsyncWriter.swift

glbrntt · 2022-08-31T08:38:20Z

Sources/NIOCore/AsyncSequences/NIOAsyncWriter.swift

+            var yieldID: UInt64
+            /// The yield's produced sequence of elements.
+            @usableFromInline
+            var elements: AnySequence<Element>


Should the AsyncWriter be generic over the Sequence type as well? How often do user's want to send different Sequence types?

I'm not sure that's a good idea in practice. I think the typical usage pattern would be for a framework to create a writer and pass it to a caller. That would mean the framework would be responsible for choosing what the sequence type is or the caller has to ask the framework for a writer with a given sequence type (which is a pretty awkward API).

For the SequenceOfOne/Two case users could always opt for an enum that implements Sequence.

Not offering a yield-one API and requiring users to implement an enum like this makes for a really user hostile API IMHO.

FWIW I also don't like AnySequence here but I think we should keep it for the time being (it is an implementation detail and can be changed later).

One alternative would be to always store [Element] and special case the API to avoid copies when an array is provided and copy into an array when anything else is provided.

Taking that further we could be to pull a 'NIOAny' trick and have an enum of array, one, and anySequence.

That said, I think we should focus on getting the API right first and then compare perf.

Sources/NIOCore/AsyncSequences/NIOAsyncWriter.swift

Lukasa

I think we're mishandling the delegate throughout this code.

Because we only call the delegate from outside the lock, we do not guarantee to maintain ordering of yields. We can suspend immediately after we drop the lock, and then have a completely unrelated Task come in and yield results, get an immediate call to didYield, and then drop into the delegate, beating the first Task.

This didn't matter in the previous delegate, which was only being called with signals that were themselves well-ordered, but it does matter here. We need a fundamentally different behaviour that maintains ordering, probably by way of keeping the lock held across these calls. That will require some thought as to how it interacts with setWritability, which we need to be careful wtih.

Sources/NIOCore/AsyncSequences/NIOAsyncWriter.swift

glbrntt

Looks great! Left a few documentation nits/fixes.

Sources/NIOCore/AsyncSequences/NIOAsyncWriter.swift

Lukasa · 2022-09-07T14:47:48Z

Sources/NIOCore/AsyncSequences/NIOAsyncWriter.swift

+            var yieldID: UInt64
+            /// The yield's produced sequence of elements.
+            @usableFromInline
+            var elements: AnySequence<Element>


As discussed offline, I think we should change this to remove the AnySequence.

FranzBusch · 2022-09-08T15:55:24Z

@Lukasa I changed the API to always yield a Deque now and I added a performance test for this. Let me know what you think!

Sources/NIOCore/AsyncSequences/NIOAsyncWriter.swift

Sources/NIOPerformanceTester/NIOAsyncWriterSingleWritesBenchmark.swift

Lukasa · 2022-09-16T15:49:37Z

Sources/NIOCore/AsyncSequences/NIOAsyncWriter.swift

+@available(macOS 10.15, iOS 13, tvOS 13, watchOS 6, *)
+public struct NIOAsyncWriter<
+    Element,
+    Failure,


Presumably we require that Element and Failure are Sendable, no?

They already are. The delegate is currently restricting the Element to be Sendable which propagates to this generic type through the constraints. Furthermore, Failure is restricted to Error which inherits from Sendable.
But I added it here as constraints as well to make it more obvious

Actually taking this back since it produces a warning since it is a duplicate constraint.

Sources/NIOCore/AsyncSequences/NIOAsyncWriter.swift

# 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

…nto the state machine

…IOAsyncWriterSinkDelegate

…h the Sink

FranzBusch · 2022-09-21T09:42:03Z

@Lukasa @glbrntt @gjcairo I just pushed a new commit that changes a few things after our offline discussion:

Allow sending an Error with the Sink/finish(error:) method. Any suspended or subsequent call to NIOAsyncWriter/yield will throw this error. It provides a way to surface an underlying error to the callers of the NIOAsyncWriter. We have been doing this in grpc-swift to surface errors from the channel pipeline
I removed the generic Failure parameter from the NIOAsyncWriter since we agreed that there is little value in providing a generic here over just an Error. Furthermore, since we want to allow to send an Error from the Sink/finish(error:) method it would mean we would have to introduce another generic parameter.
Added a few more tests for the new state and updated the documentation.

glbrntt

This is great @FranzBusch -- I think a couple of docs are out of date and should be fixed in a followup.

Sources/NIOCore/AsyncSequences/NIOAsyncWriter.swift

FranzBusch force-pushed the fb-async-writer branch from 3732c95 to 558af8a Compare August 26, 2022 18:00

FranzBusch requested review from Lukasa, dnadoba, fabianfett, glbrntt and ktoso August 26, 2022 18:01

glbrntt reviewed Aug 30, 2022

View reviewed changes

Sources/NIOCore/AsyncSequences/NIOAsyncWriter.swift Outdated Show resolved Hide resolved

Sources/NIOCore/AsyncSequences/NIOAsyncWriter.swift Outdated Show resolved Hide resolved

Lukasa added the 🔼 needs-minor-version-bump For PRs that when merged cause a bump of the minor version, ie. 1.x.0 -> 1.(x+1).0 label Aug 30, 2022

FranzBusch requested a review from glbrntt August 30, 2022 12:22

FranzBusch force-pushed the fb-async-writer branch from f215798 to 69f14ac Compare August 30, 2022 12:23

glbrntt reviewed Aug 30, 2022

View reviewed changes

FranzBusch force-pushed the fb-async-writer branch from df36553 to ac921d4 Compare August 30, 2022 16:10

fabianfett reviewed Aug 30, 2022

View reviewed changes

glbrntt reviewed Aug 31, 2022

View reviewed changes

Lukasa requested changes Sep 2, 2022

View reviewed changes

FranzBusch requested review from Lukasa and glbrntt September 7, 2022 10:45

FranzBusch force-pushed the fb-async-writer branch from ac921d4 to a81dbd1 Compare September 7, 2022 10:45

FranzBusch requested a review from fabianfett September 7, 2022 10:45

glbrntt approved these changes Sep 7, 2022

View reviewed changes

Lukasa reviewed Sep 7, 2022

View reviewed changes

FranzBusch requested a review from Lukasa September 8, 2022 15:54

glbrntt reviewed Sep 13, 2022

View reviewed changes

Sources/NIOCore/AsyncSequences/NIOAsyncWriter.swift Outdated Show resolved Hide resolved

Lukasa reviewed Sep 20, 2022

View reviewed changes

FranzBusch added 3 commits September 20, 2022 13:57

Remove the completion struct and incorporate code review comments

3d22230

Fixup some refactoring leftovers

d64143d

FranzBusch added 7 commits September 20, 2022 13:57

More code review comments

eed945a

Move to holding the lock around the delegate and moved the delegate i…

b66a70c

…nto the state machine

Comment fixups

07f2faf

More doc fixes

64ec5cc

Call finish when the sink deinits

bef958c

Refactor the writer to only yield Deques and rename the delegate to N…

031cff3

…IOAsyncWriterSinkDelegate

Review

7d3e7fa

FranzBusch force-pushed the fb-async-writer branch from f141fb8 to 7d3e7fa Compare September 20, 2022 11:57

Fix some warnings

90f2b39

FranzBusch requested a review from Lukasa September 20, 2022 12:50

FranzBusch added 2 commits September 20, 2022 15:18

Fix benchmark sendability

7aeeaa5

Remove Failure generic parameter and allow sending of an error throug…

627db1a

…h the Sink

Lukasa approved these changes Sep 21, 2022

View reviewed changes

Lukasa enabled auto-merge (squash) September 21, 2022 09:44

FranzBusch disabled auto-merge September 21, 2022 09:49

FranzBusch enabled auto-merge (squash) September 21, 2022 09:49

FranzBusch merged commit f144292 into apple:main Sep 21, 2022

FranzBusch deleted the fb-async-writer branch September 21, 2022 09:53

glbrntt reviewed Sep 21, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement a `NIOAsyncWriter` #2251

Implement a `NIOAsyncWriter` #2251

FranzBusch commented Aug 26, 2022

glbrntt left a comment

fabianfett left a comment

fabianfett Aug 30, 2022

FranzBusch Aug 30, 2022

fabianfett Aug 30, 2022 •

edited

Loading

glbrntt Aug 31, 2022

FranzBusch Aug 31, 2022

Lukasa Sep 7, 2022

fabianfett Aug 30, 2022

FranzBusch Aug 30, 2022

glbrntt Aug 31, 2022

Lukasa left a comment

glbrntt left a comment

Lukasa Sep 7, 2022

FranzBusch commented Sep 8, 2022

Lukasa Sep 16, 2022

FranzBusch Sep 20, 2022 •

edited

Loading

FranzBusch Sep 20, 2022

FranzBusch commented Sep 21, 2022

glbrntt left a comment

Implement a NIOAsyncWriter #2251

Implement a NIOAsyncWriter #2251

Conversation

FranzBusch commented Aug 26, 2022

Motivation

Modification

Result

glbrntt left a comment

Choose a reason for hiding this comment

fabianfett left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabianfett Aug 30, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Lukasa left a comment

Choose a reason for hiding this comment

glbrntt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FranzBusch commented Sep 8, 2022

Choose a reason for hiding this comment

FranzBusch Sep 20, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FranzBusch commented Sep 21, 2022

glbrntt left a comment

Choose a reason for hiding this comment

Implement a `NIOAsyncWriter` #2251

Implement a `NIOAsyncWriter` #2251

fabianfett Aug 30, 2022 •

edited

Loading

FranzBusch Sep 20, 2022 •

edited

Loading