WebSocketClient connect / upgrade timeout not configurable

[michaelkwan]

Jetty version
9.4.28
Java version
1.8.0.252
OS type/version
Centos 6
Description
WebSocketClient.connect(...) creates WebSocketUpgradeRequests with indefinite timeout. Even with the Future.cancel(), it would not cancel the requests.

This also leads to a big problem because: When the HttpDestination exchange queued exceeded, no new requests can be added. The requests stay in the queue forever.

Reproduction steps:

1. Set HttpClient MaxConnectionsPerDestination = 1 and MaxRequestsQueuedPerDestination = 1
2. Asynchronously create 20 simultaneous connections (to the same server)
3. Observed the HttpDestination queued is full and the request would never dequeue without the timeout

No new requests can be made, as queue is full.

[lachlan-roberts]

@michaelkwan there is no per request configuration for this but you can configure the timeouts through the WebSocketClient or HttpClient.

There is WebSocketClient.setConnectTimeout() and WebSocketClient.setMaxIdleTimeout() which just delegates to set these on the HttpClient. The idleTimeout set by WebSocketClient.setMaxIdleTimeout() is also used as a default for new WebSocket connections.

[michaelkwan]

@michaelkwan there is no per request configuration for this but you can configure the timeouts through the WebSocketClient or HttpClient.

There is WebSocketClient.setConnectTimeout() and WebSocketClient.setMaxIdleTimeout() which just delegates to set these on the HttpClient. The idleTimeout set by WebSocketClient.setMaxIdleTimeout() is also used as a default for new WebSocket connections.

@lachlan-roberts The connect timeout is the timeout for initializing the connection, not for the initial http upgrade request
I m not talking about the websocket messages, so the idle timeout is of no use here.

Please take a look the WebSocketUpgradeRequest is a HttpRequest. The initial value for timeout is 0.

[michaelkwan]

@lachlan-roberts Also, like I mentioned, even thou I use WebSocketClient.connect() and Future.get(Time), when you do future.cancel(true), the request stays in the httpexchange queue forever if you exceeded both the maxconnectionsperdestination and maxRequestsQueuedPerDestination and is unrecoverable unless you shutdown the client.

[lachlan-roberts]

@michaelkwan It will be simple enough to add the request timeout to the ClientUpgradeRequest.

I think the Future was intended as more of a notification that the Session was available rather than a way to cancel the websocket handshake. This could be tricky to implement because the future isn't completed until the websocket connection and session has been opened.

[lachlan-roberts]

@michaelkwan take a look at PR #5024 and let me know what you think.

[michaelkwan]

@michaelkwan take a look at PR #5024 and let me know what you think.

@lachlan-roberts thanks, i think that's sufficient.

[lachlan-roberts]

@michaelkwan we have also addressed the issue about cancelling the future in PR #5030, now if the Future is cancelled it will also abort the upgrade request.

Both these fixes will be in the 9.4.31 release which should be out in the next week or so.