Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jetty 12 - Public version of JakartaWebSocketServerContainer #9182

Closed
Sineaggi opened this issue Jan 17, 2023 · 7 comments
Closed

Jetty 12 - Public version of JakartaWebSocketServerContainer #9182

Sineaggi opened this issue Jan 17, 2023 · 7 comments

Comments

@Sineaggi
Copy link

Enhancement Description
Spring uses the internal JakartaWebSocketServerContainer, which requires the use of --add-opens org.eclipse.jetty.websocket.jakarta.server/org.eclipse.jetty.websocket.jakarta.server.internal=ALL-UNNAMED when running spring boot under the module path. The spring boot issue is spring-projects/spring-boot#33833

Currently spring boot uses the class for https://github.com/spring-projects/spring-boot/blob/fe7b13ec46f9960bebfddad984816e353f9c9349/spring-boot-project/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/websocket/servlet/JettyWebSocketServletWebServerCustomizer.java#L23 to call getContainer and ensureContainer.

@lachlan-roberts
Copy link
Contributor

@Sineaggi This code should be using JakartaWebSocketServletContainerInitializer.configure(ServletContextHandler context, Configurator configurator) or JakartaWebSocketServletContainerInitializer.initialize(ServletContextHandler context) methods for setting up Jakarta WebSockets. This will set up the same internal components but does need direct access to any classes in the internal packages.

Do you know why spring requires use of the JakartaWebSocketServerContainer class directly?

@janbartel
Copy link
Contributor

@Sineaggi nudge

@Sineaggi
Copy link
Author

Sineaggi commented Feb 9, 2023

This class is used by spring-boot spring-projects/spring-boot#33833 to configure their internal websocket customizer https://github.com/spring-projects/spring-boot/blob/fe7b13ec46f9960bebfddad984816e353f9c9349/spring-boot-project/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/websocket/servlet/JettyWebSocketServletWebServerCustomizer.java#L23.

Similar to the call JettyWebSocketServerContainer.getContainer(context.getServletContext()), spring boot makes a call to JakartaWebSocketServerContainer.getContainer(context.getServletContext()).

@Sineaggi
Copy link
Author

Sineaggi commented Feb 9, 2023

Perhaps @wilkinsona can add more context

@wilkinsona
Copy link
Contributor

JakartaWebSocketServletContainerInitializer.configure(ServletContextHandler context, Configurator configurator)

Spring Boot does not support ServletContainerInitializers so this would have no effect in a Boot app.

JakartaWebSocketServletContainerInitializer.initialize(ServletContextHandler context)

Apologies if I am looking in the wrong place but this appears to be private. If it were public, I think it would meet our needs although this is based purely on looking at the code.

@lachlan-roberts
Copy link
Contributor

@wilkinsona the JakartaWebSocketServletContainerInitializer.configure(ServletContextHandler context, Configurator configurator) method programatically registers the SCI with Jetty so I think it would still work unless you have overridden the ServletContextHandler to modify the behaviour.

But yes I thought JakartaWebSocketServletContainerInitializer.initialize was already public. I will put up a PR to make this public, and also review if we should make JakartaWebSocketServerContainer public as well.

lachlan-roberts added a commit that referenced this issue Feb 17, 2023
…erContainer public

Signed-off-by: Lachlan Roberts <[email protected]>
lachlan-roberts added a commit that referenced this issue Feb 17, 2023
…ketServetContainer

Issue #9182 - make JakartaWSSCI.initialize() and JakartaWebSocketServerContainer public
@lachlan-roberts
Copy link
Contributor

I have merged the changes from #9395 into Jetty 12.

Now both JakartaWebSocketServletContainerInitializer.initialize and the JakartaWebSocketServerContainer are public. But you should prefer to use the initialize() method instead of doing the ensure container yourself.

gregpoulos pushed a commit to gregpoulos/jetty.project that referenced this issue Feb 18, 2023
… into jetty-12.0.x-old-docs-remove-logging-sections

* 'jetty-12.0.x' of https://github.com/eclipse/jetty.project:
  Issue jetty#9182 - make JakartaWSSCI.initialize() and JakartaWebSocketServerContainer public
  ensure the WebSocketConnection is set on the WebSocketCoreSession
  remove osgi internal imports for websocket-core
  rename WebSocketUtil to WebSocketUtils
  make WebSocketCoreSession public & other fixes
  Javadocs for Response and Context. (jetty#9388)
  Moved implementation methods ensure*() from the Response interface (jetty#9390)
  Reinstate ee9 jetty runner. (jetty#9383)
  Fix jetty#9387
  remove setClassLoader from CoreSession interface
  remove exporting of internal packages in ee9 & ee10 websocket
  fix remaining JPMS issues in websocket-core
  resolve JPMS issues with CoreSession and WebSocketCoreSession
  move websocket-core-common messages and util packages out of internal
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants