Jakarta websocket @OnMessage with Reader parameter stops working when there is an unhandled exception

[natan-abolafya]

Jetty version(s)
11.0.19

Java version/vendor openjdk 21.0.1

OS type/version Ubuntu 22.04

Description
I've been debugging and trying to figure out a bug where our application stops getting websocket messages while the websocket is clearly alive, responding to pings and it is sending messages initiated from other places in the code.
Turns out the database goes down at some point which leads to a RuntimeException in the code where the incoming messages are handled (@OnMessage). The exception seems to be silently ignored and the whole websocket is basically stuck in one direction. UPDATE: seems to only happen when Reader or InputStream parameters are used.

I suppose it is connected to this:

Only one thread at a time will be delivering a message event to the onMessage method; the next message event will not be delivered until the previous call to the onMessage method has exited.

Except the lock (assuming there is a lock) is never released on that websocket connection when an exception occurs. And I think it should be released.

How to reproduce?

  @OnMessage
  public String onMessage(final Session session, Reader reader) {
      // here be a code that might throw an exception depending on circumstances or a bug
  } 

[joakime]

Smells like a bug in our impl.

BTW, Do you have a @OnError and @OnClose methods specified?
If so, what do they do when the exception occurs?

[joakime]

Note: Jetty 10 / 11 has reached End of Community Support

• EoCS (End of Community Support) for Jetty 10 / Jetty 11 - January 2024 #10485

Should this bug exist in Jetty 12, it will be fixed there.

[natan-abolafya]

BTW, Do you have a @onerror and @onclose methods specified?

yes, I do. @OnError just logs stuff. @OnClose just removes a websocket reference from a map. But I'm quite sure neither is called when this issue occurs.

[lachlan-roberts]

@natan-abolafya are you using websocket permessage-deflate extension?

[natan-abolafya]

that is not something I've enabled intentionally. This is what I use to integrate jetty websocket to my dropwizard application which maybe enables it under the hood?

JakartaWebSocketServletContainerInitializer.configure(environment.getApplicationContext(), (servletContext, wsContainer) ->...

[lachlan-roberts]

I can't seem to reproduce this.

Whenever an exception is thrown we immediately catch and fail the callback, after failing the callback the connection will be closed then @WebSocketError will be called and then @OnWebSocketClose. So I'm not sure how this could be possible.

Could you provide some debug logs when you experience this issue. Or perhaps a small example project which can reproduce it.

[natan-abolafya]

I was afraid of this :) but to be fair, it would have been caught earlier if that was easy to reproduce.

So, I've done some testing on my application where I made other changes and could not reproduce. It is behaving exactly as you said. Then I remembered that I had changed the @OnMessage parameter from Reader reader to String message.
So, if I switch back to Reader reader, then I can reproduce it.

  @OnMessage
  public String onMessage(final Session session, Reader reader) throws IOException {
    throw new RuntimeException("it will get stuck nowt");
  }

I think I had also tried InputStream to see if that helps when I was working with this problem and it was the same. So I guess it's related to the parameter being Closeable. Updating the title and description for this.

[joakime]

Note that using Reader or InputStream in your @OnMessage causes an extra thread to be used for each message.

Those two signatures, while valid, should be used sparingly as they incur quite a large hit on performance.
They are typically used for very long lived messages (eg: webrtc, video conferencing, streaming), where the message could last hours or days.

Devs often want to use Reader / InputStream because they want to reduce memory vs a whole message delivery.
But that reduction in memory is wiped away due to the new threading requirements.
Even having maxMessageSize set to dozens of megabytes for whole message delivery is often a better choice (performance wise) than using Reader / InputStream.

Another option you have is to use the "ByteBuffer / boolean" pair (for BINARY) or "String / boolean" pair (for TEXT).
That will be partial messages, with the boolean indicating if it is the last part or not.
This is a bit trickier than either whole message or stream based, but it offers a choice for those making API decisions based on performance.

[natan-abolafya]

Hey. Thanks for the insight. Quite interesting.
The reason we used it I think was an misunderstanding or maybe it was working differently. It was long ago so I don't remember the details exactly but we wanted parallel handling of messages so if a @OnMessage calls takes too long, the next message can still be processed and send a response back. As we use jsonrpc with request IDs, this is logically ok. So it wasn't really about handling long-lived messages. But since this was just handling maximum 100 connections, we've never noticed any performance issue.

When we tested at the time, if we used Reader instead of String, it was working like that (parallel message handling). Or we ran the tests wrong and misled ourselves, hard to say now. In that sense, using up a thread made was ok. Since I can't reproduce that behavior anymore, I moved back to String, so now I don't hit the issue with the exception.
In case you're wondering, I've solved the parallel handling by creating a virtual thread whenever a request that would require some work (like DB access and such).

Thread.ofVirtual().start(() -> {
   var response = doStuff();
   session.getBasicRemote().sendText(response));
  });

Anyway, the issue is still valid I suppose. It caused quite a lot of headache as it was quite hard to figure out (we've pursued quite a few red herrings :)). Would be good to spare other people hitting this IMHO.

[joakime]

The reason we used it I think was an misunderstanding or maybe it was working differently. It was long ago so I don't remember the details exactly but we wanted parallel handling of messages so if a @OnMessage calls takes too long, the next message can still be processed and send a response back. As we use jsonrpc with request IDs, this is logically ok. So it wasn't really about handling long-lived messages. But since this was just handling maximum 100 connections, we've never noticed any performance issue.

You can still do that by accepting the raw websocket message and sending that out (or queuing) to a thread that your app controls to handle the messages.
That will let the raw websocket messages to continue flowing to your code (in the order they were received by the websocket protocol).

If that handling queue is getting too big, just don't exit the @OnMessage call to prevent the websocket protocol handling from continuing.
Normal networking backpressure can then start to accrue.

[lachlan-roberts]

Interesting, seems like this is a bug. I can see a case where this could happen specifically when using the Reader or InputStream message signatures.

As a workaround I would just catch the exception and do a session.close(1011, "your error message"). But you shouldn't encounter this issue with String in the @OnMessage signature if you chose to switch to that.

[natan-abolafya]

yep, that's what I've ended up doing in the end and at the same time changing it to String without knowing :). Thanks for all the help!.