quarkusio / quarkus

Quarkus: Supersonic Subatomic Java.
https://quarkus.io
Apache License 2.0
13.77k stars 2.68k forks source link

overriding ServerEndpointConfig.Configurator's 'modifyHandshake' with blocking code #29238

Open spaykit opened 1 year ago

spaykit commented 1 year ago

Describe the bug

Hey, I'm modifying the handshake of Websocket by implementing ServerEndpointConfig.Configurator and overriding 'modifyHandshake', but the code is blocking and running on IO thread, how can I force it to run on a worker thread? 'quarkus.websocket.dispatch-to-worker=true' is working only for @ServerEndpoint @OnOpen.

I tried to annotate the 'modifyHandshake' with @Blocking but still - it's running on an IO thread.

Expected behavior

modifyHandshake should be invoked on a worker thread.

Actual behavior

modifyHandshake invoked on an IO thread.

How to Reproduce?

public class WebSocketEndpointConfigurator extends ServerEndpointConfig.Configurator {

  @Override 
  public void modifyHandshake(ServerEndpointConfig config, HandshakeRequest request, HandshakeResponse response) {
  // executing blocking code
  Thread.sleep(3000)
  }
}

Output of uname -a or ver

No response

Output of java -version

No response

GraalVM version (if different from Java)

No response

Quarkus version or git rev

2.4.1.Final

Build tool (ie. output of mvnw --version or gradlew --version)

No response

Additional information

No response

cescoffier commented 1 month ago

@mkouba do you think this use case works when using websocket-next?

The issue here is in io.undertow.websockets.handshake.HandshakeUtil#prepareUpgrade. This code is unaware of the different execution model Quarkus is proposing and does not check for @Blocking / @RunOnVirtualThread. Also, we cannot make this method async (the next instruction expects the side effects).

One possibility would be to change the behavior of io.undertow.websockets.vertx.VertxWebSocketHandler#handle and use a worker thread if quarkus.websocket.dispatch-to-worker=true. But there might be consequences I'm not aware of.

mkouba commented 1 month ago

@mkouba do you think this use case works when using websocket-next?

@cescoffier I don't think it works because the HttpUpgradeCheck is also called on the event loop.

I wonder if we should add some util methods to the VertxContextSupport to execute blocking code from an event loop, i.e. something like Uni<T> executeBlocking(Callable<T> callable) and void executeBlocking(Runnable runnable) :shrug:.

cescoffier commented 1 month ago

We could, however, that makes the caller async ( in the sense it will need to return a Uni). Would it be possible in this case?

It's clearly not possible inio.undertow.websockets.handshake.HandshakeUtil#prepareUpgrade.

mkouba commented 1 month ago

We could, however, that makes the caller async ( in the sense it will need to return a Uni). Would it be possible in this case?

It could return Uni created lazily from the Callable, i.e. something like Uni.createFrom().completionStage(() -> context.executeBlocking(callable, false).toCompletionStage()), or?

It's clearly not possible inio.undertow.websockets.handshake.HandshakeUtil#prepareUpgrade.

Yes, this would not help there.

mkouba commented 1 month ago

We could, however, that makes the caller async ( in the sense it will need to return a Uni). Would it be possible in this case?

It could return Uni created lazily from the Callable, i.e. something like Uni.createFrom().completionStage(() -> context.executeBlocking(callable, false).toCompletionStage()), or?

@cescoffier Something like this: https://github.com/quarkusio/quarkus/pull/43444?