Add `withInboundOutboud` to `NIOAsyncChannel` and deprecate deinit ba… #2589

FranzBusch · 2023-11-08T14:25:12Z

…sed 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.

FranzBusch · 2023-11-08T14:26:32Z

Sources/NIOCore/AsyncChannel/AsyncChannelOutboundWriterHandler.swift

-            break
-
-        case .closeOutput:
+        if self.isOutboundHalfClosureEnabled {


Call out for reviewers: We are still closing the output side if users are calling NIOAsyncWriter.finish and outbound half closure is enabled. This is not triggered by deinit based cleanup

glbrntt · 2023-11-09T10:58:11Z

Sources/NIOCore/AsyncChannel/AsyncChannel.swift

+    ///
+    /// - Parameter body: A closure that gets scoped access to the inbound and outbound.
+    public func withInboundOutbound<Result>(
+        _ body: (NIOAsyncChannelInboundStream<Inbound>, NIOAsyncChannelOutboundWriter<Outbound>) async throws -> Result


Please name these parameters (see https://www.swift.org/documentation/api-design-guidelines/#special-instructions)

This is a closure and we cannot add external argument labels.

I didn't ask for external label ;)

The instructions in the API guidelines say should have internal labels because they act as documentation and act as hints in auto completion etc.

Oh sorry, I misunderstood you then. Pushed a commit with internal labels.

glbrntt · 2023-11-09T10:59:11Z

Sources/NIOCore/AsyncChannel/AsyncChannel.swift

+    /// - Important: After this method returned the underlying ``Channel`` will be closed.
+    ///
+    /// - Parameter body: A closure that gets scoped access to the inbound and outbound.
+    public func withInboundOutbound<Result>(


withStreams?

Hm why? The only type called stream is the InboundStream. I was going between withInboundStreamAndOutboundWriter before but decided to follow the same logic where we renamed them to inbound and outbound

I just don't think the name is particularly clear.

I'm not sure withStreams is the right name either. I suggested "streams" because it's how I'd refer to inbound and outbound collectively.

Maybe the naming should indicate the semantics of what the function offers rather than what the closure provides to the caller? Thinking along the lines of "with auto closing streams", "with lifecycle managed streams" (I don't like these either but throwing them out anyway...)

Sources/NIOCore/AsyncChannel/AsyncChannel.swift

glbrntt · 2023-11-09T11:08:58Z

Sources/NIOCore/AsyncChannel/AsyncChannel.swift

+    public func withInbound<Result>(
+        _ body: (NIOAsyncChannelInboundStream<Inbound>) async throws -> Result
+    ) async throws -> Result where Outbound == Never{
+        try await self.withInboundOutbound { inbound, _ in
+            try await body(inbound)
+        }
+    }


Do we need this? Wondering if withStreams { inbound, _ in ... } is sufficient?

I first only had the method with both but in the case of the ServerBootstrap you have to spell withInboundOutbound { _, outbound in a lot and I personally found this very annoying.

glbrntt · 2023-11-09T11:55:50Z

Sources/NIOCore/AsyncSequences/NIOAsyncSequenceProducer.swift

@@ -139,17 +139,52 @@ public struct NIOAsyncSequenceProducer<
    /// - Parameters:
    ///   - elementType: The element type of the sequence.
    ///   - backPressureStrategy: The back-pressure strategy of the sequence.
+    ///   - finishOnDeinit: Indicates if ``NIOAsyncSequenceProducerDelegate/didTerminate()`` should be called on deinit. We do not recommend to rely on


Suggested change

/// - finishOnDeinit: Indicates if ``NIOAsyncSequenceProducerDelegate/didTerminate()`` should be called on deinit. We do not recommend to rely on

/// - finishOnDeinit: Indicates if ``NIOAsyncSequenceProducerDelegate/didTerminate()`` should be called on deinit. We do not recommend to relying on

This documentation is a bit misleading... Doesn't this toggle whether the source is finish()-ed when the source is deinit-ed. IMO that's the important behaviour to document. Any behavioural change to when didTerminate is called should also be documented though.

Right, I reworded the documentation. DidTerminate is still called at the same time just source.finish is not called anymore on deinit

glbrntt · 2023-11-09T12:39:29Z

Sources/NIOCore/AsyncSequences/NIOAsyncSequenceProducer.swift

-            @inlinable
-            deinit {
-                // We need to call finish here to resume any suspended continuation.
-                self._throwingSource.finish()
-            }


In the throwing producer we call through to sourceDeiniitialized here -- why are they different?

Because we store the throwing producer in here. So if we get deinited we will implicitly deinit the throwing one which will execute sourceDeinitialized.

Gotcha, thanks for explaining. As we no longer need deinit can this become a struct then?

glbrntt · 2023-11-09T12:42:12Z

Sources/NIOCore/AsyncSequences/NIOThrowingAsyncSequenceProducer.swift

+
+                    return .resumeContinuationWithFailureAndCallDidTerminate(continuation, nil)
+                } else {
+                    assertionFailure("Deinited NIOAsyncSequenceProducer.Source without finishing it first")


Given this can lead to leaking continuations should we raise the assertions to a preconditionFailure or deal with the continuations?

Good point. We probably should precondition here.

Sources/NIOCore/Docs.docc/swift-concurrency.md

…sed 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.

FranzBusch · 2023-11-10T15:11:52Z

@glbrntt Just renamed the withInboundOutbound to executeThenCloseChannel.

weissi · 2023-11-10T15:25:11Z

Sources/NIOCore/AsyncSequences/NIOAsyncWriter.swift

@@ -409,9 +444,14 @@ extension NIOAsyncWriter {
        @inlinable
        /* fileprivate */ internal init(


There are still two deinits in NIOAsyncWriter that haven't been fixed here: https://github.com/apple/swift-nio/pull/2582/files#diff-7914f7c4968a26adfb4f6be410d7a1c12a72a35ed7bfd1c3a62f08e08f4b37f1R178-R376

Oh I actually forgot to call the new method in one of those. The other one was already correct. Both will unconditionally call through to the state machine and we make the decision what happens in the state machine.

Can we not flip this around to make this reviewable and very clear: Unless the user tells us to, we're not gonna do anything (but assertions) in deinit. If deinit seems to do a thing (like calling something else) then you lose local reasoning and have to follow all possible trails to check if that code is actually deinit-safe Swift code. If it's just

deinit { if self.yoloMode { self.stateMachine.doStuff() } }

then it's super clear that nothing bad is going to happen

deinit shouldn't drive state machines, it shouldn't do anything if at all possible.

I disagree with this. I prefer to have all reasoning in the state machine and only have a single place that takes decisions. Furthermore, we must involve the state machine anyhow because we need to understand if we are already finished. I would prefer to keep it like I implemented here.

I do understand what you're saying but deinit is a very special case. Code that's in there runs under special rules (must not publish self or else UB or crash; may run on arbitrary thread; ...). If you want anybody to understand this code you have to have it in deinit.

Imagine this deinit

deinit { self.stateMachine.finish() }

or

deinit { self.finish() }

looks nice and simple. If a future PR changes func finish() in the state machine or on self, then the reviewer of that PR will have no clue that this code may be called from deinit. Normally, the rules are simple: No code is called from deinit. If we bend that rule it's really important that this is visible in every diff that touches it. Hiding it in the state machine might have dire consequences that can be ~impossible to fix. If Swift had a mandatory @mayBeCalledFromDeinit attribute this were different but we don't

That's why we are not calling finish in the state machine but specific methods called writerDeinitialized or sinkDenitialized. This makes it very explicit to understand where this call comes from. IMO that's better than calling out to the state machine in deinit to check for a specific state and then make the decision what to do outside of the state machine. That's just blurs the line where state logic happens.

Sources/NIOCore/AsyncSequences/NIOAsyncWriter.swift

weissi · 2023-11-13T10:27:49Z

Sources/NIOCore/AsyncSequences/NIOAsyncWriter.swift

        }

        @inlinable
        deinit {
-            _storage.writerDeinitialized()
+            if !self._finishOnDeinit && !self._storage.isWriterFinished {
+                preconditionFailure("Deinited NIOAsyncWriter without calling finish()")


weissi

thanks so much

Sources/NIOCore/AsyncSequences/NIOAsyncSequenceProducer.swift

glbrntt · 2023-11-13T10:36:57Z

Benchmarks/Benchmarks/NIOPosixBenchmarks/TCPEchoAsyncChannel.swift

-            for try await connectionChannel in serverChannel.inbound {
-                for try await inboundData in connectionChannel.inbound {
-                    try await connectionChannel.outbound.write(inboundData)
+            try await serverChannel.withInbound { serverChannelInbound in


executeThenClose

glbrntt · 2023-11-13T10:37:07Z

Benchmarks/Benchmarks/NIOPosixBenchmarks/TCPEchoAsyncChannel.swift

-                    try await connectionChannel.outbound.write(inboundData)
+            try await serverChannel.withInbound { serverChannelInbound in
+                for try await connectionChannel in serverChannelInbound {
+                    try await connectionChannel.withInboundOutbound { connectionChannelInbound, connectionChannelOutbound in


executeThenClose (and elsewhere)

glbrntt · 2023-11-13T10:46:06Z

Sources/NIOCore/AsyncChannel/AsyncChannel.swift

+    /// - Parameter body: A closure that gets scoped access to the inbound.
+    public func executeThenClose<Result>(
+        _ body: (_ inbound: NIOAsyncChannelInboundStream<Inbound>) async throws -> Result
+    ) async throws -> Result where Outbound == Never{


Suggested change

) async throws -> Result where Outbound == Never{

) async throws -> Result where Outbound == Never {

Motivation: The performance test binary was crashing ever since apple#2589 added the crash on deinit flow. Crashes here are preventing us from using the performance tester. Modifications: Correctly clean up the async writer. Result: The writer is cleaned up now.

Motivation: The performance test binary was crashing ever since #2589 added the crash on deinit flow. Crashes here are preventing us from using the performance tester. Modifications: Correctly clean up the async writer. Result: The writer is cleaned up now.

FranzBusch added the 🆕 semver/minor Adds new public API. label Nov 8, 2023

FranzBusch requested a review from glbrntt November 8, 2023 14:25

FranzBusch commented Nov 8, 2023

View reviewed changes

FranzBusch requested a review from weissi November 8, 2023 14:26

glbrntt requested changes Nov 9, 2023

View reviewed changes

FranzBusch requested a review from glbrntt November 9, 2023 18:22

FranzBusch added 3 commits November 10, 2023 09:36

Review comments

ad80443

Internal labels for closure arguments

ea3c97c

FranzBusch force-pushed the fb-scope-async-channel branch from 5ee3dd4 to ea3c97c Compare November 10, 2023 09:36

Rename to executeThenCloseChannel

2803413

weissi reviewed Nov 10, 2023

View reviewed changes

Actually call sinkDeinitialized

78d8870

FranzBusch force-pushed the fb-scope-async-channel branch from 8dda838 to 78d8870 Compare November 10, 2023 15:36

weissi reviewed Nov 10, 2023

View reviewed changes

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

FranzBusch added 2 commits November 10, 2023 15:51

Change preconditions

edbf626

Move logic to deinits

4d67404

weissi reviewed Nov 13, 2023

View reviewed changes

weissi approved these changes Nov 13, 2023

View reviewed changes

glbrntt reviewed Nov 13, 2023

View reviewed changes

FranzBusch requested a review from glbrntt November 13, 2023 10:43

glbrntt reviewed Nov 13, 2023

View reviewed changes

FranzBusch force-pushed the fb-scope-async-channel branch from 6cacc53 to b595ce6 Compare November 13, 2023 11:02

Rename to executeThenClose and review nits

8aea251

FranzBusch force-pushed the fb-scope-async-channel branch from b595ce6 to 8aea251 Compare November 13, 2023 11:06

FranzBusch requested a review from glbrntt November 13, 2023 11:06

glbrntt approved these changes Nov 13, 2023

View reviewed changes

FranzBusch enabled auto-merge (squash) November 13, 2023 11:27

FranzBusch merged commit 118de50 into apple:main Nov 13, 2023

FranzBusch deleted the fb-scope-async-channel branch November 13, 2023 11:37

Lukasa mentioned this pull request Jan 2, 2024

Fix the broken performance test binary #2619

Merged

0xTim mentioned this pull request Jan 16, 2025

Add ChannelPipeline.SynchronousOperations.Position #3065

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `withInboundOutboud` to `NIOAsyncChannel` and deprecate deinit ba… #2589

Add `withInboundOutboud` to `NIOAsyncChannel` and deprecate deinit ba… #2589

FranzBusch commented Nov 8, 2023

FranzBusch Nov 8, 2023

glbrntt Nov 9, 2023

FranzBusch Nov 9, 2023

glbrntt Nov 10, 2023

FranzBusch Nov 10, 2023

glbrntt Nov 9, 2023

FranzBusch Nov 9, 2023

glbrntt Nov 10, 2023

glbrntt Nov 9, 2023

FranzBusch Nov 9, 2023

glbrntt Nov 9, 2023

glbrntt Nov 9, 2023

FranzBusch Nov 9, 2023

glbrntt Nov 9, 2023

FranzBusch Nov 9, 2023

glbrntt Nov 10, 2023

glbrntt Nov 9, 2023

FranzBusch Nov 9, 2023

FranzBusch commented Nov 10, 2023

weissi Nov 10, 2023

FranzBusch Nov 10, 2023

weissi Nov 10, 2023

weissi Nov 10, 2023

FranzBusch Nov 10, 2023

weissi Nov 10, 2023 •

edited

Loading

FranzBusch Nov 10, 2023

weissi Nov 13, 2023

weissi left a comment

glbrntt Nov 13, 2023

glbrntt Nov 13, 2023

glbrntt Nov 13, 2023

	/// - finishOnDeinit: Indicates if ``NIOAsyncSequenceProducerDelegate/didTerminate()`` should be called on deinit. We do not recommend to rely on
	/// - finishOnDeinit: Indicates if ``NIOAsyncSequenceProducerDelegate/didTerminate()`` should be called on deinit. We do not recommend to relying on

		@@ -409,9 +444,14 @@ extension NIOAsyncWriter {
		@inlinable
		/* fileprivate */ internal init(

	) async throws -> Result where Outbound == Never{
	) async throws -> Result where Outbound == Never {

Add withInboundOutboud to NIOAsyncChannel and deprecate deinit ba… #2589

Add withInboundOutboud to NIOAsyncChannel and deprecate deinit ba… #2589

Conversation

FranzBusch commented Nov 8, 2023

Motivation

Modifications

Result

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

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

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

FranzBusch commented Nov 10, 2023

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

weissi Nov 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weissi left a 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

Add `withInboundOutboud` to `NIOAsyncChannel` and deprecate deinit ba… #2589

Add `withInboundOutboud` to `NIOAsyncChannel` and deprecate deinit ba… #2589

weissi Nov 10, 2023 •

edited

Loading