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

Avoid indefinite wait to connect in JettyWebSocketClient #23994

Closed
wants to merge 1 commit into from

Conversation

@Kukosoft
Copy link

Kukosoft commented Nov 14, 2019

In some cases, org.eclipse.jetty.websocket.client.WebSocketClient.connect(listener, uri, request) call will return a future that is never 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;
};
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;
		};
@pivotal-issuemaster

This comment has been minimized.

Copy link

pivotal-issuemaster commented Nov 14, 2019

@Kukosoft Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

@pivotal-issuemaster

This comment has been minimized.

Copy link

pivotal-issuemaster commented Nov 14, 2019

@Kukosoft Thank you for signing the Contributor License Agreement!

@rstoyanchev

This comment has been minimized.

Copy link
Contributor

rstoyanchev commented Nov 14, 2019

I've edited your comment to improve the formatting. You might want to check out this Mastering Markdown guide for future reference.

@rstoyanchev

This comment has been minimized.

Copy link
Contributor

rstoyanchev commented Nov 14, 2019

A connect timeout is better set at the level of the underlying HttpClient. In this case, org.eclipse.jetty.websocket.client.WebSocketClient does have a connectTimeout property that is set to 15 seconds by default but you can change that.

@Kukosoft

This comment has been minimized.

Copy link
Author

Kukosoft commented Nov 14, 2019

A connect timeout is better set at the level of the underlying HttpClient. In this case, org.eclipse.jetty.websocket.client.WebSocketClient does have a connectTimeout property that is set to 15 seconds by default but you can change that.

In practice, in a performance test I have set this connectTimeout to 500ms and I have few threads blocked in the get() method indefinitely...

JettyThreadPool-456
priority:5 - threadId:0x00007fbf041f9800 - nativeId:0x5ca3 - nativeId (decimal):23715 - state:WAITING
stackTrace:
java.lang.Thread.State: WAITING (parking)
at sun.misc.Unsafe.park(Native Method)
- parking to wait for <0x0000000547886900> (a java.util.concurrent.CompletableFuture$Signaller)
at java.util.concurrent.locks.LockSupport.park(LockSupport.java:175)
at java.util.concurrent.CompletableFuture$Signaller.block(CompletableFuture.java:1693)
at java.util.concurrent.ForkJoinPool.managedBlock(ForkJoinPool.java:3323)
at java.util.concurrent.CompletableFuture.waitingGet(CompletableFuture.java:1729)
at java.util.concurrent.CompletableFuture.get(CompletableFuture.java:1895)
at org.springframework.web.socket.client.jetty.JettyWebSocketClient.lambda$doHandshakeInternal$0(JettyWebSocketClient.java:158)
at org.springframework.web.socket.client.jetty.JettyWebSocketClient$$Lambda$854/1015804107.call(Unknown Source)
at java.util.concurrent.FutureTask.run(FutureTask.java:266)
at org.springframework.web.socket.client.jetty.JettyWebSocketClient.doHandshakeInternal(JettyWebSocketClient.java:167)
at org.springframework.web.socket.client.AbstractWebSocketClient.doHandshake(AbstractWebSocketClient.java:99)
at org.springframework.web.socket.client.jetty.JettyWebSocketClient.doHandshake(JettyWebSocketClient.java:135)
...

I think we need to protect against any bug from third party libraries.
Could you check this out, please?
Thanks.

@rstoyanchev

This comment has been minimized.

Copy link
Contributor

rstoyanchev commented Nov 14, 2019

Okay we could get the connectTimeout from the Jetty WebClient and wait for as long or a little longer. I don't think we should be exposing the same property for this though. It sounds like an issue that should be reported to Jetty because if the Jetty HttpClient keeps getting stuck, giving up on is also an issue.

@rstoyanchev rstoyanchev reopened this Nov 14, 2019
@rstoyanchev rstoyanchev self-assigned this Nov 14, 2019
@rstoyanchev rstoyanchev added this to the 5.2.2 milestone Nov 14, 2019
@rstoyanchev rstoyanchev changed the title Set a timeout in Future<Session> Avoid indefinite wait to connect in JettyWebSocketClient Nov 14, 2019
@Kukosoft

This comment has been minimized.

Copy link
Author

Kukosoft commented Nov 14, 2019

Okay we could get the connectTimeout from the Jetty WebClient and wait for as long or a little longer. I don't think we should be exposing the same property for this though. It sounds like an issue that should be reported to Jetty because if the Jetty HttpClient is stuck and keep giving up, that is also an issue.

I'm totally agree with you. Jetty WebSocketClient is sometimes and somehow not completing its Future connection, so, Spring's side needs a protection like you said.
Okay with connectTimeout+paddingTime, but what about throwing an exception inside the callable task? How to inform the ListenableFuture about this timeout?

@rstoyanchev

This comment has been minimized.

Copy link
Contributor

rstoyanchev commented Nov 14, 2019

If the Callable fails, listeners of the ListenableFuture will be notified (see ListenableFutureTask).

stsypanov added a commit to stsypanov/spring-framework that referenced this pull request Nov 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.