OF-2600: Upgrade Jetty 9.4.43.v20210629 to 10.0.16 #2188
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@GregDThomas didn't you do the last Jetty upgrade? Do you remember any hidden dragons? |
|
My recollection is that it was fairly straightforward - I think one API changed so had to tweak the loading of plugins in some way, but nothing major. But that was a minor version update, looks like this major version update is more substantive :( |
viv
changed the title
|
With @viv's latest tests:
|
|
Just remembered that the file org.eclipse.jetty.util.WebAppLoaderFix can probably be removed now - IIRC it only worked on older versions of JRE/Jetty, so it's probably long redundant now. Originally created to fix https://igniterealtime.atlassian.net/browse/OF-1522 in #1228 |
Remaining Issues: - JSP compilation failure caused by: apache/tomcat@224fb6c#diff-ef02fd9c3f90ca4838d3cdaa051489556eca75d537fc396022e0ed456c24d895R329 - `WebSocketClientConnectionHandler.onConnect` assumes that `session.getRemoteAddress()` supplies an `InetSocketAddress` which might not always be true. What's the best approach in our context? Fixed in this commit: - Websocket server Maven artifact renamed from `websocket-server` to `websocket-jetty-server` - `WebSocketServlet` renamed to `JettyWebSocketServlet` - `WebSocketServletFactory` renamed to `JettyWebSocketServletFactory` See: https://www.eclipse.org/jetty/documentation/jetty-10/programming-guide/index.html#pg-migration-94-to-10 - `GzipHandler` is no longer part of `ServletContextHandler` jetty/jetty.project#4765 - Renamed `setWebInfClassesDirs` to `setWebInfClassesResources` jetty/jetty.project#4506 - Split `SslContextFactory` into Client and Server jetty/jetty.project#3464
In this commit, the taglib definitions are consolidated in one place, where Jetty 10.0.5 expects to find them. A nuisance is that with this layout, the IntelliJ IDE is no longer able to find the taglibs. This can be annoying during development, but does not introduce runtime issues.
With Jetty 10.0.15, the admin console shows HTTP 503 error responses, and ClassLoading exceptions are logged. It appears that in Jetty 10, unless a ClassLoader has been explicitly set on a ContextHandler, Jetty will use its own implementation. I assume (but did not check) that in Jetty 9, this was different (possibly, it used the ClassLoader of the invoking thread?). The ClassLoader that is provided by Jetty cannot find most of the libraries in the way they are provided by Openfire. Openfire uses its own ClassLoaders. When Jetty's embedded server is instantiated to use the ClassLoader of the invoking thread as done in this commit, then the problem disappears. Questions remain: - Why does Jetty use their own ClassLoader (unless explicitly overridden)? This suggests that this is somehow important (which this change has undone - which might not be safe). - Is there a better way of doing this? Maybe use a different ClassLoader instance, or somehow configure the Jetty classloader to find the classes it needs? - Will this work with plugins? - Do we need to do this elsewhere (eg: everywhere a Jetty Server is instantiated?)
Fix to initialise web socket components. With Jetty 10 we now need to configure the WebSocket ServletContextHandler to call the JettyWebSocketServletContainerInitializer during the ServletContext initialization phase.
Correct configuration of websocket compression. Compression is provided out of the box by Jetty's `permessage-deflate` extension. Previously Openfire was registering the `permessage-deflate` extension, I assume this was attempting to enable websocket compression. Presently, websocket compression is enabled by default in Jetty. So the correct way to control websocket compression is to disable the `permessage-deflate` extension when compression is not wanted: jetty/jetty.project#1341
Thanks to GregDThomas for spotting: IIRC it only worked on older versions of JRE/Jetty, so it's probably long redundant now. Originally created to fix https://igniterealtime.atlassian.net/browse/OF-1522 in igniterealtime#1228 (with comment "Note; we may need to revisit this come Java 9.")
viv
force-pushed
the
jetty-10-upgrade
branch
from
06ac6d5 to
d1d1d12
Compare
|
Rebased onto latest from the main branch and updated to latest Jetty 10 version. |
Fishbowler
changed the title
viv
changed the title
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Remaining Issues:
JSP compilation failure caused by: apache/tomcat@224fb6c#diff-ef02fd9c3f90ca4838d3cdaa051489556eca75d537fc396022e0ed456c24d895R329
WebSocketClientConnectionHandler.onConnectassumes thatsession.getRemoteAddress()supplies anInetSocketAddresswhich might not always be true. What's the best approach in our context?Fixed in this commit:
websocket-servertowebsocket-jetty-serverWebSocketServletrenamed toJettyWebSocketServletWebSocketServletFactoryrenamed toJettyWebSocketServletFactoryGzipHandleris no longer part ofServletContextHandlerReview GzipHandler inside ServletContextHandler jetty/jetty.project#4765setWebInfClassesDirstosetWebInfClassesResourcesServlet 4 metadata-complete=true should skip introspection of annotations jetty/jetty.project#4506SslContextFactoryinto Client and Server Split SslContextFactory into Client and Server jetty/jetty.project#3464See: https://www.eclipse.org/jetty/documentation/jetty-10/programming-guide/index.html#pg-migration-94-to-10