Jakarta websocket @OnMessage with Reader parameter stops working when there is an unhandled exception about jetty.project HOT 12 CLOSED

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

• #10485

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

from jetty.project.

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?

from jetty.project.

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.

from jetty.project.

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

from jetty.project.

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) ->...

from jetty.project.

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.

from jetty.project.

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.

from jetty.project.

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.

from jetty.project.

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.

from jetty.project.

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.

from jetty.project.

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.

from jetty.project.

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!.

from jetty.project.