Tolerate shutdown message after channel is closed #646
Conversation
### Motivation A channel can close unexpectedly if something goes wrong. We may in the meantime have scheduled the connection for graceful shutdown but the connection has not yet seen the message. We need to still accept the shutdown message and just ignore it if we are already closed. ### Modification - ignore calls to shutdown if the channel is already closed - add a test which would previously crash because we have transition from the closed state to the closing state and we hit the deinit precondition - include the current state in preconditions if we are in the wrong state ### Result We don’t hit the precondition in the deinit in the scenario described above and have more descriptive crashes if something still goes wrong.
| case .closed: | ||
| // we are already closed and we need to tolerate this | ||
| break | ||
| case .initialized, .closing: | ||
| preconditionFailure("invalid state \(self.state)") |
There was a problem hiding this comment.
I think we should also support shutdown calls if we are already closing. No reason to crash here. Also do we want to support shutdown, if we haven't even connected? We could go straight to closed, which is the only way to drop a connection currently.
There was a problem hiding this comment.
Agree, changed in 1a51229
| let connection = HTTP2Connection( | ||
| channel: embedded, | ||
| connectionID: 0, | ||
| decompression: .disabled, | ||
| delegate: TestHTTP2ConnectionDelegate(), | ||
| logger: logger | ||
| ) | ||
| _ = connection.start() |
There was a problem hiding this comment.
why don't you use the existing internal start method? I think we should keep the plain start() method internal.
There was a problem hiding this comment.
The problem is that the only way for the the future returned by HTTP2Connection.start() to fulfill is by sending ByteBuffers of HTTP2Frames through the Channel. HTTP2Frame de/encoding is currently not publicly available.
There was a problem hiding this comment.
But we don't need that future, or am I missing something here?
There was a problem hiding this comment.
We do need it as only its success value contains the HTTP2Connection instance. We otherwise don't have access to it and therefore can't call .shutdown().
There was a problem hiding this comment.
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.
Sure changed in ba76fe4
dnadoba
force-pushed
the
dn-tollerate-shutdown-after-closed
branch
from
653858d to
1a51229
Compare
| case .initialized, .starting: | ||
| preconditionFailure("invalid state \(self.state)") |
There was a problem hiding this comment.
Is there a reason for us not to support going from initialized straight to closed?
There was a problem hiding this comment.
It is an invalid transition. We require a call to start() before any other interaction which transitions to the starting case. Further interaction is only allowed after the promise returned by .start() is completed which transitions to either the .active or .closed state. This makes sure we don't violate this contract.
Sources/AsyncHTTPClient/ConnectionPool/HTTP2/HTTP2Connection.swift
Outdated
Show resolved
Hide resolved
And wait for connection start to fail
| @@ -164,15 +164,23 @@ final class HTTP2Connection { | |||
| return promise.futureResult | |||
| } | |||
|
|
|||
| private func start() -> EventLoopFuture<Int> { | |||
| func start0() -> EventLoopFuture<Int> { | |||
There was a problem hiding this comment.
Nit: should we give this an underscore?
Motivation
A channel can close unexpectedly if something goes wrong. We may in the meantime have scheduled the connection for graceful shutdown but the connection has not yet seen the message. We need to still accept the shutdown message and just ignore it if we are already closed.
Modification
Result
We don’t hit the precondition in the deinit in the scenario described above and have more descriptive crashes if something still goes wrong.