Close channel during upgrade, if client closes input #2756
Conversation
| switch event { | ||
| case let evt as ChannelEvent where evt == ChannelEvent.inputClosed: | ||
| // The remote peer half-closed the channel during the upgrade so we should close | ||
| context.close(promise: nil) |
There was a problem hiding this comment.
Thanks for working on this!
Can we please go through the state machine here. In particular, if we are in the state unbuffering we should not close here but rather just hold the event until the unbuffering is done.
Can we also write a test for this and look if we need to do the same for the client upgrade handler?
There was a problem hiding this comment.
We should also consider only closing if we haven't received a full upgrade request, and otherwise we should buffer this.
There was a problem hiding this comment.
I did consider this case, but thought I would post this first to start the conversation. So should we only close the connection if we are in the state initial and awaitingUpgrader. Otherwise should I let it run until the upgrade is complete?
Second all the upgrade tests use EmbeddedChannel. Is it possible to emulate half closure with EmbeddedChannel?
There was a problem hiding this comment.
It should be possible to emulate half closure with the embedded channel by just sending the event to the channel.
| @inlinable | ||
| mutating func closeInbound() -> CloseInboundAction { | ||
| switch self.state { | ||
| case .initial, .awaitingUpgrader: |
There was a problem hiding this comment.
We should immediately close in the state initial. In the state awaitingUpgrader we have to check if we have seenFirstRequest == true. If have seen that we should buffer the inputClosed and send it after the upgrading is finished. In the state upgraderReady we need to buffer the inputClosed indefinitely.
The reason why this is so complicated is that the client could potentially send a half closure after it send the initial upgrade request + some data on the upgraded protocol. We can't just unconditionally close here but we rather need to buffer the inputClose and unbuffer it with the data. I would recommend changing the var buffer: Deque<NIOAny> in the various states to var buffer: Deque<enum(NIOAny | inputClosed)>.
| @@ -2066,5 +2073,31 @@ final class TypedHTTPServerUpgradeTestCase: HTTPServerUpgradeTestCase { | |||
| // We also want to confirm that the upgrade handler is no longer in the pipeline. | |||
| try connectedServer.pipeline.waitForUpgraderToBeRemoved() | |||
| } | |||
|
|
|||
| func testHalfClosure() throws { | |||
There was a problem hiding this comment.
Can we add one or two more tests cases here:
- That sends the request head then input close
- That sends a full request head & end and then input close to check that we continue the upgrade
There was a problem hiding this comment.
@FranzBusch
I did 2 (see testSendRequestCloseImmediately)
I'm not sure what the expected result is for 1
First I had to send a request head that included a content-length so a .head would be passed down the pipeline, but not an.end eg
OPTIONS * HTTP/1.1\r\nHost: localhost\r\ncontent-length: 10\r\nUpgrade: myproto\r\nKafkaesque: yup\r\nConnection: upgrade\r\nConnection: kafkaesque\r\n\r\n
With this it gets stuck in the state .upgradeReady because nothing else happens after closeInbound to forward the state machine onwards and the connection stays open. If I return .close when the state is .upgradeReady from the state machine everything works but you explicitly said I shouldn't do that.
There was a problem hiding this comment.
@FranzBusch any thoughts
I had to add another handler to `setUpTestWithAutoremoval` to catch upgrade errors.
Co-authored-by: Franz Busch <privat@franz-busch.de>
adam-fowler
force-pushed
the
websocket-half-close
branch
from
67750ef
to
96fb6ea
Compare
|
ping @FranzBusch and @Lukasa |
| case .data(let data): | ||
| return .fireChannelRead(data) | ||
| case .inputClosed: | ||
| return .close |
There was a problem hiding this comment.
In this case we shouldn't close the channel. It might be completely valid that the remote send us an upgrade + data on the new protocol format + closed the input all at once. The last input close just indicates a half closure but we must not escalate this to a full closure. Instead we should just unbuffer the inputClosed here and fire the channel event.
| } | ||
|
|
||
| @inlinable | ||
| mutating func closeInbound() -> CloseInboundAction { |
There was a problem hiding this comment.
Naming NIT: Can we call this inputClosed instead please
| case .modifying, .finished: | ||
| return .continue |
There was a problem hiding this comment.
In finished we should forward the inputClosed event. In modifying we should fatalError as we do in the other methods.
Check for half closure during server upgrade and close channel if client closes the channel
Motivation:
This is to fix #2742
Modifications:
Add
userInboundEventTriggeredfunction toNIOTypedHTTPServerProtocolUpgraderwhich checks forChannelEvent.inputClosedResult:
Negotiation future now errors when client closes the connection instead of never completing