Fix for Websocket client upgrade failure #2659
Conversation
Motivation: Fix for issue-2631 Modifications: Make sure the completion handler is sent on handler removed in upgrade failure path Result: Fix for both cases described in the issue.
|
Rebased main to try to fix API Breakage check. |
There was a problem hiding this comment.
Thanks for trying to tackle this. Could we start with adding a unit test that exercises the client and server flow here. I am not sure if this is the correct fix for the issue @adam-fowler described in the issue yet. Let's get started with the unit test and then we can take it from there.
|
It appears to have fixed the issue I was having in hummingbird-websocket. But yeah a local test would be a good idea. |
Motivation: Review team asked to add a unit test Modifications: Added a new unit test to WebSocketClientEndToEndTests Result: Test the change I made
|
Added a unit test for this fix that validates that even if the update path did not succeed |
Motivation: Random print statement left in
| @@ -130,7 +130,19 @@ public final class NIOTypedHTTPClientUpgradeHandler<UpgradeResult: Sendable>: Ch | |||
| public func handlerRemoved(context: ChannelHandlerContext) { | |||
| switch self.stateMachine.handlerRemoved() { | |||
| case .failUpgradePromise: | |||
| self.upgradeResultPromise.fail(ChannelError.inappropriateOperationForState) | |||
| // Make sure the completion handler is called on the failed upgrade path | |||
| self.notUpgradingCompletionHandler(context.channel) | |||
There was a problem hiding this comment.
This We cannot run the notUpgradingCompletionHandler in all cases where the state machine returns failUpgradePromise right now. What we should rather do is add a case to the HandlerRemovedAction from the state machine and return that from the states where we know that running the notUpgradingCompletionHandler is correct. In detail, running the notUpgradingCompletionHandler while we are in the state upgrading or unbuffering doesn't seem correct to me.
| let updgradeResult = try clientChannel.pipeline.syncOperations.configureUpgradableHTTPClientPipeline(configuration: .init(upgradeConfiguration: config)) | ||
|
|
||
| XCTAssertNoThrow(try interactInMemory(clientChannel, serverChannel, eventLoop: loop)) | ||
| XCTAssertNoThrow(try clientChannel.finish()) |
There was a problem hiding this comment.
While this test hits handlerRemoved it hits it for the wrong reason IMO. When calling clientChannel.finish we are closing the channel which will lead to handlerRemoved being called. What we should do instead is construct a test case where the server responds that it is not upgrading by returning a normal HTTP response and then making sure that the notUpgradingHandler is run.
Motivation:
Fix for issue-2631
Modifications:
Make sure the completion handler is sent in the upgrade failure path. Either in the expected or non-expected case.
Result:
Fix for both cases described in the issue.