PR #12659 - allow HttpVersion to be set on WebSocket ClientUpgradeRequest #13185
Conversation
…Request Signed-off-by: Lachlan Roberts <[email protected]>
| /** | ||
| * Set the HTTP Version to use for the websocket upgrade. | ||
| * <p> | ||
| * Currently only HTTP/1.1 (with RFC6455) and HTTP/2 (with RFC8441) are supported. |
There was a problem hiding this comment.
I would avoid specifying this, as when we implement it for HTTP/3 we will forget about this javadoc.
| String scheme = uri.getScheme(); | ||
| if (!HttpScheme.WS.is(scheme) && !HttpScheme.WSS.is(scheme)) | ||
| throw new IllegalArgumentException("URI scheme must be 'ws' or 'wss'"); | ||
| this.host = this.requestURI.getHost(); |
There was a problem hiding this comment.
This removal feels wrong, as WebSocketClient.connect(endPoint, URI, ClientUpgradeRequest) should have been deprecated instead.
I would have kept WebSocketClient.connect(endPoint, URI), but when I pass ClientUpgradeRequest, then everything is configured in it, rather than the URI separated from ClientUpgradeRequest, so just have
WebSocketClient.connect(endPoint, ClientUpgradeRequest).
There was a problem hiding this comment.
I saw this constructor had been deprecated for ages so I removed it.
Otherwise it is not really related to this PR, so reverting.
There was a problem hiding this comment.
Opened issue #13213 for this
| public String getHttpVersion() | ||
| { | ||
| throw new UnsupportedOperationException("HttpVersion not available on ClientUpgradeRequest"); | ||
| return null; |
There was a problem hiding this comment.
It's now very weird that we cannot get the URI host (and other things) from a request.
| assertThat(wsEndPoint.closeReason, equalTo("normal close from test")); | ||
|
|
||
| // Wait for the request to complete on server before stopping. | ||
| assertTrue(latch.await(5, TimeUnit.SECONDS)); |
There was a problem hiding this comment.
We are not sure that the upgrade actually happened with HTTP/2.
Can you modify TestJettyWebSocketServlet to have a /ws/version endpoint that returns the HTTP version as seen by the server, and then you assert that in the test?
Signed-off-by: Lachlan Roberts <[email protected]>
closes #12659
Implement
setHttpVersionmethod onClientUpgradeRequestso thatWebSocketClientssupporting both HTTP/1.1 and HTTP/2 websocket upgrades can select which one to use.