Skip to content

Commit 170f4ca

Browse files
gjcairogjcairo
authored
Don't fail closeFuture when an error occurs on closing (#487)
Strictly speaking, a `close` operation can't really fail (except when the closing mode isn't supported or the channel is already closed). Even if an error is encountered while closing a channel, or if an error caused the channel to be closed, the end result is the same: the channel will be closed. In this case in particular, when streams are closed from the client (i.e. by sending a RST_STREAM frame) the `HTTP2StreamChannel` would fail the channel's `closeFuture`. This isn't appropriate however, as the channel is successfully closed. This change stops failing the stream channel's `closeFuture`. Instead, if the close happens uncleanly, the error will be fired down the pipeline and the `close` method's promise will be failed.
1 parent 2083f5d commit 170f4ca

14 files changed

+268
-169
lines changed

‎IntegrationTests/tests_01_allocation_counters/Thresholds/5.10.json‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,6 @@
1818
"get_100000_headers_canonical_form_trimming_whitespace_from_long_string": 300050,
1919
"get_100000_headers_canonical_form_trimming_whitespace_from_short_string": 200050,
2020
"hpack_decoding": 5050,
21-
"stream_teardown_100_concurrent": 262550,
22-
"stream_teardown_100_concurrent_inline": 261650
21+
"stream_teardown_100_concurrent": 252400,
22+
"stream_teardown_100_concurrent_inline": 252400
2323
}

‎IntegrationTests/tests_01_allocation_counters/Thresholds/5.9.json‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,6 @@
1818
"get_100000_headers_canonical_form_trimming_whitespace_from_long_string": 300050,
1919
"get_100000_headers_canonical_form_trimming_whitespace_from_short_string": 200050,
2020
"hpack_decoding": 5050,
21-
"stream_teardown_100_concurrent": 262550,
22-
"stream_teardown_100_concurrent_inline": 261650
21+
"stream_teardown_100_concurrent": 252400,
22+
"stream_teardown_100_concurrent_inline": 252400
2323
}

‎IntegrationTests/tests_01_allocation_counters/Thresholds/6.0.json‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,6 @@
1818
"get_100000_headers_canonical_form_trimming_whitespace_from_long_string": 300050,
1919
"get_100000_headers_canonical_form_trimming_whitespace_from_short_string": 200050,
2020
"hpack_decoding": 5050,
21-
"stream_teardown_100_concurrent": 262450,
22-
"stream_teardown_100_concurrent_inline": 261750
21+
"stream_teardown_100_concurrent": 252450,
22+
"stream_teardown_100_concurrent_inline": 251550
2323
}

‎IntegrationTests/tests_01_allocation_counters/Thresholds/nightly-6.0.json‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,6 @@
1818
"get_100000_headers_canonical_form_trimming_whitespace_from_long_string": 300050,
1919
"get_100000_headers_canonical_form_trimming_whitespace_from_short_string": 200050,
2020
"hpack_decoding": 5050,
21-
"stream_teardown_100_concurrent": 262450,
22-
"stream_teardown_100_concurrent_inline": 261750
21+
"stream_teardown_100_concurrent": 252450,
22+
"stream_teardown_100_concurrent_inline": 251550
2323
}

‎IntegrationTests/tests_01_allocation_counters/Thresholds/nightly-main.json‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,6 @@
1818
"get_100000_headers_canonical_form_trimming_whitespace_from_long_string": 300050,
1919
"get_100000_headers_canonical_form_trimming_whitespace_from_short_string": 200050,
2020
"hpack_decoding": 5050,
21-
"stream_teardown_100_concurrent": 262450,
22-
"stream_teardown_100_concurrent_inline": 261750
21+
"stream_teardown_100_concurrent": 252450,
22+
"stream_teardown_100_concurrent_inline": 251550
2323
}

‎Sources/NIOHTTP2/HTTP2StreamChannel.swift‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -676,7 +676,7 @@ final class HTTP2StreamChannel: Channel, ChannelCore, @unchecked Sendable {
676676

677677
self.eventLoop.execute {
678678
self.removeHandlers(pipeline: self.pipeline)
679-
self.closePromise.succeed(())
679+
self.closePromise.succeed()
680680
if let streamID = self.streamID {
681681
self.multiplexer.streamClosed(id: streamID)
682682
} else {
@@ -701,7 +701,7 @@ final class HTTP2StreamChannel: Channel, ChannelCore, @unchecked Sendable {
701701

702702
self.eventLoop.execute {
703703
self.removeHandlers(pipeline: self.pipeline)
704-
self.closePromise.fail(error)
704+
self.closePromise.succeed()
705705
if let streamID = self.streamID {
706706
self.multiplexer.streamClosed(id: streamID)
707707
} else {

‎Tests/NIOHTTP2Tests/ConfiguringPipelineInlineMultiplexerTests.swift‎

Lines changed: 30 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -71,7 +71,10 @@ class ConfiguringPipelineInlineMultiplexerTests: XCTestCase {
7171
)
7272
)
7373

74-
clientMultiplexer.createStreamChannel(promise: nil) { channel in
74+
let errorHandler = ErrorEncounteredHandler()
75+
let streamChannelPromise = self.clientChannel.eventLoop.makePromise(of: Channel.self)
76+
clientMultiplexer.createStreamChannel(promise: streamChannelPromise) { channel in
77+
try? channel.pipeline.syncOperations.addHandler(errorHandler)
7578
channel.writeAndFlush(reqFrame.payload).whenComplete { _ in channel.close(promise: requestPromise) }
7679
return channel.eventLoop.makeSucceededFuture(())
7780
}
@@ -82,9 +85,12 @@ class ConfiguringPipelineInlineMultiplexerTests: XCTestCase {
8285
self.interactInMemory(self.clientChannel, self.serverChannel)
8386
(self.clientChannel.eventLoop as! EmbeddedEventLoop).run()
8487

88+
let streamChannel = try XCTUnwrap(streamChannelPromise.futureResult.wait())
89+
XCTAssertNoThrow(try streamChannel.closeFuture.wait())
8590
XCTAssertThrowsError(try requestPromise.futureResult.wait()) { error in
8691
XCTAssertTrue(error is NIOHTTP2Errors.StreamClosed)
8792
}
93+
XCTAssertTrue(errorHandler.encounteredError is NIOHTTP2Errors.StreamClosed)
8894

8995
// We should have received a HEADERS and a RST_STREAM frame.
9096
// The RST_STREAM frame is from closing an incomplete stream on the client side.
@@ -133,7 +139,10 @@ class ConfiguringPipelineInlineMultiplexerTests: XCTestCase {
133139
)
134140
)
135141

136-
clientMultiplexer.createStreamChannel(promise: nil) { channel in
142+
let errorHandler = ErrorEncounteredHandler()
143+
let streamChannelPromise = self.clientChannel.eventLoop.makePromise(of: Channel.self)
144+
clientMultiplexer.createStreamChannel(promise: streamChannelPromise) { channel in
145+
try? channel.pipeline.syncOperations.addHandler(errorHandler)
137146
channel.writeAndFlush(reqFrame.payload).whenComplete { _ in channel.close(promise: requestPromise) }
138147
return channel.eventLoop.makeSucceededFuture(())
139148
}
@@ -144,9 +153,12 @@ class ConfiguringPipelineInlineMultiplexerTests: XCTestCase {
144153
self.interactInMemory(self.clientChannel, self.serverChannel)
145154
(self.clientChannel.eventLoop as! EmbeddedEventLoop).run()
146155

156+
let streamChannel = try XCTUnwrap(streamChannelPromise.futureResult.wait())
157+
XCTAssertNoThrow(try streamChannel.closeFuture.wait())
147158
XCTAssertThrowsError(try requestPromise.futureResult.wait()) { error in
148159
XCTAssertTrue(error is NIOHTTP2Errors.StreamClosed)
149160
}
161+
XCTAssertTrue(errorHandler.encounteredError is NIOHTTP2Errors.StreamClosed)
150162

151163
// We should have received a HEADERS and a RST_STREAM frame.
152164
// The RST_STREAM frame is from closing an incomplete stream on the client side.
@@ -466,7 +478,10 @@ class ConfiguringPipelineInlineMultiplexerTests: XCTestCase {
466478
)
467479
)
468480

469-
clientMultiplexer.createStreamChannel(promise: nil) { channel in
481+
let errorHandler = ErrorEncounteredHandler()
482+
let streamChannelPromise = self.clientChannel.eventLoop.makePromise(of: Channel.self)
483+
clientMultiplexer.createStreamChannel(promise: streamChannelPromise) { channel in
484+
try? channel.pipeline.syncOperations.addHandler(errorHandler)
470485
channel.writeAndFlush(reqFrame.payload).whenComplete { _ in channel.close(promise: requestPromise) }
471486
return channel.eventLoop.makeSucceededFuture(())
472487
}
@@ -476,9 +491,13 @@ class ConfiguringPipelineInlineMultiplexerTests: XCTestCase {
476491
(self.clientChannel.eventLoop as! EmbeddedEventLoop).run()
477492
self.interactInMemory(self.clientChannel, self.serverChannel)
478493
(self.clientChannel.eventLoop as! EmbeddedEventLoop).run()
494+
495+
let streamChannel = try XCTUnwrap(streamChannelPromise.futureResult.wait())
496+
XCTAssertNoThrow(try streamChannel.closeFuture.wait())
479497
XCTAssertThrowsError(try requestPromise.futureResult.wait()) { error in
480498
XCTAssertTrue(error is NIOHTTP2Errors.StreamClosed)
481499
}
500+
XCTAssertTrue(errorHandler.encounteredError is NIOHTTP2Errors.StreamClosed)
482501

483502
// Assert that the user-provided handler received the
484503
// HTTP1 parts corresponding to the H2 message sent
@@ -556,7 +575,10 @@ class ConfiguringPipelineInlineMultiplexerTests: XCTestCase {
556575
)
557576
)
558577

559-
clientMultiplexer.createStreamChannel(promise: nil) { channel in
578+
let errorHandler = ErrorEncounteredHandler()
579+
let streamChannelPromise = self.clientChannel.eventLoop.makePromise(of: Channel.self)
580+
clientMultiplexer.createStreamChannel(promise: streamChannelPromise) { channel in
581+
try? channel.pipeline.syncOperations.addHandler(errorHandler)
560582
channel.writeAndFlush(reqFrame.payload).whenComplete { _ in channel.close(promise: requestPromise) }
561583
return channel.eventLoop.makeSucceededFuture(())
562584
}
@@ -566,9 +588,13 @@ class ConfiguringPipelineInlineMultiplexerTests: XCTestCase {
566588
(self.clientChannel.eventLoop as! EmbeddedEventLoop).run()
567589
self.interactInMemory(self.clientChannel, self.serverChannel)
568590
(self.clientChannel.eventLoop as! EmbeddedEventLoop).run()
591+
592+
let streamChannel = try XCTUnwrap(streamChannelPromise.futureResult.wait())
593+
XCTAssertNoThrow(try streamChannel.closeFuture.wait())
569594
XCTAssertThrowsError(try requestPromise.futureResult.wait()) { error in
570595
XCTAssertTrue(error is NIOHTTP2Errors.StreamClosed)
571596
}
597+
XCTAssertTrue(errorHandler.encounteredError is NIOHTTP2Errors.StreamClosed)
572598

573599
// Assert that the user-provided handler received the
574600
// HTTP1 parts corresponding to the H2 message sent

‎Tests/NIOHTTP2Tests/ConfiguringPipelineTests.swift‎

Lines changed: 30 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -61,7 +61,10 @@ class ConfiguringPipelineTests: XCTestCase {
6161
)
6262
)
6363

64-
clientHandler.createStreamChannel(promise: nil) { channel, streamID in
64+
let errorHandler = ErrorEncounteredHandler()
65+
let streamChannelPromise = self.clientChannel.eventLoop.makePromise(of: Channel.self)
66+
clientHandler.createStreamChannel(promise: streamChannelPromise) { channel, streamID in
67+
try? channel.pipeline.syncOperations.addHandler(errorHandler)
6568
XCTAssertEqual(streamID, HTTP2StreamID(1))
6669
channel.writeAndFlush(reqFrame).whenComplete { _ in channel.close(promise: requestPromise) }
6770
return channel.eventLoop.makeSucceededFuture(())
@@ -73,9 +76,12 @@ class ConfiguringPipelineTests: XCTestCase {
7376
self.interactInMemory(self.clientChannel, self.serverChannel)
7477
(self.clientChannel.eventLoop as! EmbeddedEventLoop).run()
7578

79+
let streamChannel = try XCTUnwrap(streamChannelPromise.futureResult.wait())
80+
XCTAssertNoThrow(try streamChannel.closeFuture.wait())
7681
XCTAssertThrowsError(try requestPromise.futureResult.wait()) { error in
7782
XCTAssertTrue(error is NIOHTTP2Errors.StreamClosed)
7883
}
84+
XCTAssertTrue(errorHandler.encounteredError is NIOHTTP2Errors.StreamClosed)
7985

8086
// We should have received a HEADERS and a RST_STREAM frame.
8187
// The RST_STREAM frame is from closing an incomplete stream on the client side.
@@ -116,7 +122,10 @@ class ConfiguringPipelineTests: XCTestCase {
116122
)
117123
)
118124

119-
clientHandler.createStreamChannel(promise: nil) { channel, streamID in
125+
let errorHandler = ErrorEncounteredHandler()
126+
let streamChannelPromise = self.clientChannel.eventLoop.makePromise(of: Channel.self)
127+
clientHandler.createStreamChannel(promise: streamChannelPromise) { channel, streamID in
128+
try? channel.pipeline.syncOperations.addHandler(errorHandler)
120129
XCTAssertEqual(streamID, HTTP2StreamID(1))
121130
channel.writeAndFlush(reqFrame).whenComplete { _ in channel.close(promise: requestPromise) }
122131
return channel.eventLoop.makeSucceededFuture(())
@@ -128,9 +137,12 @@ class ConfiguringPipelineTests: XCTestCase {
128137
self.interactInMemory(self.clientChannel, self.serverChannel)
129138
(self.clientChannel.eventLoop as! EmbeddedEventLoop).run()
130139

140+
let streamChannel = try XCTUnwrap(streamChannelPromise.futureResult.wait())
141+
XCTAssertNoThrow(try streamChannel.closeFuture.wait())
131142
XCTAssertThrowsError(try requestPromise.futureResult.wait()) { error in
132143
XCTAssertTrue(error is NIOHTTP2Errors.StreamClosed)
133144
}
145+
XCTAssertTrue(errorHandler.encounteredError is NIOHTTP2Errors.StreamClosed)
134146

135147
// We should have received a HEADERS and a RST_STREAM frame.
136148
// The RST_STREAM frame is from closing an incomplete stream on the client side.
@@ -403,7 +415,10 @@ class ConfiguringPipelineTests: XCTestCase {
403415
)
404416
)
405417

406-
clientHandler.createStreamChannel(promise: nil) { channel, streamID in
418+
let errorHandler = ErrorEncounteredHandler()
419+
let streamChannelPromise = self.clientChannel.eventLoop.makePromise(of: Channel.self)
420+
clientHandler.createStreamChannel(promise: streamChannelPromise) { channel, streamID in
421+
try? channel.pipeline.syncOperations.addHandler(errorHandler)
407422
XCTAssertEqual(streamID, HTTP2StreamID(1))
408423
channel.writeAndFlush(reqFrame).whenComplete { _ in channel.close(promise: requestPromise) }
409424
return channel.eventLoop.makeSucceededFuture(())
@@ -414,9 +429,13 @@ class ConfiguringPipelineTests: XCTestCase {
414429
(self.clientChannel.eventLoop as! EmbeddedEventLoop).run()
415430
self.interactInMemory(self.clientChannel, self.serverChannel)
416431
(self.clientChannel.eventLoop as! EmbeddedEventLoop).run()
432+
433+
let streamChannel = try XCTUnwrap(streamChannelPromise.futureResult.wait())
434+
XCTAssertNoThrow(try streamChannel.closeFuture.wait())
417435
XCTAssertThrowsError(try requestPromise.futureResult.wait()) { error in
418436
XCTAssertTrue(error is NIOHTTP2Errors.StreamClosed)
419437
}
438+
XCTAssertTrue(errorHandler.encounteredError is NIOHTTP2Errors.StreamClosed)
420439

421440
let serverChildChannel = try serverChildChannelPromise.futureResult.wait()
422441
try serverChildChannel.pipeline.handler(type: HTTP1ServerRequestRecorderHandler.self).map { serverRecorder in
@@ -492,7 +511,10 @@ class ConfiguringPipelineTests: XCTestCase {
492511
)
493512
)
494513

495-
clientHandler.createStreamChannel(promise: nil) { channel, streamID in
514+
let errorHandler = ErrorEncounteredHandler()
515+
let streamChannelPromise = self.clientChannel.eventLoop.makePromise(of: Channel.self)
516+
clientHandler.createStreamChannel(promise: streamChannelPromise) { channel, streamID in
517+
try? channel.pipeline.syncOperations.addHandler(errorHandler)
496518
XCTAssertEqual(streamID, HTTP2StreamID(1))
497519
channel.writeAndFlush(reqFrame).whenComplete { _ in channel.close(promise: requestPromise) }
498520
return channel.eventLoop.makeSucceededFuture(())
@@ -503,9 +525,13 @@ class ConfiguringPipelineTests: XCTestCase {
503525
(self.clientChannel.eventLoop as! EmbeddedEventLoop).run()
504526
self.interactInMemory(self.clientChannel, self.serverChannel)
505527
(self.clientChannel.eventLoop as! EmbeddedEventLoop).run()
528+
529+
let streamChannel = try XCTUnwrap(streamChannelPromise.futureResult.wait())
530+
XCTAssertNoThrow(try streamChannel.closeFuture.wait())
506531
XCTAssertThrowsError(try requestPromise.futureResult.wait()) { error in
507532
XCTAssertTrue(error is NIOHTTP2Errors.StreamClosed)
508533
}
534+
XCTAssertTrue(errorHandler.encounteredError is NIOHTTP2Errors.StreamClosed)
509535

510536
let serverChildChannel = try serverChildChannelPromise.futureResult.wait()
511537
try serverChildChannel.pipeline.handler(type: HTTP1ServerRequestRecorderHandler.self).map { serverRecorder in

0 commit comments

Comments
 (0)