[JENKINS-73130] Upgrade core from Jetty 10.x to 12.x (EE 8) #9590
Conversation
Co-authored-by: Olivier Lamy <[email protected]>
| "enabled": false, | ||
| "matchPackageNames": [ | ||
| "jakarta.servlet:jakarta.servlet-api", | ||
| "jakarta.servlet.jsp.jstl:jakarta.servlet.jsp.jstl-api" |
There was a problem hiding this comment.
This exclusion remains valid, but not for the reason currently given. The exclusion remains valid for the reason given in the previous exclusion for jakarta.servlet:jakarta.servlet-api. Indeed, that reason applies to both jakarta.servlet:jakarta.servlet-api and jakarta.servlet.jsp.jstl:jakarta.servlet.jsp.jstl-api, so move these exclusions to the same section.
| <file url="file://$PROJECT_DIR$/websocket/jetty12-ee8/src/filter/resources" charset="UTF-8" /> | ||
| <file url="file://$PROJECT_DIR$/websocket/jetty12-ee8/src/main/java" charset="UTF-8" /> | ||
| <file url="file://$PROJECT_DIR$/websocket/jetty12-ee8/src/main/resources" charset="UTF-8" /> |
There was a problem hiding this comment.
Add a new Jetty 12 (EE 8) WebSocket implementation. Both the Jetty 10 and Jetty 12 (EE 8) implementations will be dropped in a forthcoming PR when we add support for Jetty 12 (EE 9).
bom/pom.xml
Outdated
| <commons-fileupload2.version>2.0.0-M2</commons-fileupload2.version> | ||
| <slf4jVersion>2.0.15</slf4jVersion> | ||
| <stapler.version>1892.v73465f3d074d</stapler.version> | ||
| <!-- TODO JENKINS-73122 https://github.com/jenkinsci/stapler/pull/537 --> |
pom.xml
Outdated
| <!-- Make sure to keep the jetty-maven-plugin version in war/pom.xml in sync with the Jetty release in Winstone: --> | ||
| <winstone.version>6.21</winstone.version> | ||
| <!-- TODO JENKINS-73126 https://github.com/jenkinsci/winstone/pull/383 --> | ||
| <winstone.version>7.0-rc921.b_dc6422f61b_6</winstone.version> |
| <groupId>${project.groupId}</groupId> | ||
| <artifactId>jenkins-test-harness</artifactId> | ||
| <version>2225.2230.v6210cb_b_827f9</version> | ||
| <version>2250.v03a_1295b_0a_30</version> |
There was a problem hiding this comment.
The latest version of Jenkins Test Harness (JTH), which requires Java 17 and is based on Jetty 12, using reflection to choose between EE 8 and EE 9.
| } | ||
|
|
||
| private static class RemoteUpdateSiteHandler extends AbstractHandler { | ||
| private static class RemoteUpdateSiteHandler extends Handler.Abstract { |
| // The client might still be sending us more of the request, but we have had enough of it already and | ||
| // have decided to stop processing it. Drain the read end of the socket so that the client can finish | ||
| // sending its request in order to read the response we are about to provide. | ||
| try (OutputStream os = OutputStream.nullOutputStream()) { | ||
| req.getInputStream().transferTo(os); | ||
| } |
There was a problem hiding this comment.
See JENKINS-73127 for an explanation of this change.
| protected void setWebAppInitParameter(String property, String value) { | ||
| Assume.assumeThat(j.jenkins.servletContext, Matchers.instanceOf(ContextHandler.Context.class)); | ||
| ((ContextHandler.Context) j.jenkins.servletContext).getContextHandler().getInitParams().put(property, value); | ||
| Assume.assumeThat(j.jenkins.servletContext, Matchers.instanceOf(WebAppContext.Context.class)); |
There was a problem hiding this comment.
Adapting to new Jetty 12 APIs as per the migration guide.
| @@ -0,0 +1,85 @@ | |||
| <?xml version="1.0" encoding="UTF-8"?> | |||
There was a problem hiding this comment.
To ease reviewing, a diff from Jetty 10:
The MIT License
-Copyright (c) 2019, CloudBees, Inc.
+Copyright (c) 2019, 2024 CloudBees, Inc.
Permission is hereby granted, free of charge, to any person obtaining a copy
of this software and associated documentation files (the "Software"), to deal
@@ -32,9 +32,9 @@ THE SOFTWARE.
<relativePath>../..</relativePath>
</parent>
- <artifactId>websocket-jetty10</artifactId>
- <name>Jetty 10 implementation for WebSocket</name>
- <description>An implementation of the WebSocket handler that works with Jetty 10.</description>
+ <artifactId>websocket-jetty12-ee8</artifactId>
+ <name>Jetty 12 (EE 8) implementation for WebSocket</name>
+ <description>An implementation of the WebSocket handler that works with Jetty 12 (EE 8).</description>
<dependencyManagement>
<dependencies>
@@ -52,7 +52,7 @@ THE SOFTWARE.
<dependency>
<groupId>org.jenkins-ci</groupId>
<artifactId>winstone</artifactId>
- <version>6.21</version>
+ <version>${winstone.version}</version>
<optional>true</optional>
</dependency>
<dependency>| @@ -0,0 +1,178 @@ | |||
| /* | |||
There was a problem hiding this comment.
To ease reviewing, a diff from Jetty 10:
diff --git a/websocket/jetty10/src/main/java/jenkins/websocket/Jetty10Provider.java b/websocket/jetty10/src/main/java/jenkins/websocket/Jetty10Provider.java
index b5243e627f..cc006b180f 100644
--- a/websocket/jetty10/src/main/java/jenkins/websocket/Jetty10Provider.java
+++ b/websocket/jetty10/src/main/java/jenkins/websocket/Jetty10Provider.java
@@ -1,7 +1,7 @@
* The MIT License
*
- * Copyright 2022 CloudBees, Inc.
+ * Copyright 2022, 2024 CloudBees, Inc.
*
* Permission is hereby granted, free of charge, to any person obtaining a copy
* of this software and associated documentation files (the "Software"), to deal
@@ -31,35 +31,35 @@ import java.util.concurrent.CompletableFuture;
import java.util.concurrent.Future;
import javax.servlet.http.HttpServletRequest;
import javax.servlet.http.HttpServletResponse;
-import org.eclipse.jetty.websocket.api.Session;
-import org.eclipse.jetty.websocket.api.WebSocketListener;
-import org.eclipse.jetty.websocket.api.WriteCallback;
-import org.eclipse.jetty.websocket.server.JettyServerUpgradeRequest;
-import org.eclipse.jetty.websocket.server.JettyServerUpgradeResponse;
-import org.eclipse.jetty.websocket.server.JettyWebSocketServerContainer;
+import org.eclipse.jetty.ee8.websocket.api.Session;
+import org.eclipse.jetty.ee8.websocket.api.WebSocketListener;
+import org.eclipse.jetty.ee8.websocket.api.WriteCallback;
+import org.eclipse.jetty.ee8.websocket.server.JettyServerUpgradeRequest;
+import org.eclipse.jetty.ee8.websocket.server.JettyServerUpgradeResponse;
+import org.eclipse.jetty.ee8.websocket.server.JettyWebSocketServerContainer;
import org.kohsuke.MetaInfServices;
import org.kohsuke.accmod.Restricted;
import org.kohsuke.accmod.restrictions.NoExternalUse;
@Restricted(NoExternalUse.class)
@MetaInfServices(Provider.class)
-public class Jetty10Provider implements Provider {
+public class Jetty12EE8Provider implements Provider {
/**
* Number of seconds a WebsocketConnection may stay idle until it expires.
* Zero to disable.
* This value must be higher than the <code>jenkins.websocket.pingInterval</code>.
- * Per <a href=https://www.eclipse.org/jetty/documentation/jetty-10/programming-guide/index.html#pg-websocket-session-ping>Jetty 10 documentation</a>
+ * Per <a href=https://eclipse.dev/jetty/documentation/jetty-12/programming-guide/index.html#pg-websocket-session-ping>Jetty 12 documentation</a>
* a ping mechanism should keep the websocket active. Therefore, the idle timeout must be higher than the ping
* interval to avoid timeout issues.
*/
private static long IDLE_TIMEOUT_SECONDS = Long.getLong("jenkins.websocket.idleTimeout", 60L);
- private static final String ATTR_LISTENER = Jetty10Provider.class.getName() + ".listener";
+ private static final String ATTR_LISTENER = Jetty12EE8Provider.class.getName() + ".listener";
private boolean initialized = false;
- public Jetty10Provider() {
+ public Jetty12EE8Provider() {
JettyWebSocketServerContainer.class.hashCode();
}
@@ -74,12 +74,12 @@ public class Jetty10Provider implements Provider {
public Handler handle(HttpServletRequest req, HttpServletResponse rsp, Listener listener) throws Exception {
init(req);
req.setAttribute(ATTR_LISTENER, listener);
- // TODO Jetty 10 has no obvious equivalent to WebSocketServerFactory.isUpgradeRequest; RFC6455Negotiation?
+ // TODO Jetty 10+ has no obvious equivalent to WebSocketServerFactory.isUpgradeRequest; RFC6455Negotiation?
if (!"websocket".equalsIgnoreCase(req.getHeader("Upgrade"))) {
rsp.sendError(HttpServletResponse.SC_BAD_REQUEST, "only WS connections accepted here");
return null;
}
- if (!JettyWebSocketServerContainer.getContainer(req.getServletContext()).upgrade(Jetty10Provider::createWebSocket, req, rsp)) {
+ if (!JettyWebSocketServerContainer.getContainer(req.getServletContext()).upgrade(Jetty12EE8Provider::createWebSocket, req, rsp)) {
rsp.sendError(HttpServletResponse.SC_BAD_REQUEST, "did not manage to upgrade");
return null;
}
@@ -175,5 +175,4 @@ public class Jetty10Provider implements Provider {
}
};
}
-
}| <config implementation="org.eclipse.jetty.maven.MavenResource"> | ||
| <resourceAsString>${basedir}/src/realm.properties</resourceAsString> | ||
| </config> |
There was a problem hiding this comment.
| Reload webapp when you hit ENTER. (See JETTY-282 for more) | ||
| --> | ||
| <reload>manual</reload> | ||
| <scan>0</scan> |
There was a problem hiding this comment.
MarkEWaite
left a comment
There was a problem hiding this comment.
Changes look good to me.
I see that the websocket/jetty10 module is retained in the pom.xml at the root of the repository. Is that needed for API compatibility so that the jenkins.websocket.Jetty10Provider class is not removed from the Jenkins API or is there something else that I'm not understanding about the Jetty 10 websocket provider being built and packaged in the Jenkins war file along with the Jetty 12 websocket provider?
Historically we have retained older WebSocket Jetty modules in core to allow for older test harnesses (with older Jetty versions) to be used with newer cores, and this PR follows past precedent. In any case this state of affairs will only last for a few weeks, as we'll be removing both the Jetty 10 and Jetty 12 EE 8 WebSocket modules when adding support for Jetty 12 EE 9 in core. I haven't made any attempt to get a Jetty 12 EE 9 core to work with an older test harness. Instead I have updated PCT to dynamically upgrade the test harness to one that supports Jetty 12 EE 8 and EE 9. |
Thanks! I think you had even said that in your earlier comments on the change. I must have missed that earlier comment when I reviewed the code. |
alecharp
left a comment
There was a problem hiding this comment.
Once we have release stapler, winstone and JTH we can update the versions in the poms here.
…html#handler-response-content-apis
|
Please take a moment and address the merge conflicts of your pull request. Thanks! |
Kevin-CB
left a comment
There was a problem hiding this comment.
Security wise, it looks good to me!
|
This PR is ready for merge once the versions are updated for the upcoming winstone and stapler releases: |
|
Thanks all! I am merging this to do one final set of tests before it is released tomorrow. |
|
I'm trying to
Is this happening for anyone else? |
I'm seeing the same thing when I run with I don't see that when I run with: |
It is happening for me as well. |
|
@basil Is this easily followed up with a fix, or should we revert the PR? Any idea? |
|
@daniel-beck Not sure offhand. Have you done anything to debug the issue? |
|
My attempts in the debugger have not shown me anything yet, but I'll continue after lunch to see if I can discover something |
Context
See JENKINS-73130 and its linked epic.
Problem
January 1, 2024 marked the end of community support for the Jetty 10 and 11 releases. Jenkins currently delivers Jetty 10.
Solution
Upgrade to Jetty 12 (EE 8).
This release does not have any significant compatibility implications for plugins—the
javaxServlet API packages remain in use. A small number of plugins rely on Jetty implementation details in their test suites; these test suites must be adapted to the new Jetty 12 APIs, but the production code does not. The main API differences between Jetty 10 and Jetty 12 (EE 8) are that the Jetty codebase has been turned into a modular architecture that can support multiple Servlet API versions. Previous versions of Jetty were tied to a particular version of the Servlet API.Implementation
In general, replace dependencies with EE 8 versions in
pom.xmland update all relevant imports. More notes are left in the self-review below.Testing done
Full PCT/ATH
Future work
A forthcoming PR will migrate from EE 8 to EE 9.
Proposed changelog entries
Upgrade Jetty from 10.0.22 to 12.0.12.
Proposed upgrade guidelines
N/A
Desired reviewers
@mention
Before the changes are marked as
ready-for-merge: