Asssociate timeout to underlying Jetty HTTP client #24007
Conversation
In some cases, org.eclipse.jetty.websocket.client.WebSocketClient.connect(listener, uri, request) call will return a future that never is completed. It is reasonable that our future.get() must have a timeout to avoid thread blocking.
I suggest something like:
Callable<WebSocketSession> connectTask = () -> {
Future<Session> future = this.client.connect(listener, uri, request);
try {
// TODO Configurable timeout
future.get(2000, TimeUnit.MILLISECONDS);
} catch (Exception ex){
logger.error("Failed to connect to remote websocket endpoint", ex);
future.cancel(true); // This method will stop the running underlying task
}
return wsSession;
};
Connection timeout is now associated to Jetty client timeout plus 50ms extra padding.
Timeout must consider also the idle timeout of the Jetty client
Change idle Jetty max idle timeout for underlying HTTP Client
spring-projects-issues
added
the
status: waiting-for-triage
|
The timeout part has already been done (see the commit that closed #23994). As for cancelling the future, as far as I can see in Jetty's |
rstoyanchev
added
status: duplicate
|
Ok, Rossen. One note about the current timeout implementation. I think that this.client.getHttpClient().getIdleTimeout() must be added to HTTP client connect timeout because the websocket upgrade can take also the request "read time" |
|
@Kukosoft, I agree handshake time isn't quite the same as the connect time, but If you goal is to have configurable control, you already can change |
The websocket handshake timeout is now the sum of connection timeout, idle tiemout and 1 margin second.