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

WebSocketGraphQlInterceptor is not notified when idle connection is closed #872

Closed
rsi2m opened this issue Jan 5, 2024 · 1 comment
Closed
Assignees
Labels
type: bug A general bug
Milestone

Comments

@rsi2m
Copy link

rsi2m commented Jan 5, 2024

Back in 528 it was said that:

There is also a more global WebSocketGraphQlInterceptor that lets you be notified when a connection is closed, and/or when a subscription is cancelled.

But in case of websocket connection timing out on a server side, webSocketGraphQlInterceptor.handleConnectionClosed is not invoked. Relevant exception:

org.eclipse.jetty.websocket.api.exceptions.WebSocketTimeoutException: Connection Idle Timeout
	at org.eclipse.jetty.websocket.common.JettyWebSocketFrameHandler.convertCause(JettyWebSocketFrameHandler.java:541)
	at org.eclipse.jetty.websocket.common.JettyWebSocketFrameHandler.onError(JettyWebSocketFrameHandler.java:259)
	at org.eclipse.jetty.websocket.core.internal.WebSocketCoreSession.lambda$closeConnection$2(WebSocketCoreSession.java:284)
	at org.eclipse.jetty.server.handler.ContextHandler.handle(ContextHandler.java:1466)
	at org.eclipse.jetty.server.handler.ContextHandler.handle(ContextHandler.java:1485)
	at org.eclipse.jetty.websocket.core.server.internal.AbstractHandshaker$1.handle(AbstractHandshaker.java:212)
	at org.eclipse.jetty.websocket.core.internal.WebSocketCoreSession.closeConnection(WebSocketCoreSession.java:284)
	at org.eclipse.jetty.websocket.core.internal.WebSocketCoreSession.lambda$sendFrame$7(WebSocketCoreSession.java:519)
	at org.eclipse.jetty.util.Callback$3.succeeded(Callback.java:155)
	at org.eclipse.jetty.websocket.core.internal.TransformingFlusher.notifyCallbackSuccess(TransformingFlusher.java:197)
	at org.eclipse.jetty.websocket.core.internal.TransformingFlusher$Flusher.process(TransformingFlusher.java:154)
	at org.eclipse.jetty.util.IteratingCallback.processing(IteratingCallback.java:243)
	at org.eclipse.jetty.util.IteratingCallback.iterate(IteratingCallback.java:224)
	at org.eclipse.jetty.websocket.core.internal.TransformingFlusher.sendFrame(TransformingFlusher.java:77)
	at org.eclipse.jetty.websocket.core.internal.WebSocketCoreSession.sendFrame(WebSocketCoreSession.java:522)
	at org.eclipse.jetty.websocket.core.internal.WebSocketCoreSession.close(WebSocketCoreSession.java:239)
	at org.eclipse.jetty.websocket.core.internal.WebSocketCoreSession.processHandlerError(WebSocketCoreSession.java:371)
	at org.eclipse.jetty.websocket.core.internal.WebSocketConnection.onIdleExpired(WebSocketConnection.java:242)
	at org.eclipse.jetty.io.AbstractEndPoint.onIdleExpired(AbstractEndPoint.java:407)
	at org.eclipse.jetty.io.IdleTimeout.checkIdleTimeout(IdleTimeout.java:170)
	at org.eclipse.jetty.io.IdleTimeout.idleCheck(IdleTimeout.java:112)
	at java.base/java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:539)
	at java.base/java.util.concurrent.FutureTask.run$$$capture(FutureTask.java:264)
	at java.base/java.util.concurrent.FutureTask.run(FutureTask.java)
	at java.base/java.util.concurrent.ScheduledThreadPoolExecutor$ScheduledFutureTask.run(ScheduledThreadPoolExecutor.java:304)
	at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1136)
	at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:635)
	at java.base/java.lang.Thread.run(Thread.java:833)
Caused by: org.eclipse.jetty.websocket.core.exception.WebSocketTimeoutException: Connection Idle Timeout
	... 11 common frames omitted

I suspect, that happens because GraphQlWebSocketHandler's handleTransportError method ( removing the session from the sessionInfoMap) is called before afterConnectionClosed.
See source code:

public void afterConnectionClosed(WebSocketSession session, CloseStatus closeStatus) {
  String id = session.getId();
  SessionState state = this.sessionInfoMap.remove(id);
  if (state != null) {
	  state.dispose();
	  Map<String, Object> connectionInitPayload = state.getConnectionInitPayload();
	  if (connectionInitPayload != null) {
		  this.webSocketGraphQlInterceptor.handleConnectionClosed(
				  state.getSessionInfo(), closeStatus.getCode(), connectionInitPayload);
	  }
  }
}

I'm wondering is this intentional that handleConnectionClosed is not called in case of idle connection is closed? If yes, what should one do to actually be notified about connection closure?
If no, what would be an appropriate fix or workaround for that? One straightforward workaround would be to override methods of GraphQlWebSocketHandler and handle session closure there, WDYT?

Thinking of a proper fix, does transport error always results in a connection getting closed? If yes, then the session removal from sessionInfoMap could be done only in afterConnectionClosed method?

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Jan 5, 2024
@bclozel bclozel changed the title [Question] WebSocketGraphQlInterceptor is not notified when idle connection is closed WebSocketGraphQlInterceptor is not notified when idle connection is closed Jan 5, 2024
@rstoyanchev
Copy link
Contributor

It's not intentional, and it's something that needs fixing. We need to either ensure that handleConnectionClosed is closed from both handleTransportError and afterConnectionClosed, or alternatively, only cancel subscriptions from handleTransportError and allow the session to be removed from afterConnectionClosed.

@rstoyanchev rstoyanchev self-assigned this Mar 7, 2024
@rstoyanchev rstoyanchev added type: bug A general bug and removed status: waiting-for-triage An issue we've not yet triaged labels Mar 7, 2024
@rstoyanchev rstoyanchev added this to the 1.2.6 milestone Mar 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug A general bug
Projects
None yet
Development

No branches or pull requests

3 participants