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

Failure to instantiate a websocket endpoint is now properly reported #9461

Merged

Conversation

lorban
Copy link
Contributor

@lorban lorban commented Mar 2, 2023

Closes #9412

@lorban lorban added Bug For general bugs on Jetty side Jetty 12 labels Mar 2, 2023
@lorban lorban self-assigned this Mar 2, 2023
@lorban lorban requested a review from sbordet March 2, 2023 16:50
@lorban lorban force-pushed the fix/jetty-12-9412-fix-upgrade-to-throwing-endpoint branch from 0fa3ae5 to 52d4d73 Compare March 2, 2023 17:26
@joakime
Copy link
Contributor

joakime commented Mar 2, 2023

The Jakarta WebSocket Spec has a jakarta.websocket.DeploymentException that we have to be careful to implement properly.

@lorban
Copy link
Contributor Author

lorban commented Mar 3, 2023

@joakime The EE10 DeploymentException javadoc says Checked exception indicating some kind of failure either to publish an endpoint on its server, or a failure to connect a client to its server. EE 9 and 8 versions of this class have identical javadoc.

If my understanding of what you're saying is correct:

  • On the server, the CustomUpgradeServlet used in ProgrammaticWebSocketUpgradeTest should get a DeploymentException in response to its ServerContainer.upgradeHttpToWebSocket() call.
  • On the client, WebSocketContainer.connectToServer() should throw DeploymentException.

I can make the necessary changes, but it looks like Jetty 11 also does not respect this contract.

@lorban lorban force-pushed the fix/jetty-12-9412-fix-upgrade-to-throwing-endpoint branch from 52d4d73 to 62382de Compare March 6, 2023 15:17
@lorban
Copy link
Contributor Author

lorban commented Mar 6, 2023

@joakime I think there is indeed an issue with DeploymentException so I've opened #9466 to track it, and I'm going to happily ignore it in the context of this PR as it's out of this scope IMHO.

@lorban lorban merged commit 71c2ef2 into jetty-12.0.x Mar 8, 2023
@lorban lorban deleted the fix/jetty-12-9412-fix-upgrade-to-throwing-endpoint branch March 8, 2023 14:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug For general bugs on Jetty side
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Jetty 12: WebSocket hangs when ServerEndpointConfig.Configurator.getEndpointInstance() throws
3 participants