Delay stream creation until the next loop tick (#416)

Motivation: A recent change allowed stream creation to run immediately if the caller was on the correct event loop. Normally that's fine, however, it opens up a code path where streams can become activated twice which leads to a crash. Modifications: - Delay stream creation until the next loop tick - Add a loop tick to tests which now require it Result: Avoid re-entrantly activating streams twice.
apple · Aug 23, 2023 · 92afa4f · 92afa4f
1 parent 2bf5733
commit 92afa4f
Show file tree

Hide file tree

Showing 8 changed files with 60 additions and 50 deletions.
diff --git a/Sources/NIOHTTP2/HTTP2CommonInboundStreamMultiplexer.swift b/Sources/NIOHTTP2/HTTP2CommonInboundStreamMultiplexer.swift
@@ -312,12 +312,11 @@ extension HTTP2CommonInboundStreamMultiplexer {
         promise: EventLoopPromise<Output>?,
         _ streamStateInitializer: @escaping NIOChannelInitializerWithOutput<Output>
     ) {
-        if self.channel.eventLoop.inEventLoop {
+        // Always create streams channels on the next event loop tick. This avoids re-entrancy
+        // issues where handlers interposed between the two HTTP/2 handlers could create streams
+        // in channel active which become activated twice.
+        self.channel.eventLoop.execute {
             self._createStreamChannel(multiplexer, promise, streamStateInitializer)
-        } else {
-            self.channel.eventLoop.execute {
-                self._createStreamChannel(multiplexer, promise, streamStateInitializer)
-            }
         }
     }
 
@@ -354,12 +353,11 @@ extension HTTP2CommonInboundStreamMultiplexer {
         promise: EventLoopPromise<Channel>?,
         _ streamStateInitializer: @escaping (Channel) -> EventLoopFuture<Void>
     ) {
-        if self.channel.eventLoop.inEventLoop {
+        // Always create streams channels on the next event loop tick. This avoids re-entrancy
+        // issues where handlers interposed between the two HTTP/2 handlers could create streams
+        // in channel active which become activated twice.
+        self.channel.eventLoop.execute {
             self._createStreamChannel(multiplexer, promise, streamStateInitializer)
-        } else {
-            self.channel.eventLoop.execute {
-                self._createStreamChannel(multiplexer, promise, streamStateInitializer)
-            }
         }
     }
 

diff --git a/Tests/NIOHTTP2Tests/HTTP2FramePayloadStreamMultiplexerTests.swift b/Tests/NIOHTTP2Tests/HTTP2FramePayloadStreamMultiplexerTests.swift
@@ -1007,6 +1007,9 @@ final class HTTP2FramePayloadStreamMultiplexerTests: XCTestCase {
             }
         }
 
+        // Run the loop to create the channels.
+        self.channel.embeddedEventLoop.run()
+
         XCTAssertEqual(createdChannelCount, 3)
         XCTAssertEqual(configuredChannelCount, 0)
         XCTAssertEqual(streamIDs.count, 0)
@@ -1048,6 +1051,9 @@ final class HTTP2FramePayloadStreamMultiplexerTests: XCTestCase {
             }
         }
 
+        // Run the loop to create the channels.
+        self.channel.embeddedEventLoop.run()
+
         XCTAssertEqual(createdChannelCount, 3)
         XCTAssertEqual(configuredChannelCount, 0)
         XCTAssertEqual(streamIDs.count, 0)

diff --git a/Tests/NIOHTTP2Tests/HTTP2InlineStreamMultiplexerTests.swift b/Tests/NIOHTTP2Tests/HTTP2InlineStreamMultiplexerTests.swift
@@ -1041,6 +1041,9 @@ final class HTTP2InlineStreamMultiplexerTests: XCTestCase {
             }
         }
 
