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

ReactorNetty websocket issue for multiple clients with different protocols #25315

Closed
croissong opened this issue Jun 25, 2020 · 1 comment
Closed
Assignees
Labels
in: web Issues in web modules (web, webmvc, webflux, websocket) type: regression A bug that is also a regression
Milestone

Comments

@croissong
Copy link

Affects: Spring Framework >= v5.2.6


We've observed an issue, using spring-cloud-gateway, where websocket handshakes fail if different clients, with different websocket protocols, are connecting in succession:

  1. wsClientA connects without any (sub-)protocol -> success
  2. wsClientB connects with protocol v10.stomp -> success
  3. wsClientA connects without any protocol -> failure

The bug first appeared in spring-webflux v5.2.6, with the newly introduced ReactorNettyRequestUpgradeStrategy (#24959 -> 0520ee0, ping @rstoyanchev).
The global WebsocketServerSpec.Builder is mutated for each request, persisting the client-specific protocol across requests, and thereby potentially breaking subsequent requests for clients with different (or null) subprotocols (see ReactorNettyRequestUpgradeStrategy.java#L89).
In the example above, the third handshake request is silently aborted in HttpServerOperations.java#L649, because websocketServerSpec.protocols() is set (by the second request), but ops.selectedSubprotocol() is null.

I created a simple WebSocketIntegration testcase to reproduce the issue:
https://github.com/Croissong/spring-framework/blob/ReactorNetty-websocket-protocol-bug/spring-webflux/src/test/java/org/springframework/web/reactive/socket/ReactorNettyWebsocketProtocolBugTests.java.
The test fails for all combinations using the ReactorHttpServer. However, the ReactorNettyWebSocketClient uses the same logic: ReactorNettyWebSocketClient.java#L111.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Jun 25, 2020
@rstoyanchev rstoyanchev self-assigned this Jul 20, 2020
@rstoyanchev rstoyanchev added in: web Issues in web modules (web, webmvc, webflux, websocket) type: regression A bug that is also a regression and removed status: waiting-for-triage An issue we've not yet triaged or decided on labels Jul 20, 2020
@rstoyanchev rstoyanchev added this to the 5.2.8 milestone Jul 20, 2020
@rstoyanchev
Copy link
Contributor

This should be addressed now. It's only the default constructor of ReactorNettyRequestUpgradeStrategy that was affected because it uses WebsocketServerSpect.builder() as the the Supplier but that returns the same instance always.

The other constructor receives an external Supplier and it's the responsibility of the caller to ensure that supplier returns a difference builder each time, so no change needed there. ReactorNettyWebSocketClient only has one constructor with an external Supplier and therefore is not affected by this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: web Issues in web modules (web, webmvc, webflux, websocket) type: regression A bug that is also a regression
Projects
None yet
Development

No branches or pull requests

3 participants