search

eclipse-lsp4j / lsp4j

A Java implementation of the language server protocol intended to be consumed by tools and language servers implemented in Java.
https://eclipse.org/lsp4j
Other
613 stars 145 forks source link

Tomcat TEXT_FULL_WRITING state exception while multiple messages sent from server to client #632

Closed ivy-lli closed 2 years ago

ivy-lli commented 2 years ago

Hi

We use GLSP to display a diagram. GLSP itself uses LSP (LSP4J) as communication layer between server and client. As our application already has a Tomcat web server we wanted to reuse (same port etc.) this one instead of provide something new like Jetty.

But now we run into many exceptions in our log, that a message couldn't be sent to the client:

[WARN ][org.eclipse.lsp4j.jsonrpc.RemoteEndpoint][IvyGlspActionExecutor-1]{}
Failed to send notification message.
java.lang.IllegalStateException: The remote endpoint was in state [TEXT_FULL_WRITING] which is an invalid state for called method
    at org.apache.tomcat.websocket.WsRemoteEndpointImplBase$StateMachine.checkState(WsRemoteEndpointImplBase.java:1274)
    at org.apache.tomcat.websocket.WsRemoteEndpointImplBase$StateMachine.textStart(WsRemoteEndpointImplBase.java:1236)
    at org.apache.tomcat.websocket.WsRemoteEndpointImplBase.sendStringByCompletion(WsRemoteEndpointImplBase.java:213)
    at org.apache.tomcat.websocket.WsRemoteEndpointImplBase.sendStringByFuture(WsRemoteEndpointImplBase.java:201)
    at org.apache.tomcat.websocket.WsRemoteEndpointAsync.sendText(WsRemoteEndpointAsync.java:53)
    at org.eclipse.lsp4j.websocket.WebSocketMessageConsumer.sendMessage(WebSocketMessageConsumer.java:57)
    at org.eclipse.lsp4j.websocket.WebSocketMessageConsumer.consume(WebSocketMessageConsumer.java:47)
    at org.eclipse.lsp4j.jsonrpc.RemoteEndpoint.notify(RemoteEndpoint.java:126)
    at org.eclipse.lsp4j.jsonrpc.services.EndpointProxy.invoke(EndpointProxy.java:88)
    at com.sun.proxy.$Proxy102.process(Unknown Source)
    at org.eclipse.glsp.server.actions.ClientActionHandler.send(ClientActionHandler.java:58)
    at org.eclipse.glsp.server.actions.ClientActionHandler.execute(ClientActionHandler.java:52)
    at ch.ivyteam.glsp.core.action.IvyActionDispatcher.runAction(IvyActionDispatcher.java:110)
    at ch.ivyteam.glsp.core.action.IvyActionDispatcher.handleAction(IvyActionDispatcher.java:89)
    at ch.ivyteam.glsp.core.action.IvyActionDispatcher.dispatch(IvyActionDispatcher.java:56)
    at org.eclipse.glsp.server.features.core.model.RequestModelActionHandler.notifyFinishedLoading(RequestModelActionHandler.java:68)
    at org.eclipse.glsp.server.features.core.model.RequestModelActionHandler.executeAction(RequestModelActionHandler.java:54)
    at org.eclipse.glsp.server.features.core.model.RequestModelActionHandler.executeAction(RequestModelActionHandler.java:1)
    at org.eclipse.glsp.server.actions.AbstractActionHandler.execute(AbstractActionHandler.java:53)
    at ch.ivyteam.glsp.core.action.IvyActionDispatcher.runAction(IvyActionDispatcher.java:110)
    at ch.ivyteam.glsp.core.action.IvyActionDispatcher.handleAction(IvyActionDispatcher.java:89)
    at ch.ivyteam.glsp.core.action.IvyActionDispatcher.lambda$0(IvyActionDispatcher.java:70)
    at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1128)
    at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:628)
    at java.base/java.lang.Thread.run(Thread.java:829)

After some code investigations and research, this sometime happens when multiple messages are sent to the client at the same "time" (or better quickly after each other). It seems that LSP4J sends here messages without waiting if the last message sending is completed: WebSocketMessageConsumer#sendMessage The sendText method would return a Future<Void> to check if the message is sent completely.

Tomcat will directly try to send those messages to the client without waiting, it only checks the state of the connection. And because of this the exception above appears:

I've also done some quick researches in Jetty and it seems that there is a queue implemented which handles this "multiple" messages internally (but I'm not sure if I'm right): https://github.com/eclipse/jetty.project/blob/jetty-9.4.x/jetty-websocket/websocket-common/src/main/java/org/eclipse/jetty/websocket/common/io/FrameFlusher.java#L78

I'm also not sure what is defined by the WebSocket protocol or the Java API if this should be handled by the server or the protocol (https://tomcat.apache.org/tomcat-9.0-doc/web-socket-howto.html). But if I understood it correctly form an older Tomcat issue, sending multiple messages without waiting is not supported: https://bz.apache.org/bugzilla/show_bug.cgi?id=63931#c1

And I think there is also no way to detect it outside of LSP4J e.g. in the GLSPClient#process, as there is no return value or exception (as LSP4J catches the exception and simply log it: RemoteEndpoint#notify.

So in the end I think the WebSocketMessageConsumer#sendMessage should hold a queue of messages and only send a new one if the previous is sent completely. Or did I understand something wrong? Thank for you help 🙂

Currently in charge:

jonahgraham commented 2 years ago

@ivy-lli Sorry that no one has an answer on this. The core developers of LSP4J don't use websockets much and the websockets bundles have been provided primarily as a convenience for consumers to have that shared code here. If you have a suggested PR I can try to review it.

I am tagging some of the known websocket LSP4J users in case they have more insight - they may not have seen your original issue.

cc: @mitasov-ra @apupier @spoenemann

apupier commented 2 years ago

I am tagging some of the known websocket LSP4J users in case they have more insight - they may not have seen your original issue. Effectively not noticed the issue :-)

I never seen the reported of error. But I used Websocket mostly in Proof of Concept so far.

@ivy-lli Your analysis seems to be consistent and good.

spoenemann commented 2 years ago

The approach with a queue sounds good. But strange that the websocket implementation doesn't take care of it by itself.

ivy-lli commented 2 years ago

@spoenemann I've found another bug request in the tomcat bugzilla archive about this topic: https://bz.apache.org/bugzilla/show_bug.cgi?id=56026

It seems that they strictly followed the spec and the spec don't seems to clearly define if the server or the application should handle this.