[JENKINS-68933] Update to Jetty 10, with WebSocket #6785
Conversation
…-JENKINS-68933
…46.v20220331 to 10.0.11" (jenkinsci#6781)" This reverts commit 4ba9c08.
websocket/jetty10/src/main/java/jenkins/websocket/Jetty10Provider.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
I was exploring this logic earlier today and came to a similar conclusion about isUpgradeRequest. I had been thinking about inlining it, but I guess retaining only the first check as you did here is probably good enough.
This looks good pending a few remaining tasks:
- Merge in the static analysis fixes I just made to [JENKINS-68933] Better WebSocket testing, removal of reflection #6780 because otherwise this won't build in CI
- Probably need to add Enforcer excludes for the new modules as well to get Enforcer to pass on the
war/module - Add the call to
JettyWebSocketServletContainerInitializer.configurein Winstone per [JENKINS-68933] Update to Jetty 10, with WebSocket #6785 (comment) - Once the CI build is passing, I would personally like to run another build with the Jetty 10 based
JenkinsRulefrom Jetty 10 basedJenkinsRulejenkins-test-harness#453 and PCT/BOM testing with the same. It should not turn up anything new, but I would rather be safe than sorry. With passing core and BOM/PCT suites with a Jetty 10 basedJenkinsRuleI would feel very confident about this change.
Happy to help with these tasks, so let me know how we can best drive this forward together.
I think the first three are now covered, so in principle this PR should now be ready as far as WebSocket support is concerned as soon as the Winstone change is released. If you want to set up a separate draft PR for testing against jenkinsci/jenkins-test-harness#453 that would certainly increase our confidence. (There are apparently other regressions like #6694 (comment) and #6694 (comment) which could be addressed as well. I was not planning to look into those, so if you want to look into them, it may make more sense to file a fresh PR subsuming this one which is truly production-ready. My interest was in restoring WS functionality, especially since I was responsible for the reflection tech debt that made it tricky to understand where Jetty 10 was not working; I guess it did not even occur to me at the time that Jetty would change their APIs incompatibly.) |
|
Interactively verified |
Otherwise closing connections sometimes produces stack traces ``` WARNING j.websocket.WebSocketSession#error: unhandled WebSocket service error java.nio.channels.ClosedChannelException at org.eclipse.jetty.websocket.core.internal.WebSocketSessionState.onEof(WebSocketSessionState.java:169) at org.eclipse.jetty.websocket.core.internal.WebSocketCoreSession.onEof(WebSocketCoreSession.java:253) at org.eclipse.jetty.websocket.core.internal.WebSocketConnection.fillAndParse(WebSocketConnection.java:482) at org.eclipse.jetty.websocket.core.internal.WebSocketConnection.onFillable(WebSocketConnection.java:340) at org.eclipse.jetty.io.AbstractConnection$ReadCallback.succeeded(AbstractConnection.java:319) at … ``` or ``` WARNING j.agents.WebSocketAgents$Session#error java.nio.channels.ClosedChannelException at org.eclipse.jetty.websocket.core.internal.WebSocketSessionState.onEof(WebSocketSessionState.java:169) at org.eclipse.jetty.websocket.core.internal.WebSocketCoreSession.onEof(WebSocketCoreSession.java:253) at … ``` though I am not sure if that is actually new behavior.
Otherwise closing connections sometimes produces stack traces ``` WARNING j.websocket.WebSocketSession#error: unhandled WebSocket service error java.nio.channels.ClosedChannelException at org.eclipse.jetty.websocket.core.internal.WebSocketSessionState.onEof(WebSocketSessionState.java:169) at org.eclipse.jetty.websocket.core.internal.WebSocketCoreSession.onEof(WebSocketCoreSession.java:253) at org.eclipse.jetty.websocket.core.internal.WebSocketConnection.fillAndParse(WebSocketConnection.java:482) at org.eclipse.jetty.websocket.core.internal.WebSocketConnection.onFillable(WebSocketConnection.java:340) at org.eclipse.jetty.io.AbstractConnection$ReadCallback.succeeded(AbstractConnection.java:319) at … ``` or ``` WARNING j.agents.WebSocketAgents$Session#error java.nio.channels.ClosedChannelException at org.eclipse.jetty.websocket.core.internal.WebSocketSessionState.onEof(WebSocketSessionState.java:169) at org.eclipse.jetty.websocket.core.internal.WebSocketCoreSession.onEof(WebSocketCoreSession.java:253) at … ``` though I am not sure if that is actually new behavior.
|
FYI I was topologically merging in the upstream branch, which Git tracks better than cherry-picks. (Until you squash-merge the upstream PR. One reason I prefer to use true merges.) |
There was a problem hiding this comment.
I was not planning to look into those, so if you want to look into them, it may make more sense to file a fresh PR subsuming this one which is truly production-ready.
Sure, let's get a green CI build for #6785 and let's get #6780 merged and then I can file a new PR for the final reintegration. Many thanks for the collaboration thus far. I plan to do two sets of core PRs (one with regular test harness and one with Jetty 10 test harness) and two corresponding BOM/PCT runs.
…-JENKINS-68933
github-actions
bot
added
the
unresolved-merge-conflict
|
Please take a moment and address the merge conflicts of your pull request. Thanks! |
|
Continuing in #6801 |
…-JENKINS-68933-jetty-10
github-actions
bot
removed
the
unresolved-merge-conflict
Subsumes #6780; reverts #6781, reapplying #6694.
See JENKINS-68933.
Development loop:
Proposed changelog entries
Proposed upgrade guidelines
N/A
Maintainer checklist
Before the changes are marked as
ready-for-merge:Proposed changelog entriesare accurate, human-readable, and in the imperative moodupgrade-guide-neededlabel is set and there is aProposed upgrade guidelinessection in the PR title. (example)lts-candidateto be considered (see query).