-
Notifications
You must be signed in to change notification settings - Fork 128
Tolerate shutdown message after channel is closed #646
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 4 commits
3b3b5d0
1a51229
4d8b475
7775fa0
ba76fe4
b17d92e
510d635
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -111,7 +111,7 @@ final class HTTP2Connection { | |
|
||
deinit { | ||
guard case .closed = self.state else { | ||
preconditionFailure("Connection must be closed, before we can deinit it") | ||
preconditionFailure("Connection must be closed, before we can deinit it. Current state: \(self.state)") | ||
} | ||
} | ||
|
||
|
@@ -164,15 +164,23 @@ final class HTTP2Connection { | |
return promise.futureResult | ||
} | ||
|
||
private func start() -> EventLoopFuture<Int> { | ||
func start() -> EventLoopFuture<Int> { | ||
self.channel.eventLoop.assertInEventLoop() | ||
|
||
let readyToAcceptConnectionsPromise = self.channel.eventLoop.makePromise(of: Int.self) | ||
|
||
self.state = .starting(readyToAcceptConnectionsPromise) | ||
self.channel.closeFuture.whenComplete { _ in | ||
self.state = .closed | ||
self.delegate.http2ConnectionClosed(self) | ||
switch self.state { | ||
case .initialized, .closed: | ||
preconditionFailure("invalid state \(self.state)") | ||
case .starting(let readyToAcceptConnectionsPromise): | ||
self.state = .closed | ||
readyToAcceptConnectionsPromise.fail(HTTPClientError.remoteConnectionClosed) | ||
case .active, .closing: | ||
self.state = .closed | ||
self.delegate.http2ConnectionClosed(self) | ||
} | ||
} | ||
|
||
do { | ||
|
@@ -258,16 +266,26 @@ final class HTTP2Connection { | |
private func shutdown0() { | ||
self.channel.eventLoop.assertInEventLoop() | ||
|
||
self.state = .closing | ||
switch self.state { | ||
case .active: | ||
self.state = .closing | ||
|
||
// inform all open streams, that the currently running request should be cancelled. | ||
self.openStreams.forEach { box in | ||
box.channel.triggerUserOutboundEvent(HTTPConnectionEvent.shutdownRequested, promise: nil) | ||
} | ||
// inform all open streams, that the currently running request should be cancelled. | ||
self.openStreams.forEach { box in | ||
box.channel.triggerUserOutboundEvent(HTTPConnectionEvent.shutdownRequested, promise: nil) | ||
} | ||
|
||
// inform the idle connection handler, that connection should be closed, once all streams | ||
// are closed. | ||
self.channel.triggerUserOutboundEvent(HTTPConnectionEvent.shutdownRequested, promise: nil) | ||
// inform the idle connection handler, that connection should be closed, once all streams | ||
// are closed. | ||
self.channel.triggerUserOutboundEvent(HTTPConnectionEvent.shutdownRequested, promise: nil) | ||
|
||
case .closed, .closing: | ||
// we are already closing/closed and we need to tolerate this | ||
break | ||
|
||
case .initialized, .starting: | ||
preconditionFailure("invalid state \(self.state)") | ||
Comment on lines
+286
to
+287
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there a reason for us not to support going from initialized straight to closed? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It is an invalid transition. We require a call to |
||
} | ||
} | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -42,6 +42,28 @@ class HTTP2ConnectionTests: XCTestCase { | |
).wait()) | ||
} | ||
|
||
func testConnectionToleratesShutdownEventsAfterAlreadyClosed() { | ||
let embedded = EmbeddedChannel() | ||
XCTAssertNoThrow(try embedded.connect(to: SocketAddress(ipAddress: "127.0.0.1", port: 3000)).wait()) | ||
|
||
let logger = Logger(label: "test.http2.connection") | ||
let connection = HTTP2Connection( | ||
channel: embedded, | ||
connectionID: 0, | ||
decompression: .disabled, | ||
delegate: TestHTTP2ConnectionDelegate(), | ||
logger: logger | ||
) | ||
_ = connection.start() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why don't you use the existing internal start method? I think we should keep the plain There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The problem is that the only way for the the future returned by There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. But we don't need that future, or am I missing something here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We do need it as only its success value contains the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. stupid me... makes sense. can we rename it to start0 though. to make sure it is called on el, if someone else internally tries to call it? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sure changed in ba76fe4 |
||
|
||
XCTAssertNoThrow(try embedded.close().wait()) | ||
// to really destroy the channel we need to tick once | ||
embedded.embeddedEventLoop.run() | ||
|
||
// should not crash | ||
connection.shutdown() | ||
} | ||
|
||
func testSimpleGetRequest() { | ||
let eventLoopGroup = MultiThreadedEventLoopGroup(numberOfThreads: 1) | ||
let eventLoop = eventLoopGroup.next() | ||
|
Uh oh!
There was an error while loading. Please reload this page.