Get NIOEmbedded clean under strict concurrency (#3030)

Motivation: NIOEmbedded is used all over NIO-land for testing various pieces of the infrastructure, and so requires a substantial audit for strict concurrency. Modifications: - Mark a few things Sendable. - Fix the tests, which actually did have some nasty bugs Result: Sendable-clean NIOEmbedded
apple · Dec 17, 2024 · 2d72ada · 2d72ada
1 parent 1a3229b
commit 2d72ada
Show file tree

Hide file tree

Showing 14 changed files with 224 additions and 128 deletions.
diff --git a/IntegrationTests/tests_04_performance/Thresholds/5.10.json b/IntegrationTests/tests_04_performance/Thresholds/5.10.json
@@ -1,8 +1,8 @@
 {
     "10000000_asyncsequenceproducer": 19,
     "1000000_asyncwriter": 1000050,
-    "1000_addHandlers": 43050,
-    "1000_addHandlers_sync": 36050,
+    "1000_addHandlers": 44050,
+    "1000_addHandlers_sync": 37050,
     "1000_addRemoveHandlers_handlercontext": 8050,
     "1000_addRemoveHandlers_handlername": 8050,
     "1000_addRemoveHandlers_handlertype": 8050,
@@ -11,7 +11,7 @@
     "1000_copying_bytebufferview_to_array": 1050,
     "1000_copying_circularbuffer_to_array": 1050,
     "1000_getHandlers": 8050,
-    "1000_getHandlers_sync": 34,
+    "1000_getHandlers_sync": 35,
     "1000_reqs_1_conn": 26400,
     "1000_rst_connections": 145050,
     "1000_tcpbootstraps": 3050,

diff --git a/IntegrationTests/tests_04_performance/Thresholds/5.9.json b/IntegrationTests/tests_04_performance/Thresholds/5.9.json
@@ -1,8 +1,8 @@
 {
     "10000000_asyncsequenceproducer": 19,
     "1000000_asyncwriter": 1000050,
-    "1000_addHandlers": 43050,
-    "1000_addHandlers_sync": 36050,
+    "1000_addHandlers": 44050,
+    "1000_addHandlers_sync": 37050,
     "1000_addRemoveHandlers_handlercontext": 8050,
     "1000_addRemoveHandlers_handlername": 8050,
     "1000_addRemoveHandlers_handlertype": 8050,
@@ -11,7 +11,7 @@
     "1000_copying_bytebufferview_to_array": 1050,
     "1000_copying_circularbuffer_to_array": 1050,
     "1000_getHandlers": 8050,
-    "1000_getHandlers_sync": 34,
+    "1000_getHandlers_sync": 35,
     "1000_reqs_1_conn": 26400,
     "1000_rst_connections": 147050,
     "1000_tcpbootstraps": 4050,

diff --git a/IntegrationTests/tests_04_performance/Thresholds/6.0.json b/IntegrationTests/tests_04_performance/Thresholds/6.0.json
@@ -1,8 +1,8 @@
 {
     "10000000_asyncsequenceproducer": 19,
     "1000000_asyncwriter": 1000050,
-    "1000_addHandlers": 43050,
-    "1000_addHandlers_sync": 36050,
+    "1000_addHandlers": 44050,
+    "1000_addHandlers_sync": 37050,
     "1000_addRemoveHandlers_handlercontext": 8050,
     "1000_addRemoveHandlers_handlername": 8050,
     "1000_addRemoveHandlers_handlertype": 8050,
@@ -11,7 +11,7 @@
     "1000_copying_bytebufferview_to_array": 1050,
     "1000_copying_circularbuffer_to_array": 1050,
     "1000_getHandlers": 8050,
-    "1000_getHandlers_sync": 34,
+    "1000_getHandlers_sync": 35,
     "1000_reqs_1_conn": 26400,
     "1000_rst_connections": 145050,
     "1000_tcpbootstraps": 3050,

diff --git a/IntegrationTests/tests_04_performance/Thresholds/nightly-6.0.json b/IntegrationTests/tests_04_performance/Thresholds/nightly-6.0.json
@@ -1,8 +1,8 @@
 {
     "10000000_asyncsequenceproducer": 19,
     "1000000_asyncwriter": 1000050,
-    "1000_addHandlers": 43050,
-    "1000_addHandlers_sync": 36050,
+    "1000_addHandlers": 44050,
+    "1000_addHandlers_sync": 37050,
     "1000_addRemoveHandlers_handlercontext": 8050,
     "1000_addRemoveHandlers_handlername": 8050,
     "1000_addRemoveHandlers_handlertype": 8050,
@@ -11,7 +11,7 @@
     "1000_copying_bytebufferview_to_array": 1050,
     "1000_copying_circularbuffer_to_array": 1050,
     "1000_getHandlers": 8050,
-    "1000_getHandlers_sync": 34,
+    "1000_getHandlers_sync": 35,
     "1000_reqs_1_conn": 26400,
     "1000_rst_connections": 145050,
     "1000_tcpbootstraps": 3050,

diff --git a/IntegrationTests/tests_04_performance/Thresholds/nightly-main.json b/IntegrationTests/tests_04_performance/Thresholds/nightly-main.json
@@ -1,8 +1,8 @@
 {
     "10000000_asyncsequenceproducer": 19,
     "1000000_asyncwriter": 1000050,
-    "1000_addHandlers": 43050,
-    "1000_addHandlers_sync": 36050,
+    "1000_addHandlers": 44050,
+    "1000_addHandlers_sync": 37050,
     "1000_addRemoveHandlers_handlercontext": 8050,
     "1000_addRemoveHandlers_handlername": 8050,
     "1000_addRemoveHandlers_handlertype": 8050,
@@ -11,7 +11,7 @@
     "1000_copying_bytebufferview_to_array": 1050,
     "1000_copying_circularbuffer_to_array": 1050,
     "1000_getHandlers": 8050,
-    "1000_getHandlers_sync": 34,
+    "1000_getHandlers_sync": 35,
     "1000_reqs_1_conn": 26400,
     "1000_rst_connections": 145050,
     "1000_tcpbootstraps": 3050,
@@ -34,10 +34,10 @@
     "execute_hop_10000_tasks": 0,
     "future_erase_result": 4050,
     "future_lots_of_callbacks": 53050,
-    "get_100000_headers_canonical_form": 700050,
-    "get_100000_headers_canonical_form_trimming_whitespace": 700050,
-    "get_100000_headers_canonical_form_trimming_whitespace_from_long_string": 700050,
-    "get_100000_headers_canonical_form_trimming_whitespace_from_short_string": 700050,
+    "get_100000_headers_canonical_form": 500050,
+    "get_100000_headers_canonical_form_trimming_whitespace": 500050,
+    "get_100000_headers_canonical_form_trimming_whitespace_from_long_string": 500050,
+    "get_100000_headers_canonical_form_trimming_whitespace_from_short_string": 500050,
     "modifying_1000_circular_buffer_elements": 0,
     "modifying_byte_buffer_view": 6050,
     "ping_pong_1000_reqs_1_conn": 319,

diff --git a/Package.swift b/Package.swift
@@ -105,7 +105,8 @@ let package = Package(
                 "_NIODataStructures",
                 swiftAtomics,
                 swiftCollections,
-            ]
+            ],
+            swiftSettings: strictConcurrencySettings
         ),
         .target(
             name: "NIOPosix",
@@ -429,7 +430,8 @@ let package = Package(
                 "NIOConcurrencyHelpers",
                 "NIOCore",
                 "NIOEmbedded",
-            ]
+            ],
+            swiftSettings: strictConcurrencySettings
         ),
         .testTarget(
             name: "NIOPosixTests",

diff --git a/Sources/NIOEmbedded/AsyncTestingChannel.swift b/Sources/NIOEmbedded/AsyncTestingChannel.swift
@@ -199,24 +199,28 @@ public final class NIOAsyncTestingChannel: Channel {
     /// `nil` because ``NIOAsyncTestingChannel``s don't have parents.
     public let parent: Channel? = nil
 
-    // This is only written once, from a single thread, and never written again, so it's _technically_ thread-safe. Most methods cannot safely
+    // These two variables are only written once, from a single thread, and never written again, so they're _technically_ thread-safe. Most methods cannot safely
     // be used from multiple threads, but `isActive`, `isOpen`, `eventLoop`, and `closeFuture` can all safely be used from any thread. Just.
+    #if compiler(>=5.10)
+    @usableFromInline
+    nonisolated(unsafe) var channelcore: EmbeddedChannelCore!
+    nonisolated(unsafe) private var _pipeline: ChannelPipeline!
+    #else
     @usableFromInline
     var channelcore: EmbeddedChannelCore!
+    private var _pipeline: ChannelPipeline!
+    #endif
 
-    /// Guards any of the getters/setters that can be accessed from any thread.
-    private let stateLock: NIOLock = NIOLock()
-
-    // Guarded by `stateLock`
-    private var _isWritable: Bool = true
-
-    // Guarded by `stateLock`
-    private var _localAddress: SocketAddress? = nil
-
-    // Guarded by `stateLock`
-    private var _remoteAddress: SocketAddress? = nil
+    private struct State {
+        var isWritable: Bool
+        var localAddress: SocketAddress?
+        var remoteAddress: SocketAddress?
+    }
 
-    private var _pipeline: ChannelPipeline!
+    /// Guards any of the getters/setters that can be accessed from any thread.
+    private let stateLock = NIOLockedValueBox(
+        State(isWritable: true, localAddress: nil, remoteAddress: nil)
+    )
 
     /// - see: `Channel._channelCore`
     public var _channelCore: ChannelCore {
@@ -231,35 +235,35 @@ public final class NIOAsyncTestingChannel: Channel {
     /// - see: `Channel.isWritable`
     public var isWritable: Bool {
         get {
-            self.stateLock.withLock { self._isWritable }
+            self.stateLock.withLockedValue { $0.isWritable }
         }
         set {
-            self.stateLock.withLock { () -> Void in
-                self._isWritable = newValue
+            self.stateLock.withLockedValue {
+                $0.isWritable = newValue
             }
         }
     }
 
     /// - see: `Channel.localAddress`
     public var localAddress: SocketAddress? {
         get {
-            self.stateLock.withLock { self._localAddress }
+            self.stateLock.withLockedValue { $0.localAddress }
         }
         set {
-            self.stateLock.withLock { () -> Void in
-                self._localAddress = newValue
+            self.stateLock.withLockedValue {
+                $0.localAddress = newValue
             }
         }
     }
 
     /// - see: `Channel.remoteAddress`
     public var remoteAddress: SocketAddress? {
         get {
-            self.stateLock.withLock { self._remoteAddress }
+            self.stateLock.withLockedValue { $0.remoteAddress }
         }
         set {
-            self.stateLock.withLock { () -> Void in
-                self._remoteAddress = newValue
+            self.stateLock.withLockedValue {
+                $0.remoteAddress = newValue
             }
         }
     }
@@ -283,8 +287,11 @@ public final class NIOAsyncTestingChannel: Channel {
     /// - Parameters:
     ///   - handler: The `ChannelHandler` to add to the `ChannelPipeline` before register.
     ///   - loop: The ``NIOAsyncTestingEventLoop`` to use.
-    public convenience init(handler: ChannelHandler, loop: NIOAsyncTestingEventLoop = NIOAsyncTestingEventLoop()) async
-    {
+    @preconcurrency
+    public convenience init(
+        handler: ChannelHandler & Sendable,
+        loop: NIOAsyncTestingEventLoop = NIOAsyncTestingEventLoop()
+    ) async {
         await self.init(handlers: [handler], loop: loop)
     }
 
@@ -295,8 +302,9 @@ public final class NIOAsyncTestingChannel: Channel {
     /// - Parameters:
     ///   - handlers: The `ChannelHandler`s to add to the `ChannelPipeline` before register.
     ///   - loop: The ``NIOAsyncTestingEventLoop`` to use.
+    @preconcurrency
     public convenience init(
-        handlers: [ChannelHandler],
+        handlers: [ChannelHandler & Sendable],
         loop: NIOAsyncTestingEventLoop = NIOAsyncTestingEventLoop()
     ) async {
         self.init(loop: loop)
@@ -671,3 +679,7 @@ extension NIOAsyncTestingChannel.LeftOverState: @unchecked Sendable {}
 @available(macOS 10.15, iOS 13.0, watchOS 6.0, tvOS 13.0, *)
 extension NIOAsyncTestingChannel.BufferState: @unchecked Sendable {}
 #endif
+
+// Synchronous options are never Sendable.
+@available(*, unavailable)
+extension NIOAsyncTestingChannel.SynchronousOptions: Sendable {}
diff --git a/Sources/NIOEmbedded/AsyncTestingEventLoop.swift b/Sources/NIOEmbedded/AsyncTestingEventLoop.swift
@@ -14,7 +14,13 @@
 
 #if canImport(Dispatch)
 import Atomics
+
+#if canImport(Darwin)
 import Dispatch
+#else
+@preconcurrency import Dispatch
+#endif
+
 import NIOConcurrencyHelpers
 import NIOCore
 import _NIODataStructures
@@ -125,7 +131,7 @@ public final class NIOAsyncTestingEventLoop: EventLoop, @unchecked Sendable {
         self.scheduledTasks.removeFirst { $0.id == taskID }
     }
 
-    private func insertTask<ReturnType>(
+    private func insertTask<ReturnType: Sendable>(
         taskID: UInt64,
         deadline: NIODeadline,
         promise: EventLoopPromise<ReturnType>,
@@ -152,7 +158,11 @@ public final class NIOAsyncTestingEventLoop: EventLoop, @unchecked Sendable {
 
     /// - see: `EventLoop.scheduleTask(deadline:_:)`
     @discardableResult
-    public func scheduleTask<T>(deadline: NIODeadline, _ task: @escaping () throws -> T) -> Scheduled<T> {
+    @preconcurrency
+    public func scheduleTask<T: Sendable>(
+        deadline: NIODeadline,
+        _ task: @escaping @Sendable () throws -> T
+    ) -> Scheduled<T> {
         let promise: EventLoopPromise<T> = self.makePromise()
         let taskID = self.scheduledTaskCounter.loadThenWrappingIncrement(ordering: .relaxed)
 
@@ -190,7 +200,8 @@ public final class NIOAsyncTestingEventLoop: EventLoop, @unchecked Sendable {
 
     /// - see: `EventLoop.scheduleTask(in:_:)`
     @discardableResult
-    public func scheduleTask<T>(in: TimeAmount, _ task: @escaping () throws -> T) -> Scheduled<T> {
+    @preconcurrency
+    public func scheduleTask<T: Sendable>(in: TimeAmount, _ task: @escaping @Sendable () throws -> T) -> Scheduled<T> {
         self.scheduleTask(deadline: self.now + `in`, task)
     }
 
@@ -230,7 +241,8 @@ public final class NIOAsyncTestingEventLoop: EventLoop, @unchecked Sendable {
 
     /// On an `NIOAsyncTestingEventLoop`, `execute` will simply use `scheduleTask` with a deadline of _now_. Unlike with the other operations, this will
     /// immediately execute, to eliminate a common class of bugs.
-    public func execute(_ task: @escaping () -> Void) {
+    @preconcurrency
+    public func execute(_ task: @escaping @Sendable () -> Void) {
         if self.inEventLoop {
             self.scheduleTask(deadline: self.now, task)
         } else {
@@ -359,7 +371,8 @@ public final class NIOAsyncTestingEventLoop: EventLoop, @unchecked Sendable {
     }
 
     /// - see: `EventLoop.shutdownGracefully`
-    public func shutdownGracefully(queue: DispatchQueue, _ callback: @escaping (Error?) -> Void) {
+    @preconcurrency
+    public func shutdownGracefully(queue: DispatchQueue, _ callback: @escaping @Sendable (Error?) -> Void) {
         self.queue.async {
             self._shutdownGracefully()
             queue.async {