+        // Run the loop to create the channels.
+        self.channel.embeddedEventLoop.run()
+
         XCTAssertEqual(createdChannelCount.load(ordering: .sequentiallyConsistent), 3)
         XCTAssertEqual(configuredChannelCount, 0)
         XCTAssertEqual(streamIDs.count, 0)
@@ -2012,6 +2015,9 @@ final class HTTP2InlineStreamMultiplexerTests: XCTestCase {
             }
         }
 
+        // Run the loop to create the channels.
+        self.channel.embeddedEventLoop.run()
+
         XCTAssertEqual(createdChannelCount.load(ordering: .sequentiallyConsistent), 3)
         XCTAssertEqual(configuredChannelCount, 0)
         XCTAssertEqual(streamIDs.count, 0)

diff --git a/docker/docker-compose.2004.56.yaml b/docker/docker-compose.2004.56.yaml
@@ -32,14 +32,14 @@ services:
       - MAX_ALLOCS_ALLOWED_1k_requests_inline_noninterleaved=34100
       - MAX_ALLOCS_ALLOWED_1k_requests_interleaved=41150
       - MAX_ALLOCS_ALLOWED_1k_requests_noninterleaved=40100
-      - MAX_ALLOCS_ALLOWED_client_server_h1_request_response=288050
-      - MAX_ALLOCS_ALLOWED_client_server_h1_request_response_inline=265050
-      - MAX_ALLOCS_ALLOWED_client_server_request_response=257050
-      - MAX_ALLOCS_ALLOWED_client_server_request_response_inline=240050
-      - MAX_ALLOCS_ALLOWED_client_server_request_response_many=1202050
-      - MAX_ALLOCS_ALLOWED_client_server_request_response_many_inline=885050
-      - MAX_ALLOCS_ALLOWED_create_client_stream_channel=33050
-      - MAX_ALLOCS_ALLOWED_create_client_stream_channel_inline=33050
+      - MAX_ALLOCS_ALLOWED_client_server_h1_request_response=294050
+      - MAX_ALLOCS_ALLOWED_client_server_h1_request_response_inline=271050
+      - MAX_ALLOCS_ALLOWED_client_server_request_response=263050
+      - MAX_ALLOCS_ALLOWED_client_server_request_response_inline=246050
+      - MAX_ALLOCS_ALLOWED_client_server_request_response_many=1208050
+      - MAX_ALLOCS_ALLOWED_client_server_request_response_many_inline=891050
+      - MAX_ALLOCS_ALLOWED_create_client_stream_channel=39050
+      - MAX_ALLOCS_ALLOWED_create_client_stream_channel_inline=39050
       - MAX_ALLOCS_ALLOWED_get_100000_headers_canonical_form=200050
       - MAX_ALLOCS_ALLOWED_get_100000_headers_canonical_form_trimming_whitespace=200050
       - MAX_ALLOCS_ALLOWED_get_100000_headers_canonical_form_trimming_whitespace_from_long_string=300050

diff --git a/docker/docker-compose.2204.57.yaml b/docker/docker-compose.2204.57.yaml
@@ -32,14 +32,14 @@ services:
       - MAX_ALLOCS_ALLOWED_1k_requests_inline_noninterleaved=34100
       - MAX_ALLOCS_ALLOWED_1k_requests_interleaved=41150
       - MAX_ALLOCS_ALLOWED_1k_requests_noninterleaved=40100
