Sendability warnings #417

rnro · 2023-09-04T09:14:18Z

Motivation:

To address warnings revealed with
.enableExperimentalFeature("StrictConcurrency=complete"), in Swift 5.8

Modifications:

Numerous protections of state in tests
Make the multiplexer instances explicitly loop bound
Update channel initializers to be marked as Sendable
Mark error backing storage types as unchecked sendable

Result:

Better thread safety

glbrntt · 2023-09-04T13:10:07Z

Sources/NIOHTTP2/HTTP2ChannelHandler+InlineStreamMultiplexer.swift

@@ -227,20 +239,31 @@ extension NIOHTTP2Handler {
    /// `OutboundStreamOutput`. This type may be `HTTP2Frame` or changed to any other type.
    @available(macOS 10.15, iOS 13.0, watchOS 6.0, tvOS 13.0, *)
    @_spi(AsyncChannel)
-    public struct AsyncStreamMultiplexer<InboundStreamOutput> {
-        private let inlineStreamMultiplexer: InlineStreamMultiplexer
+    public struct AsyncStreamMultiplexer<InboundStreamOutput>: @unchecked Sendable {


Aren't all the properties of this type Sendable?

Yep, marked as Sendable

Sources/NIOHTTP2/HTTP2ChannelHandler+InlineStreamMultiplexer.swift

glbrntt · 2023-09-04T13:13:14Z

Sources/NIOHTTP2/HTTP2ChannelHandler.swift

@@ -1078,8 +1082,9 @@ extension NIOHTTP2Handler {
                return try self.syncMultiplexer()
            }
        } else {
+            let loopBoundSelf = NIOLoopBound(self, eventLoop: self.eventLoop!)


We can't do this: NIOLoopBound preconditions that it's on the right EL on init and we know we're not on the EL here as we just checked.

Yeah, this is the canonical use of UnsafeTransfer.

glbrntt · 2023-09-04T13:19:03Z

Sources/NIOHTTP2/HTTP2PipelineHelpers.swift

+                let loopBoundMultiplexer = NIOLoopBound(multiplexer, eventLoop: self.eventLoop)
+                let loopBoundHandlers = NIOLoopBound(handlers, eventLoop: self.eventLoop)
+                let loopBoundPosition = NIOLoopBound(position, eventLoop: self.eventLoop)


We know we're not on the EL here so these will fail preconditions on init.

Rather than moving the handlers onto the EL we can just create them on the EL. The pipeline position is super awkward though because it isn't Sendable. I'm not sure what the solution there is (this is a problem NIO needs to solve).

I've moved the handler creation but left the warning for this and other use of position rather than trying to hide it because it truly is not sendable.

glbrntt · 2023-09-04T13:22:19Z

Sources/NIOHTTP2/HTTP2PipelineHelpers.swift

+            let loopBoundStreamDelegate = NIOLoopBound(streamDelegate, eventLoop: self.eventLoop)
+            let loopBoundPosition = NIOLoopBound(position, eventLoop: self.eventLoop)


We know we're not on the EL here: these will precondition failure on init.

I've made it a requirement that the streamDelegate be Sendable and left the position warning there.

glbrntt · 2023-09-04T13:22:47Z

Sources/NIOHTTP2/HTTP2PipelineHelpers.swift

@@ -460,7 +483,7 @@ extension Channel {
            }
        } else {
            return self.eventLoop.submit {
-                return try self.pipeline.syncOperations.configureAsyncHTTP2Pipeline(
+                return try self.pipeline.syncOperations .configureAsyncHTTP2Pipeline(


Suggested change

return try self.pipeline.syncOperations .configureAsyncHTTP2Pipeline(

return try self.pipeline.syncOperations.configureAsyncHTTP2Pipeline(

Lukasa · 2023-09-04T15:06:31Z

Sources/NIOHTTP2/HTTP2ChannelHandler+InlineStreamMultiplexer.swift

@@ -167,11 +167,15 @@ extension NIOHTTP2Handler {
    public struct StreamMultiplexer: @unchecked Sendable {


At this point do we need @unchecked?

Lukasa · 2023-09-04T15:15:05Z

Sources/NIOHTTP2/HTTP2ChannelHandler+InlineStreamMultiplexer.swift

@@ -167,11 +167,15 @@ extension NIOHTTP2Handler {
    public struct StreamMultiplexer: @unchecked Sendable {
        // '@unchecked Sendable' because this state is not intrinsically `Sendable`
        // but it is only accessed in `createStreamChannel` which executes the work on the right event loop
-        private let inlineStreamMultiplexer: InlineStreamMultiplexer
+
+        private let inlineStreamMultiplexer: NIOLoopBound<InlineStreamMultiplexer>


I don't think NIOLoopBound is the right approach here. There isn't a particular requirement to force the event loop dance out this far, I don't think. In practice we're calling an API that is thread-safe inside InlineStreamMultiplexer, so I wonder if using a lens type might be better.

I have made InlineStreamMultiplexerSendableView for this purpose

Sources/NIOHTTP2/HTTP2ChannelHandler+InlineStreamMultiplexer.swift

Lukasa · 2023-09-04T15:16:15Z

Sources/NIOHTTP2/HTTP2ChannelHandler.swift

@@ -103,7 +103,7 @@ public final class NIOHTTP2Handler: ChannelDuplexHandler {

    /// The delegate for (de)multiplexing inbound streams.
    private var inboundStreamMultiplexerState: InboundStreamMultiplexerState
-    internal var inboundStreamMultiplexer: InboundStreamMultiplexer? {
+    internal var inboundStreamMultiplexer: NIOLoopBound<InboundStreamMultiplexer>? {


Why does this need to be a NIOLoopBound?

It doesn't, removed.

Lukasa · 2023-09-04T15:18:18Z

Sources/NIOHTTP2/HTTP2ChannelHandler.swift

@@ -1078,8 +1082,9 @@ extension NIOHTTP2Handler {
                return try self.syncMultiplexer()
            }
        } else {
+            let loopBoundSelf = NIOLoopBound(self, eventLoop: self.eventLoop!)


Yeah, this is the canonical use of UnsafeTransfer.

Lukasa · 2023-09-04T15:19:28Z

Sources/NIOHTTP2/HTTP2Error.swift

@@ -467,7 +467,9 @@ public enum NIOHTTP2Errors {
            }
        }

-        private final class Storage: Equatable {
+        private final class Storage: Equatable, @unchecked Sendable {


I mean, this type is very trivially not Sendable. Can it be made to be? Same for the other changes here.

I'd put the annotation in the wrong place, I've moved it to the broader error type.

glbrntt · 2023-09-11T09:51:35Z

Sources/NIOHPACK/HPACKErrors.swift

@@ -124,3 +123,5 @@ public enum NIOHPACKErrors {
        public init() {}
    }
 }
+
+public protocol Foo: Collection, Sendable {}


glbrntt · 2023-09-11T09:58:33Z

Sources/NIOHTTP2/HTTP2CommonInboundStreamMultiplexer.swift

@@ -546,3 +561,35 @@ extension AsyncThrowingStream {
    }
 }
 #endif
+
+struct HTTP2CommonInboundStreamMultiplexerSendableView: @unchecked Sendable {


We usually nest views inside the type they are viewing, e.g. HTTP2CommonInboundStreamMultiplexer.SendableView

I think that actually I didn't end up using this type, so I'll remove it. I did make a different view to which I'll apply this tip though.

glbrntt · 2023-09-11T11:43:13Z

Sources/NIOHTTP2/HTTP2Error.swift

@@ -1572,7 +1574,9 @@ public enum NIOHTTP2Errors {
    /// meaningfully be `Equatable`, so they aren't. There's also no additional location information: that's
    /// provided by the base error.
    public struct StreamError: Error {
-        private final class Storage {
+        private final class Storage: @unchecked Sendable {


The storage isn't sendable, but StreamError is @unchecked Sendable because it does the CoW dance when modifying storage.

glbrntt · 2023-09-11T11:44:45Z

Sources/NIOHTTP2/HTTP2Error.swift

@@ -1690,7 +1694,7 @@ private func _location(file: String, line: UInt) -> String {
    return "\(file):\(line)"
 }

-private final class StringAndLocationStorage: Equatable {
+private final class StringAndLocationStorage: Equatable,  @unchecked Sendable {


This shouldn't be Sendable, it's a class with mutable properties. It can be used in a Sendable type if the caller does the appropriate CoW dance though.

glbrntt · 2023-09-11T11:46:39Z

Sources/NIOHTTP2/HTTP2PipelineHelpers.swift

+        } catch {
+            return self.eventLoop.makeFailedFuture(error)
+        }


Aim to keep do-catch blocks as small as possible. Here we should scope it to the if self.eventLoop.inEventLoop block.

Ah, I missed that self.eventLoop.submit handles the thrown error so I thought both branches could throw and wanted to avoid duplication.

glbrntt · 2023-09-11T11:48:34Z

Sources/NIOHTTP2/HTTP2PipelineHelpers.swift

+        var handlers = [ChannelHandler]()
+        handlers.reserveCapacity(2)  // Update this if we need to add more handlers, to avoid unnecessary reallocation.
+
+        handlers.append(NIOHTTP2Handler(mode: mode, initialSettings: initialLocalSettings))
+        let multiplexer = HTTP2StreamMultiplexer(mode: mode, channel: channel, targetWindowSize: targetWindowSize, inboundStreamInitializer: inboundStreamInitializer)
+        handlers.append(multiplexer)
+
+        try self.addHandlers(handlers, position: position)


Given we're doing synchronous operations here we can actually save an allocation by avoiding the handlers array and just calling addHandler multiple times.

Motivation: To address warnings revealed with `.enableExperimentalFeature("StrictConcurrency=complete"),` in Swift 5.8 Modifications: * Numerous protections of state in tests * Make the multiplexer instances explicitly loop bound * Update channel initializers to be marked as Sendable * Mark error backing storage types as unchecked sendable Result: Better thread safety

rnro · 2023-09-11T15:18:22Z

I squashed all of the reviewed changes into one commit and merged the changes from main into this branch to try and highlight any new changes.

glbrntt · 2023-09-13T09:53:35Z

Sources/NIOHTTP2/HTTP2PipelineHelpers.swift


-        return self.pipeline.addHandlers(handlers, position: position).map { multiplexer }
+        if self.eventLoop.inEventLoop {
+            do {


FYI eventLoop has a makeCompletedFuture function which takes a closure which returns a Result type. It's effectively the Result-based counterpart to makeSucceededFuture and makeFailedFuture and allows you to avoid doing the do-catch block here.

glbrntt · 2023-09-13T09:55:38Z

Sources/NIOHTTP2/HTTP2PipelineHelpers.swift

        configurator: @escaping NIOChannelInitializer
    ) -> EventLoopFuture<Void> {
+        let loopBoundStreamDelegate = NIOLoopBound(streamDelegate, eventLoop: self.eventLoop)


Is this correct? This pipeline helpers isn't synchronous so we can't make the guarantee that we'll be in the EL at this point. Also I think stream delegate is Sendable now anyway isn't it?

It is indeed sendable, good spot. I don't know if this came up in the rebase or if it was just an outstanding issue.

…2 into sendability_warnings

Lukasa · 2023-09-18T10:41:00Z

Sources/NIOHTTP2/HTTP2ChannelHandler+InlineStreamMultiplexer.swift

+        }
+
+        internal func createStreamChannel(promise: EventLoopPromise<Channel>?, _ streamStateInitializer: @escaping NIOHTTP2Handler.StreamInitializer) {
+            self.inlineStreamMultiplexer.createStreamChannel(promise: promise, streamStateInitializer)


I'm a bit confused: is this method thread-safe, or is the usage point protecting it?

Good spot. It's intended that the method is thread-safe. I've moved the event loop dance inside this method to make that the case.

Lukasa · 2023-09-18T10:41:15Z

Sources/NIOHTTP2/HTTP2ChannelHandler+InlineStreamMultiplexer.swift

+            if self.eventLoop.inEventLoop {
+                self.inlineStreamMultiplexer.createStreamChannel(promise: promise, streamStateInitializer)
+            } else {
+                self.eventLoop.execute {


See my comment on the SendableView.

Lukasa · 2023-09-18T10:45:25Z

Sources/NIOHTTP2/HTTP2ChannelHandler.swift

+extension UnsafeTransfer: @unchecked Sendable {}
+
+extension UnsafeTransfer: Equatable where Wrapped: Equatable {}
+extension UnsafeTransfer: Hashable where Wrapped: Hashable {}


Can we put this in its own file rather than smuggling it into here?

Lukasa · 2023-09-18T10:46:21Z

Sources/NIOHTTP2/HTTP2CommonInboundStreamMultiplexer.swift

                inboundStreamStateInitializer: .includesStreamID(nil)
            )
-            self.streams[streamID] = channel
+            loopBounds.value.0.streams[streamID] = channel


Repeatedly unpacking the loop bounds here kinda sucks: can we unwrap them at the top of the execute { } block and then use their names?

Lukasa · 2023-09-21T13:53:00Z

Sources/NIOHTTP2/HTTP2ChannelHandler+InlineStreamMultiplexer.swift

-    public struct AsyncStreamMultiplexer<InboundStreamOutput> {
-        private let inlineStreamMultiplexer: InlineStreamMultiplexer
+    public struct AsyncStreamMultiplexer<InboundStreamOutput: Sendable>: Sendable {
+        private let inlineStreamMultiplexer: NIOLoopBound<InlineStreamMultiplexer>


Is there a reason we don't use the sendable view here?

I don't think so, an oversight. I have used it here and fleshed out its API a bit more to mirror the underlying multiplexer.

* HTTP2StreamMultiplexer stream creation previously called down to underlying code which assumed it was always on the correct event loop however that wasn't the case via this public API. I have introduced the event loop dance. * Remove the use of NIOLoopBound in cases where we know we are on the correct event loop and unsafe transfer is more applicable.

Calls to `createStreamChannel` now always go via a `SendableView` lens type which serializes access on an event loop with an unconditional hop. This combines the serialization with the unconditional hop which was previously performed at the `HTTP2CommonInboundStreamMultiplexer` level, in some cases saving a hop. The `SendableView` is performed at the higher multiplexer levels rather than at the 'common' level because the higher levels pass in a `self` to lower calls which would otherwise need to be transferred in to the event loop hop.

FranzBusch

In general this looks good to me. I would leave the final review to @Lukasa

FranzBusch · 2023-11-01T13:52:28Z

Sources/NIOHTTP2/HTTP2StreamChannel.swift

@@ -146,7 +146,9 @@ private enum HTTP2StreamData {
 }

 @usableFromInline
-final class HTTP2StreamChannel: Channel, ChannelCore {
+final class HTTP2StreamChannel: Channel, ChannelCore, @unchecked Sendable {
+    // @unchecked Sendable because the only mutable state is `_pipeline` which is only modified in init


That's not fully correct there are a bunch of methods on the ChannelCore that must be called on the EL such as write0. However, nobody should really call those methods.

rnro · 2023-11-01T14:29:14Z

I believe that the discovered API breakages are okay, just to expand on that.
Add sendable requirement to generic parameters:

  💔 API breakage: struct NIOHTTP2Handler.AsyncStreamMultiplexer has generic signature change from <InboundStreamOutput> to <InboundStreamOutput where InboundStreamOutput : Swift.Sendable>
  💔 API breakage: accessor NIOHTTP2Handler.AsyncStreamMultiplexer.inbound.Get() has generic signature change from <InboundStreamOutput> to <InboundStreamOutput where InboundStreamOutput : Swift.Sendable>
  💔 API breakage: func NIOHTTP2Handler.AsyncStreamMultiplexer.openStream(_:) has generic signature change from <InboundStreamOutput, Output where Output : Swift.Sendable> to <InboundStreamOutput, Output where InboundStreamOutput : Swift.Sendable, Output : Swift.Sendable>
  💔 API breakage: struct NIOHTTP2AsyncSequence has generic signature change from <Output> to <Output where Output : Swift.Sendable>
  💔 API breakage: struct NIOHTTP2AsyncSequence.AsyncIterator has generic signature change from <Output> to <Output where Output : Swift.Sendable>
  💔 API breakage: typealias NIOHTTP2AsyncSequence.AsyncIterator.Element has generic signature change from <Output> to <Output where Output : Swift.Sendable>
  💔 API breakage: func NIOHTTP2AsyncSequence.AsyncIterator.next() has generic signature change from <Output> to <Output where Output : Swift.Sendable>
  💔 API breakage: typealias NIOHTTP2AsyncSequence.Element has generic signature change from <Output> to <Output where Output : Swift.Sendable>
  💔 API breakage: func NIOHTTP2AsyncSequence.makeAsyncIterator() has generic signature change from <Output> to <Output where Output : Swift.Sendable>
  💔 API breakage: protocol NIOHTTP2StreamDelegate has generic signature change from  to <Self : Swift.Sendable>

Add sendable requirement:

  💔 API breakage: protocol NIOHTTP2StreamDelegate has added inherited protocol Sendable

Change from explicit closure type to equivalent type alias:

  💔 API breakage: constructor HTTP2StreamMultiplexer.init(mode:channel:targetWindowSize:inboundStreamStateInitializer:) has parameter 3 type change from ((NIOCore.Channel, NIOHTTP2.HTTP2StreamID) -> NIOCore.EventLoopFuture<Swift.Void>)? to NIOHTTP2.NIOChannelInitializerWithStreamID?
  💔 API breakage: constructor HTTP2StreamMultiplexer.init(mode:channel:targetWindowSize:outboundBufferSizeHighWatermark:outboundBufferSizeLowWatermark:inboundStreamStateInitializer:) has parameter 5 type change from ((NIOCore.Channel, NIOHTTP2.HTTP2StreamID) -> NIOCore.EventLoopFuture<Swift.Void>)? to NIOHTTP2.NIOChannelInitializerWithStreamID?
  💔 API breakage: func Channel.configureHTTP2Pipeline(mode:initialLocalSettings:position:inboundStreamStateInitializer:) has parameter 3 type change from ((NIOCore.Channel, NIOHTTP2.HTTP2StreamID) -> NIOCore.EventLoopFuture<Swift.Void>)? to NIOHTTP2.NIOChannelInitializerWithStreamID?
  💔 API breakage: func Channel.configureHTTP2Pipeline(mode:initialLocalSettings:position:targetWindowSize:inboundStreamStateInitializer:) has parameter 4 type change from ((NIOCore.Channel, NIOHTTP2.HTTP2StreamID) -> NIOCore.EventLoopFuture<Swift.Void>)? to NIOHTTP2.NIOChannelInitializerWithStreamID?

Change from explicit closure type to equivalent type alias:

  💔 API breakage: constructor HTTP2StreamMultiplexer.init(mode:channel:targetWindowSize:outboundBufferSizeHighWatermark:outboundBufferSizeLowWatermark:inboundStreamInitializer:) has parameter 5 type change from ((NIOCore.Channel) -> NIOCore.EventLoopFuture<Swift.Void>)? to NIOHTTP2.NIOChannelInitializer?
  💔 API breakage: func Channel.configureHTTP2Pipeline(mode:initialLocalSettings:position:targetWindowSize:inboundStreamInitializer:) has parameter 4 type change from ((NIOCore.Channel) -> NIOCore.EventLoopFuture<Swift.Void>)? to NIOHTTP2.NIOChannelInitializer?
  💔 API breakage: func Channel.configureCommonHTTPServerPipeline(h2ConnectionChannelConfigurator:_:) has parameter 0 type change from ((NIOCore.Channel) -> NIOCore.EventLoopFuture<Swift.Void>)? to NIOHTTP2.NIOChannelInitializer?
  💔 API breakage: func Channel.configureCommonHTTPServerPipeline(h2ConnectionChannelConfigurator:targetWindowSize:_:) has parameter 0 type change from ((NIOCore.Channel) -> NIOCore.EventLoopFuture<Swift.Void>)? to NIOHTTP2.NIOChannelInitializer?
  💔 API breakage: func Channel.configureCommonHTTPServerPipeline(connectionConfiguration:streamConfiguration:streamDelegate:h2ConnectionChannelConfigurator:configurator:) has parameter 3 type change from ((NIOCore.Channel) -> NIOCore.EventLoopFuture<Swift.Void>)? to NIOHTTP2.NIOChannelInitializer?

rnro requested a review from glbrntt September 4, 2023 09:14

glbrntt requested changes Sep 4, 2023

View reviewed changes

Lukasa reviewed Sep 4, 2023

View reviewed changes

rnro force-pushed the sendability_warnings branch from f3faf6b to da4f322 Compare September 6, 2023 07:48

rnro requested review from glbrntt and Lukasa September 6, 2023 08:57

glbrntt reviewed Sep 11, 2023

View reviewed changes

rnro force-pushed the sendability_warnings branch from 3730e9a to 823875d Compare September 11, 2023 14:56

Merge branch 'main' into sendability_warnings

2df3fd3

rnro force-pushed the sendability_warnings branch from f8ff28b to 2df3fd3 Compare September 11, 2023 15:33

reduce alloc allowance

b12bff7

rnro requested a review from glbrntt September 11, 2023 15:51

reduce alloc allowance

3616f0e

glbrntt reviewed Sep 13, 2023

View reviewed changes

rnro added 2 commits September 14, 2023 09:30

review comments

939759e

Merge branch 'sendability_warnings' of github.com:rnro/swift-nio-http…

ceba056

…2 into sendability_warnings

rnro requested a review from glbrntt September 14, 2023 08:56

glbrntt approved these changes Sep 14, 2023

View reviewed changes

Lukasa added the semver/minor Adds new public API. label Sep 18, 2023

Lukasa reviewed Sep 18, 2023

View reviewed changes

review comments

3b861c5

rnro requested a review from Lukasa September 21, 2023 09:57

Lukasa reviewed Sep 21, 2023

View reviewed changes

expand use of InlineStreamMultiplexer.SendableView

6a08241

rnro requested a review from Lukasa September 21, 2023 15:45

Merge branch 'main' into sendability_warnings

fe57f5e

Lukasa mentioned this pull request Oct 27, 2023

Add Sendable annotations to new async APIs #427

Closed

Merge branch 'main' into sendability_warnings

7d47eb1

rnro added 3 commits October 30, 2023 10:28

update alloc counts

0482873

FranzBusch approved these changes Nov 1, 2023

View reviewed changes

glbrntt enabled auto-merge (squash) November 1, 2023 14:30

glbrntt disabled auto-merge November 1, 2023 14:36

glbrntt merged commit 285e444 into apple:main Nov 1, 2023
7 of 8 checks passed

rnro deleted the sendability_warnings branch November 1, 2023 14:38

		let loopBoundStreamDelegate = NIOLoopBound(streamDelegate, eventLoop: self.eventLoop)
		let loopBoundPosition = NIOLoopBound(position, eventLoop: self.eventLoop)

	return try self.pipeline.syncOperations .configureAsyncHTTP2Pipeline(
	return try self.pipeline.syncOperations.configureAsyncHTTP2Pipeline(

		@@ -167,11 +167,15 @@ extension NIOHTTP2Handler {
		public struct StreamMultiplexer: @unchecked Sendable {

Sendability warnings #417

Sendability warnings #417

Conversation

rnro commented Sep 4, 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

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

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

rnro commented Sep 11, 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rnro commented Nov 1, 2023