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

Issue #5229 - WebSocket documentation. #6623

Merged
merged 4 commits into from Sep 15, 2021

Conversation

sbordet
Copy link
Contributor

@sbordet sbordet commented Aug 16, 2021

WebSocket server documentation.

Signed-off-by: Simone Bordet simone.bordet@gmail.com

WebSocket server documentation.

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
@sbordet sbordet added this to In progress in Jetty 10.0.7/11.0.7 FROZEN via automation Aug 16, 2021
@sbordet sbordet linked an issue Aug 16, 2021 that may be closed by this pull request
@gregw
Copy link
Contributor

gregw commented Aug 16, 2021

just working out why PRs are not building atm...

Copy link
Contributor

@lachlan-roberts lachlan-roberts left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You might also want to write about the JettyWebSocketCreator class which can be registered to pathspec and can be used to dynamically create the endpoint using information from the request and response.

Also maybe some information about the JettyWebSocketServerContainer.upgrade() feature which allows an upgrade directly without registering a mapping.

Jetty 10.0.7/11.0.7 FROZEN automation moved this from In progress to Review in progress Aug 17, 2021
Updates after review.

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
Copy link
Contributor

@gregw gregw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I only looked at the first bit and think it needs a bit of refactoring.
I'll look deeper once @lachlan-roberts has commented.

[[pg-server-websocket-configure-filter]]
==== Advanced `WebSocketUpgradeFilter` Configuration

The WebSocket ``ServletContainerInitializer``s, namely `JavaxWebSocketServletContainerInitializer` and `JettyWebSocketServletContainerInitializer`, install the `WebSocketUpgradeFilter` to handle HTTP requests that upgrade to WebSocket.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not correct.

The jetty SCI doesn't add the filter, rather it is added in the addMapping method on the container. Only the javax SCI always adds the filter.

Moreover, the SCIs do a lot more than just install the filter: pools for inflaters, deflaters, buffers; executors; object factory; mappings; contains; lifecycle listeners etc.

The SCIs initialize a lot of machinery and much of it is extensible/configurable to the users so probably should be of note in the "Advanced" documentation. So I think we should open with a full enumeration of the mechanism that need to be initialized and how/where they are.


The WebSocket ``ServletContainerInitializer``s, namely `JavaxWebSocketServletContainerInitializer` and `JettyWebSocketServletContainerInitializer`, install the `WebSocketUpgradeFilter` to handle HTTP requests that upgrade to WebSocket.

Typically, the `WebSocketUpgradeFilter` is not present in the `web.xml` configuration, and therefore the WebSocket ``ServletContainerInitializer``s create a new `WebSocketUpgradeFilter` and install it _before_ any other Filter declared in `web.xml`, under the default name of `"org.eclipse.jetty.websocket.servlet.WebSocketUpgradeFilter"` and with path mapping `/*`.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Once we have described all the bits that are initialized, then we need subsections saying how each bit of machinery can be customized. The filter being only one of these subsections. We also need to say how pools etc. can be customized/configured/replaced.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gregw the machinery can be customized, but without APIs.
It's all relying on some magic "look at this attribute in the context" and the attribute names are based on classes that are not really meant to be exposed in this way, say for example o.e.j.w.core.WebSocketComponents.
I would not document this now, and if people really struggle, then add the docs later.

Updates after review.

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
@sbordet sbordet requested a review from gregw September 6, 2021 13:51
@sbordet
Copy link
Contributor Author

sbordet commented Sep 6, 2021

@lachlan-roberts @gregw I have updated the documentation, see also my comments about your comments.
Please re-review.

Updates after review.

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
Jetty 10.0.7/11.0.7 FROZEN automation moved this from Review in progress to Reviewer approved Sep 8, 2021
@sbordet sbordet merged commit 4793092 into jetty-10.0.x Sep 15, 2021
Jetty 10.0.7/11.0.7 FROZEN automation moved this from Reviewer approved to Done Sep 15, 2021
@sbordet sbordet deleted the jetty-10.0.x-websocket-server-documentation branch September 15, 2021 17:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

WebSocket documentation in Jetty 10
3 participants