-      - MAX_ALLOCS_ALLOWED_client_server_h1_request_response=288050
-      - MAX_ALLOCS_ALLOWED_client_server_h1_request_response_inline=265050
-      - MAX_ALLOCS_ALLOWED_client_server_request_response=257050
-      - MAX_ALLOCS_ALLOWED_client_server_request_response_inline=240050
-      - MAX_ALLOCS_ALLOWED_client_server_request_response_many=1202050
-      - MAX_ALLOCS_ALLOWED_client_server_request_response_many_inline=885050
-      - MAX_ALLOCS_ALLOWED_create_client_stream_channel=33050
-      - MAX_ALLOCS_ALLOWED_create_client_stream_channel_inline=33050
+      - MAX_ALLOCS_ALLOWED_client_server_h1_request_response=294050
+      - MAX_ALLOCS_ALLOWED_client_server_h1_request_response_inline=271050
+      - MAX_ALLOCS_ALLOWED_client_server_request_response=263050
+      - MAX_ALLOCS_ALLOWED_client_server_request_response_inline=246050
+      - MAX_ALLOCS_ALLOWED_client_server_request_response_many=1208050
+      - MAX_ALLOCS_ALLOWED_client_server_request_response_many_inline=891050
+      - MAX_ALLOCS_ALLOWED_create_client_stream_channel=39050
+      - MAX_ALLOCS_ALLOWED_create_client_stream_channel_inline=39050
       - MAX_ALLOCS_ALLOWED_get_100000_headers_canonical_form=200050
       - MAX_ALLOCS_ALLOWED_get_100000_headers_canonical_form_trimming_whitespace=200050
       - MAX_ALLOCS_ALLOWED_get_100000_headers_canonical_form_trimming_whitespace_from_long_string=300050

diff --git a/docker/docker-compose.2204.58.yaml b/docker/docker-compose.2204.58.yaml
@@ -32,14 +32,14 @@ services:
       - MAX_ALLOCS_ALLOWED_1k_requests_inline_noninterleaved=34100
       - MAX_ALLOCS_ALLOWED_1k_requests_interleaved=41150
       - MAX_ALLOCS_ALLOWED_1k_requests_noninterleaved=40100
-      - MAX_ALLOCS_ALLOWED_client_server_h1_request_response=288050
-      - MAX_ALLOCS_ALLOWED_client_server_h1_request_response_inline=265050
-      - MAX_ALLOCS_ALLOWED_client_server_request_response=257050
-      - MAX_ALLOCS_ALLOWED_client_server_request_response_inline=240050
-      - MAX_ALLOCS_ALLOWED_client_server_request_response_many=1202050
-      - MAX_ALLOCS_ALLOWED_client_server_request_response_many_inline=885050
-      - MAX_ALLOCS_ALLOWED_create_client_stream_channel=33050
-      - MAX_ALLOCS_ALLOWED_create_client_stream_channel_inline=33050
+      - MAX_ALLOCS_ALLOWED_client_server_h1_request_response=294050
+      - MAX_ALLOCS_ALLOWED_client_server_h1_request_response_inline=271050
+      - MAX_ALLOCS_ALLOWED_client_server_request_response=263050
+      - MAX_ALLOCS_ALLOWED_client_server_request_response_inline=246050
+      - MAX_ALLOCS_ALLOWED_client_server_request_response_many=1208050
+      - MAX_ALLOCS_ALLOWED_client_server_request_response_many_inline=891050
+      - MAX_ALLOCS_ALLOWED_create_client_stream_channel=39050
+      - MAX_ALLOCS_ALLOWED_create_client_stream_channel_inline=39050
       - MAX_ALLOCS_ALLOWED_get_100000_headers_canonical_form=200050
       - MAX_ALLOCS_ALLOWED_get_100000_headers_canonical_form_trimming_whitespace=200050
       - MAX_ALLOCS_ALLOWED_get_100000_headers_canonical_form_trimming_whitespace_from_long_string=300050

diff --git a/docker/docker-compose.2204.59.yaml b/docker/docker-compose.2204.59.yaml
@@ -31,14 +31,14 @@ services:
       - MAX_ALLOCS_ALLOWED_1k_requests_inline_noninterleaved=34100
       - MAX_ALLOCS_ALLOWED_1k_requests_interleaved=41150
       - MAX_ALLOCS_ALLOWED_1k_requests_noninterleaved=40100
-      - MAX_ALLOCS_ALLOWED_client_server_h1_request_response=288050
-      - MAX_ALLOCS_ALLOWED_client_server_h1_request_response_inline=265050
-      - MAX_ALLOCS_ALLOWED_client_server_request_response=257050
-      - MAX_ALLOCS_ALLOWED_client_server_request_response_inline=240050
-      - MAX_ALLOCS_ALLOWED_client_server_request_response_many=1202050
-      - MAX_ALLOCS_ALLOWED_client_server_request_response_many_inline=885050
-      - MAX_ALLOCS_ALLOWED_create_client_stream_channel=33050
-      - MAX_ALLOCS_ALLOWED_create_client_stream_channel_inline=33050
+      - MAX_ALLOCS_ALLOWED_client_server_h1_request_response=294050
+      - MAX_ALLOCS_ALLOWED_client_server_h1_request_response_inline=271050
+      - MAX_ALLOCS_ALLOWED_client_server_request_response=263050
+      - MAX_ALLOCS_ALLOWED_client_server_request_response_inline=246050
+      - MAX_ALLOCS_ALLOWED_client_server_request_response_many=1208050
+      - MAX_ALLOCS_ALLOWED_client_server_request_response_many_inline=891050
+      - MAX_ALLOCS_ALLOWED_create_client_stream_channel=39050
+      - MAX_ALLOCS_ALLOWED_create_client_stream_channel_inline=39050
       - MAX_ALLOCS_ALLOWED_get_100000_headers_canonical_form=200050
       - MAX_ALLOCS_ALLOWED_get_100000_headers_canonical_form_trimming_whitespace=200050
       - MAX_ALLOCS_ALLOWED_get_100000_headers_canonical_form_trimming_whitespace_from_long_string=300050

diff --git a/docker/docker-compose.2204.main.yaml b/docker/docker-compose.2204.main.yaml
@@ -31,14 +31,14 @@ services:
       - MAX_ALLOCS_ALLOWED_1k_requests_inline_noninterleaved=34100
       - MAX_ALLOCS_ALLOWED_1k_requests_interleaved=41150
       - MAX_ALLOCS_ALLOWED_1k_requests_noninterleaved=40100
-      - MAX_ALLOCS_ALLOWED_client_server_h1_request_response=288050
-      - MAX_ALLOCS_ALLOWED_client_server_h1_request_response_inline=265050
-      - MAX_ALLOCS_ALLOWED_client_server_request_response=257050
-      - MAX_ALLOCS_ALLOWED_client_server_request_response_inline=240050
-      - MAX_ALLOCS_ALLOWED_client_server_request_response_many=1202050
-      - MAX_ALLOCS_ALLOWED_client_server_request_response_many_inline=885050
-      - MAX_ALLOCS_ALLOWED_create_client_stream_channel=33050
-      - MAX_ALLOCS_ALLOWED_create_client_stream_channel_inline=33050
+      - MAX_ALLOCS_ALLOWED_client_server_h1_request_response=294050
+      - MAX_ALLOCS_ALLOWED_client_server_h1_request_response_inline=271050
+      - MAX_ALLOCS_ALLOWED_client_server_request_response=263050
+      - MAX_ALLOCS_ALLOWED_client_server_request_response_inline=246050
+      - MAX_ALLOCS_ALLOWED_client_server_request_response_many=1208050
+      - MAX_ALLOCS_ALLOWED_client_server_request_response_many_inline=891050
+      - MAX_ALLOCS_ALLOWED_create_client_stream_channel=39050
+      - MAX_ALLOCS_ALLOWED_create_client_stream_channel_inline=39050
       - MAX_ALLOCS_ALLOWED_get_100000_headers_canonical_form=200050
       - MAX_ALLOCS_ALLOWED_get_100000_headers_canonical_form_trimming_whitespace=200050
       - MAX_ALLOCS_ALLOWED_get_100000_headers_canonical_form_trimming_whitespace_from_long_string=300050