@EnableWebSocketMessageBroker breaks Jetty integration testing in 5.0.x [SPR-16263]

[spring-projects-issues]

Jared Jacobs opened SPR-16263 and commented

When we attempted to migrate our codebase from 4.3.3 to 5.0.2, our controller integration tests all failed, because our ApplicationContext in the test environment couldn't be created, since we have one controller that uses STOMP over a web socket. We actually aren't even testing STOMP messages in our integration tests. We only test simple HTTP requests, but again, those tests won't run because the ApplicationContext can't be created. This looks like a regression from 4.3.3.

I reproduced the issue easily using an entirely different codebase, a sample application written to showcase testing of spring-websocket. See the example test at the Reference URL above (note the fork).

Problems:

1. WebSocketServerFactory.doStart() throws an IllegalStateException because it doesn't have a DecoratedObjectFactory. This can be worked around pretty easily by setting the DecoratedObjectFactory.ATTR attribute on the ServletContext.
2. WebSocketServerFactory throws a NullPointerException because it doesn't have an Executor and can't get one via a ContextHandler. This is because Spring's MockServletContext doesn't implement Jetty's ContextHandler.Context. I can’t seem to find a workaround because these spring-websocket types seem to form a chain of construction via new:

• WebMvcStompWebSocketEndpointRegistration
• DefaultSockJsService
• DefaultHandshakeHandler
• JettyRequestUpgradeStrategy
• WebSocketServerFactory

For the time being, we are working around the issue by omitting the web socket mappings from our integration test environment (no longer using @EnableWebSocketMessageBroker in our integration tests). Until this issue is fixed, it seems we no longer have the option of adding tests that exercise the web socket.

---

Affects: 5.0.2

Reference URL: https://github.com/2is10/spring-websocket-portfolio/blob/master/src/test/java/org/springframework/samples/portfolio/web/jetty/JettyIntegrationTest.java

[spring-projects-issues]

Rossen Stoyanchev commented

I guess WebSocketServerFactory expects to be running in a server. I don't know what the right way to initialize it is for a server-less test but the JettyRequestUpgradeStrategy has a constructor that accepts an already created instance. You can plug it in like this in WebSocketConfig (based on the spring-websocket-portfolio sample):

	@Override
	public void registerStompEndpoints(StompEndpointRegistry registry) {
		registry.addEndpoint("/portfolio").setHandshakeHandler(handshakeHandler()).withSockJS();
	}

	@Bean
	public HandshakeHandler handshakeHandler() {
		return new DefaultHandshakeHandler(requestUpgradeStrategy());
	}

	@Bean
	public JettyRequestUpgradeStrategy requestUpgradeStrategy() {
		WebSocketServerFactory factory = ...
		return new JettyRequestUpgradeStrategy(factory);
	}

[spring-projects-issues]

Solace S commented

But that's the problem. The method signature for WebSocketServerFactory is now: WebSocketServerFactory​(javax.servlet.ServletContext context, WebSocketPolicy policy)

I am not sure how to get the current Jetty ServletContext for the WebSocketServerFactory

[spring-projects-issues]

Jared Jacobs commented

Not only that, but integration testing shouldn’t require copying stuff already in WebSocketConfig with slight modifications. One of the main points of integration testing is to make sure that everything is configured correctly. Substituting a fake a certain spot in the dependency graph should be simple, atomic operation (e.g. a single, trivial bean declaration). It shouldn’t require building and grafting in a sizable new dependency chain or branch. Ideally it’d be handled automatically by the test framework.

I’d be open to suggesting API changes to improve testability if that would be helpful.

[spring-projects-issues]

Rossen Stoyanchev commented

I am not sure how to get the current Jetty ServletContext for the WebSocketServerFactory

We're talking about a test with a MockServletContext (i.e. no running Jetty server), right?

I’d be open to suggesting API changes to improve testability if that would be helpful.

Feel free to make suggestions. As far as I can see the WebSocketServerFactory requires a DecoratedObjectFactory and an Executor. The former can be detected from a ServletContext attribute while for the latter the ServletContext would have to implement the Jetty ContextHandler, so that would have to be a sub-class of MockServletContext at best. I am not quite sure how or where those customizations would be applied from.

[spring-projects-issues]

Joakim Erdfelt commented

I'm taking a look at JettyRequestUpgradeStrategy as part of this and spring-projects/spring-boot#7487

I'm working on an overhaul of this entire layer to bring it back to JSR356 by using an existing ServletContainerInitializer that has been present since Jetty 9.0.0

In the meantime, where should I file bad assumptions that I come across. Here in Jira or on github?

Example: JettyWebSocketHandlerAdapter has an invalid close assumption. (onWebSocketClose is not after connection closed, its the same as JSR356 onClose, where the application can choose to finish up behaviors and close with a different code if they want)

   @OnWebSocketClose
public void onWebSocketClose(int statusCode, String reason) {
     CloseStatus closeStatus = new CloseStatus(statusCode, reason);
     try {
          this.webSocketHandler.afterConnectionClosed(this.wsSession, closeStatus);
     }
     catch (Throwable ex) {
          if (logger.isWarnEnabled()) {
               logger.warn("Unhandled exception after connection closed for " + this, ex);
          }
     }
}

[spring-projects-issues]

Joakim Erdfelt commented

I'm tabling the moving of the implementation from what you have to using the SCI, for now.
But the existing SCI should really be used to init the JSR356 container, and then you can use the normal ServletContext.getAttribute("javax.websocket.server.ServerContainer") behaviors to obtain the standard JSR356 container to add endpoints. Normal upgrade then proceeds via the standard HTTP requests into the server.

Meanwhile, to address this, I'm working on a branch off of jetty-9.4.x to see if we can relax the ServletContextHandler and ContextHandler requirements a great deal.

[spring-projects-issues]

Joakim Erdfelt commented

Just a heads up: Jetty 10.x has a big refactor of the websocket layer to support reactive websockets and http/2 websockets. We should align our efforts to yours before the release.
It would be good for me to understand the history of the decisions on the spring side.

[spring-projects-issues]

Joakim Erdfelt commented

A proposed fix (at the Jetty side) can be found in discussions at spring-projects/spring-boot#7487

[spring-projects-issues]

Rossen Stoyanchev commented

I'm resolving as "Invalid" because the JettyRequestUpgradeStategy is not intended to be able to start without a running Jetty server. You can partition the configuration so that WebSocket-related config can be excluded through imports or profiles.

[spring-projects-issues]

Rossen Stoyanchev commented

Actually it looks like this might have been just resolved on the Jetty side with Jetty#2376 following a discussion under Boot#7487.

[spring-projects-issues]

Rossen Stoyanchev commented

Joakim Erdfelt, I had this ticket open in my browser and did not see your comments until now. Thanks for the fix related to this ticket and test environments.

where should I file bad assumptions that I come across. Here in Jira or on github?

This here is the issue tracker for the Spring Framework and it's the right place anything related to JettyRequestUpgradeStrategy and WebSocket overall.

onWebSocketClose is not after connection closed, its the same as JSR356 onClose, where the application can choose to finish up behaviors and close with a different code if they want

Perhaps the name could have been better but the Javadoc does say:

* Invoked after the WebSocket connection has been closed by either side, or after a
* transport error has occurred. Although the session may technically still be open,
* depending on the underlying implementation, sending messages at this point is
* discouraged and most likely will not succeed.

We could improve that with an extra clarification that the status may still be changed, e.g. on Jetty and JSR-356.

the existing SCI should really be used to init the JSR356 container, and then you can use the normal ServletContext.getAttribute("javax.websocket.server.ServerContainer") behaviors to obtain the standard JSR356 container to add endpoints.

I addressed why this is not good enough for us under the boot ticket. Perhaps respond / discuss there? In general it would be great to coordinate this and the larger efforts related to your WebSocket API overhaul and HTTP/2. Note that we have this #21016, #19658, and #21014 planned.