When using HandshakeInterceptor with ServerHttpAsyncRequestControl, Jetty10RequestUpgradeStrategy throws exception due to null servletContext

[ajr3-gen]

We are upgrading our project from Jetty v9 to v10 and have encountered a problem with the new spring-boot Jetty implementation.

Our WebSocketController includes a custom HandshakeInterceptor. In the beforeHandshake() method, we use this line:

        ServerHttpAsyncRequestControl requestControl = request.getAsyncRequestControl(response);
        requestControl.start();

so that we can authorize the request before completing it. We do that authorization in a separate thread and then call DefaultHandshakeHandler to complete the handshake:

        handshakeHandler.doHandshake(request, response, wsHandler, attributes);
        requestControl.complete();

while the beforeHandshake method exits on the main thread.

This worked fine with Jetty v9 (which is implemented in spring-boot via the class org.springframework.web.socket.server.jetty.JettyRequestUpgradeStrategy). However, with Jetty v10 (which uses a new class: org.springframework.web.socket.server.jetty.Jetty10RequestUpgradeStrategy), there is a NullPointerException. This is because Jetty10RequestUpgradeStrategy uses reflection to invoke JettyWebSocketServerContainer getContainer(ServletContext servletContext) which tries to resolve an attribute from the request's servletContext -- but servletContext has now been cleared out as we have exited the beforeHandshake() method on the main thread.

I confirmed this by adding a Thread.sleep() to our beforeHandshake() method to delay it from returning for a couple of seconds, and this allowed the thread with DefaultHandshakeHandler to complete. Without this delay, we catch this error and the handshake does not complete:
org.springframework.web.socket.server.HandshakeFailureException: Failed to upgrade; nested exception is java.lang.NullPointerException

Versions used:
spring.boot = 2.5.6
jetty.version = 10.0.6
spring.framework = 5.3.12

[ajr3-gen]

I can't include our product code here, but here's a very simplified example of our relevant class. (I'll try to work up a sample project to duplicate the problem for you.)

package com.genesys.hawk.controller;

import javax.servlet.ServletContext;
import java.util.concurrent.*;
//etc...

@Controller
public class SampleWebSocketController extends TextWebSocketHandler implements HandshakeInterceptor, ServletContextAware {

    private static final Logger LOG = LoggerName.getLoggerTrimmed(SampleWebSocketController.class);
    private final ChannelAuthCache channelAuthCache;
    private HandshakeHandler handshakeHandler;

    @Autowired
    public SampleWebSocketController(ChannelAuthCache channelAuthCache) {
        this.channelAuthCache = channelAuthCache;
        handshakeHandler = new DefaultHandshakeHandler();
    }

    @Override
    public boolean beforeHandshake(
            ServerHttpRequest request,
            ServerHttpResponse response,
            WebSocketHandler wsHandler,
            Map<String, Object> attributes) {

        // Get channel ID from request.  This is the value we need to authorize before performing the handshake.
        String channelId = request.getURI().getPath().split("/")[1];

        // Put the request into async mode
        ServerHttpAsyncRequestControl requestControl = request.getAsyncRequestControl(response);
        requestControl.start();

        // Note that request.getServletRequest().getServletContext() would return the correct context here.
        
        // Authorize the request; authorizeChannel() returns a Future and runs on a new thread
        channelAuthCache.authorizeChannel(channelId)
                .then(authResp -> {
                    // !!! At this point, request.getServletRequest().getServletContext() now returns a NULL value
                            
                    handshakeHandler.doHandshake(request, response, wsHandler, attributes);
                    requestControl.complete();
                })
                .fail(error -> {
                    if (error instanceof ChannelNotFoundException ||
                            error instanceof UnauthorizedException) {
                        LOG.warn("Failed to authorize channel {}: {}", channelId, error.getMessage());
                        response.setStatusCode(HttpStatus.UNAUTHORIZED);
                    } else {
                        // !!! This is where we catch the error from Jetty10RequestUpgradeStrategy
                        // !!! org.springframework.web.socket.server.HandshakeFailureException: Failed to upgrade; nested exception is java.lang.NullPointerException
                        LOG.error("Unknown error authorizing channel {}", channelId, error);
                        response.setStatusCode(HttpStatus.INTERNAL_SERVER_ERROR);
                    }
                    requestControl.complete();
                });

        // Adding this thread block will pause the main thread long enough for the handshake to complete.
        /*
        try {
            Thread.sleep(5000);
        } catch (InterruptedException ie) {
            // no-op
        }
        */

        // Prevent handshake until requests complete
        return false;
    }

    @Override
    public void afterHandshake(
            ServerHttpRequest request,
            ServerHttpResponse response,
            WebSocketHandler wsHandler,
            Exception exception) {
        // Nothing to do here
    }

    public HandshakeHandler getHandshakeHandler() {
        return handshakeHandler;
    }

}

[ajr3-gen]

I have created a sample project to demonstrate this bug.
You should be able to:

• Clone this repo: https://github.com/ajr3-gen/spring-boot-28525

• Run mvn clean package

• Run java -jar target/jetty-websocket-demo-0.0.1-SNAPSHOT.jar

• Use Postman v8.5.0 or later to test it. (Opening a websocket is a fairly new capability of Postman; this page explains it if you are unfamiliar.)

• To see good, working behavior, try to open a WebSocket connection to ws://localhost:8080/websocket/good. You will be able to connect, send a message to the server, receive a reply, and disconnect.

• To see the error, try to open a WebSocket connection to ws://localhost:8080/websocket/bad. You will not be able to connect, and if you check the console you will see log messages that show that servletContext is null and that the handshake has encountered a NPE.

The pertinent code can be seen in the files GoodHandshakeInterceptor.java and BadHandshakeInterceptor.java respectively. I hope it's clear. Let me know if you have questions.

Thanks.

[ajr3-gen]

Has anyone had a chance to look into this issue yet? Our upgrade to Jetty v10 is on hold waiting for this. Thanks.

[ajr3-gen]

@snicoll @rstoyanchev Can I please get someone to look at this? We're going to have to upgrade to Jetty v10 eventually, and I'd like to get this blocking issue resolved before it becomes urgent. Thanks.

[rstoyanchev]

What you're doing with an asynchronous request before a handshake is not at all an expected scenario. Even from a Servlet container perspective, when you get off the Servlet container thread, you are expected to use AsyncContext#dispatch to continue processing on a Servlet container thread in order for everything to work.

I can see that we don't expose dispatch on ServerHttpAsyncRequestControl but I'm not sure why you need to use that abstraction and not the HttpServletRequest directly especially for something like this. This brings back the point this isn't at all something expected.

You can consider simply blocking for the authentication, or using a redirect, or you can try starting an async request directly through the Servlet API and then dispatching back to the same URL, but again I don't know if you will run into any other issues with that.

[spring-projects-issues]

If you would like us to look at this issue, please provide the requested information. If the information is not provided within the next 7 days this issue will be closed.

[ajr3-gen]

Thanks for getting back to me. I'm afraid I'm having some trouble seeing the solution, though. One of your suggestions is to block for the authentication, but I don't want to do that because it defeats the point of being asynchronous. You also suggested a redirect, but (a) we don't want to change the behavior our clients are expecting, and (b) websocket clients aren't guaranteed to honor redirects (according to this: https://stackoverflow.com/questions/46962881/is-it-possible-to-seamlessly-redirect-websockets).

That leaves the third option, of dispatching to the same URL through the Servlet API. I tried that, but it just caused a loop that exhausted memory, so either it doesn't work or I am not following what you had in mind. Maybe you could expand on that?

I will say that ServerHttpAsyncRequestControl seemed like the right thing to use here, based on its brief description in the API Javadoc: "A control that can put the processing of an HTTP request in asynchronous mode during which the response remains open until explicitly closed." That's what we're doing here, and it did work with Jetty9.

[rstoyanchev]

The Java/Jakarta EE WebSocket API did not make it possible to initiate a WebSocket handshake at runtime and we've had to find ways to do it on each container. The way we did it on Jetty 9 made it possible to do what you want to do. However, the Jetty WebSocket API in Jetty changed significantly from 9 to 10, and we had to change too, even request an API change to enable what we needed to do, see jetty/jetty.project#5866. Now we're using the new API and the upgrade requires access to the ServletContext. You can see the same in the main branch which compiles against Jetty 10 and does not need reflection, i.e. this is not related to the use of reflection.

I'm afraid I don't know what else we can do about this. It's what we have to do in order to upgrade on Jetty 10+. I also don't know what your application does with WebSocket (STOMP, raw WebSocket, etc) to be able to suggest other changes like using WebFlux or otherwise dropping on a lower level, but I don't see any easy answers.

[spring-projects-issues]

If you would like us to look at this issue, please provide the requested information. If the information is not provided within the next 7 days this issue will be closed.

[ajr3-gen]

Thanks for the feedback. I guess we'll have to try to figure something else out. I may have more questions as we work at it, but for now it seems clear that the current logic won't work with Jetty 10.

[rstoyanchev]

Okay, I'm closing for now, but feel free to add comments if needed.