Skip to content
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

Fix for Websocket client upgrade failure #2659

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

agnosticdev
Copy link
Contributor

@agnosticdev agnosticdev commented Feb 19, 2024

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.

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.
@agnosticdev
Copy link
Contributor Author

Rebased main to try to fix API Breakage check.

Copy link
Member

@FranzBusch FranzBusch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@adam-fowler
Copy link
Contributor

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
@agnosticdev
Copy link
Contributor Author

Added a unit test for this fix that validates that even if the update path did not succeed notUpgradingCompletionHandler should still be called.

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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants