Skip to content

Conversation

@basil
Copy link
Member

@basil basil commented Mar 24, 2025

Context

See JENKINS-75571. Part of the following series of PRs:

Description

Upgrade Jenkins core from EE 9 to EE 10 by bumping the version numbers of the relevant libraries to match those used by Jetty 12 (EE 10).

The main change in this PR is adapting the WebSocket implementation to recent Jetty changes. See the Jetty migration guide for details. See also the Jetty 12 Programming Guide documentation about WebSocket clients.

This PR is ready for review and ready for merge from a Jenkins perspective, but it is not yet ready for merge from a CloudBees CI perspective (hence the "on hold" label).

Testing done

Proposed changelog entries

  • Upgrade from Jakarta Enterprise Edition (EE) 9 to 10.

Proposed changelog category

/label

Proposed upgrade guidelines

N/A

Submitter checklist

  • The Jira issue, if it exists, is well-described.
  • The changelog entries and upgrade guidelines are appropriate for the audience affected by the change (users or developers, depending on the change) and are in the imperative mood (see examples). Fill in the Proposed upgrade guidelines section only if there are breaking changes or changes that may require extra steps from users during upgrade.
  • There is automated testing or an explanation as to why this change has no tests.
  • New public classes, fields, and methods are annotated with @Restricted or have @since TODO Javadocs, as appropriate.
  • New deprecations are annotated with @Deprecated(since = "TODO") or @Deprecated(forRemoval = true, since = "TODO"), if applicable.
  • New or substantially changed JavaScript is not defined inline and does not call eval to ease future introduction of Content Security Policy (CSP) directives (see documentation).
  • For dependency updates, there are links to external changelogs and, if possible, full differentials.
  • For new APIs and extension points, there is a link to at least one consumer.

Desired reviewers

@mention

Before the changes are marked as ready-for-merge:

Maintainer checklist

  • There are at least two (2) approvals for the pull request and no outstanding requests for change.
  • Conversations in the pull request are over, or it is explicit that a reviewer is not blocking the change.
  • Changelog entries in the pull request title and/or Proposed changelog entries are accurate, human-readable, and in the imperative mood.
  • Proper changelog labels are set so that the changelog can be generated automatically.
  • If the change needs additional upgrade steps from users, the upgrade-guide-needed label is set and there is a Proposed upgrade guidelines section in the pull request title (see example).
  • If it would make sense to backport the change to LTS, a Jira issue must exist, be a Bug or Improvement, and be labeled as lts-candidate to be considered (see query).

@basil basil added work-in-progress The PR is under active development, not ready to the final review rfe For changelog: Minor enhancement. use `major-rfe` for changes to be highlighted labels Mar 24, 2025
basil added a commit to basil/bom that referenced this pull request Mar 24, 2025
basil added a commit to basil/bom that referenced this pull request Mar 24, 2025
@basil basil force-pushed the ee10 branch 6 times, most recently from ba57f50 to 6dde1f3 Compare March 25, 2025 21:18
basil added a commit to basil/bom that referenced this pull request Mar 26, 2025
basil added a commit to basil/bom that referenced this pull request Mar 26, 2025
basil added a commit to basil/bom that referenced this pull request Mar 26, 2025
basil added a commit to basil/bom that referenced this pull request Mar 26, 2025
basil added a commit to basil/bom that referenced this pull request Mar 27, 2025
basil added a commit to basil/bom that referenced this pull request Mar 27, 2025
basil added a commit to basil/bom that referenced this pull request Mar 27, 2025
@basil basil force-pushed the ee10 branch 3 times, most recently from 166b63a to 998c3ae Compare March 28, 2025 15:35
basil added a commit to basil/bom that referenced this pull request Mar 28, 2025
basil added a commit to basil/bom that referenced this pull request Mar 31, 2025
@github-actions
Copy link
Contributor

github-actions bot commented Apr 2, 2025

Please take a moment and address the merge conflicts of your pull request. Thanks!

@github-actions github-actions bot added the unresolved-merge-conflict There is a merge conflict with the target branch. label Apr 2, 2025
@basil basil changed the title EE 9 to EE 10 [JENKINS-75571] Upgrade core from EE 9 to EE 10 Apr 16, 2025
@basil basil added on-hold This pull request depends on another event/release, and it cannot be merged right now and removed work-in-progress The PR is under active development, not ready to the final review labels Apr 16, 2025
@basil basil added needs-ath-build Needs to run through the full acceptance-test-harness suite pct-successful This PR has successfully passed the full plugin-compatibility-test suite labels Apr 16, 2025
<groovy.version>2.4.21</groovy.version>
<jelly.version>1.1-jenkins-20250108</jelly.version>
<stapler.version>1967.vfcb_700b_75e9e</stapler.version>
<!-- TODO https://github.com/jenkinsci/jelly/pull/140 -->
Copy link
Member Author

Choose a reason for hiding this comment

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

<stapler.version>1967.vfcb_700b_75e9e</stapler.version>
<!-- TODO https://github.com/jenkinsci/jelly/pull/140 -->
<jelly.version>1.1-jenkins-20250109-rc236.4ea_1eb_d2fee9</jelly.version>
<!-- TODO https://github.com/jenkinsci/stapler/pull/617 -->
Copy link
Member Author

Choose a reason for hiding this comment

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

<!-- Make sure to keep the jetty-ee9-maven-plugin version in war/pom.xml in sync with the Jetty release in Winstone: -->
<winstone.version>8.7</winstone.version>
<!-- Make sure to keep the jetty-ee10-maven-plugin version in war/pom.xml in sync with the Jetty release in Winstone: -->
<!-- TODO https://github.com/jenkinsci/winstone/pull/422 -->
Copy link
Member Author

Choose a reason for hiding this comment

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

Comment on lines 209 to 217
if (data.hasArray()) {
connection.handle(new DataInputStream(new ByteArrayInputStream(data.array(), data.arrayOffset() + data.position(), data.remaining())));
receivedBytes += data.remaining();
} else {
byte[] payload = new byte[data.remaining()];
data.get(payload);
connection.handle(new DataInputStream(new ByteArrayInputStream(payload)));
receivedBytes += payload.length;
}

This comment was marked as outdated.

LOGGER.finest(() -> "reading block of length " + data.remaining() + " from " + agent);
try {
transport.receive(ByteBuffer.wrap(payload, offset, len));
transport.receive(data);

This comment was marked as outdated.


@Override
public void onWebSocketBinary(byte[] payload, int offset, int length) {
public void onWebSocketBinary(ByteBuffer data) {

This comment was marked as outdated.

Comment on lines +106 to +107
Assume.assumeThat(j.jenkins.getServletContext(), Matchers.instanceOf(WebAppContext.ServletApiContext.class));
((WebAppContext.ServletApiContext) j.jenkins.getServletContext()).getContext().getServletContextHandler().getInitParams().put(property, value);
Copy link
Member Author

Choose a reason for hiding this comment

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

Fragile test, but I see no simple way to make it less fragile.

@@ -0,0 +1,85 @@
<?xml version="1.0" encoding="UTF-8"?>
Copy link
Member Author

Choose a reason for hiding this comment

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

Diff from EE 9, to aid reviewers:

--- websocket/jetty12-ee9/pom.xml	2025-04-16 13:17:09.662772797 -0700
+++ websocket/jetty12-ee10/pom.xml	2025-04-16 13:17:09.662772797 -0700
@@ -32,9 +32,9 @@
     <relativePath>../..</relativePath>
   </parent>
 
-  <artifactId>websocket-jetty12-ee9</artifactId>
-  <name>Jetty 12 (EE 9) implementation for WebSocket</name>
-  <description>An implementation of the WebSocket handler that works with Jetty 12 (EE 9).</description>
+  <artifactId>websocket-jetty12-ee10</artifactId>
+  <name>Jetty 12 (EE 10) implementation for WebSocket</name>
+  <description>An implementation of the WebSocket handler that works with Jetty 12 (EE 10).</description>
 
   <dependencyManagement>
     <dependencies>
@@ -52,7 +52,7 @@
     <dependency>
       <groupId>org.jenkins-ci</groupId>
       <artifactId>winstone</artifactId>
-      <version>8.7</version>
+      <version>${winstone.version}</version>
       <optional>true</optional>
     </dependency>
     <dependency>

@@ -0,0 +1,172 @@
/*
Copy link
Member Author

@basil basil Apr 16, 2025

Choose a reason for hiding this comment

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

Diff from EE 9, to aid reviewers:

--- websocket/jetty12-ee9/src/main/java/jenkins/websocket/Jetty12EE9Provider.java	2025-04-24 10:42:11.394388977 -0700
+++ websocket/jetty12-ee10/src/main/java/jenkins/websocket/Jetty12EE10Provider.java	2025-04-24 11:05:35.878588667 -0700
@@ -26,40 +26,37 @@
 
 import jakarta.servlet.http.HttpServletRequest;
 import jakarta.servlet.http.HttpServletResponse;
-import java.io.IOException;
 import java.nio.ByteBuffer;
 import java.time.Duration;
-import java.util.concurrent.CompletableFuture;
+import java.util.Objects;
 import java.util.concurrent.Future;
-import org.eclipse.jetty.ee9.websocket.api.Session;
-import org.eclipse.jetty.ee9.websocket.api.WebSocketListener;
-import org.eclipse.jetty.ee9.websocket.api.WriteCallback;
-import org.eclipse.jetty.ee9.websocket.server.JettyServerUpgradeRequest;
-import org.eclipse.jetty.ee9.websocket.server.JettyServerUpgradeResponse;
-import org.eclipse.jetty.ee9.websocket.server.JettyWebSocketServerContainer;
+import org.eclipse.jetty.ee10.websocket.server.JettyServerUpgradeRequest;
+import org.eclipse.jetty.ee10.websocket.server.JettyServerUpgradeResponse;
+import org.eclipse.jetty.ee10.websocket.server.JettyWebSocketServerContainer;
+import org.eclipse.jetty.websocket.api.Callback;
+import org.eclipse.jetty.websocket.api.Session;
 import org.kohsuke.MetaInfServices;
 import org.kohsuke.accmod.Restricted;
 import org.kohsuke.accmod.restrictions.NoExternalUse;
 
 @Restricted(NoExternalUse.class)
 @MetaInfServices(Provider.class)
-public class Jetty12EE9Provider implements Provider {
+public class Jetty12EE10Provider 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://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.
+     * 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://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 = Jetty12EE9Provider.class.getName() + ".listener";
+    private static final String ATTR_LISTENER = Jetty12EE10Provider.class.getName() + ".listener";
 
     private boolean initialized = false;
 
-    public Jetty12EE9Provider() {
+    public Jetty12EE10Provider() {
         JettyWebSocketServerContainer.class.hashCode();
     }
 
@@ -79,41 +76,41 @@
             rsp.sendError(HttpServletResponse.SC_BAD_REQUEST, "only WS connections accepted here");
             return null;
         }
-        if (!JettyWebSocketServerContainer.getContainer(req.getServletContext()).upgrade(Jetty12EE9Provider::createWebSocket, req, rsp)) {
+        if (!JettyWebSocketServerContainer.getContainer(req.getServletContext()).upgrade(Jetty12EE10Provider::createWebSocket, req, rsp)) {
             rsp.sendError(HttpServletResponse.SC_BAD_REQUEST, "did not manage to upgrade");
             return null;
         }
         return new Handler() {
             @Override
-            public Future<Void> sendBinary(ByteBuffer data) throws IOException {
-                CompletableFuture<Void> f = new CompletableFuture<>();
-                session().getRemote().sendBytes(data, new WriteCallbackImpl(f));
+            public Future<Void> sendBinary(ByteBuffer data) {
+                Callback.Completable f = new Callback.Completable();
+                session().sendBinary(data, f);
                 return f;
             }
 
             @Override
-            public Future<Void> sendBinary(ByteBuffer partialByte, boolean isLast) throws IOException {
-                CompletableFuture<Void> f = new CompletableFuture<>();
-                session().getRemote().sendPartialBytes(partialByte, isLast, new WriteCallbackImpl(f));
+            public Future<Void> sendBinary(ByteBuffer partialByte, boolean isLast) {
+                Callback.Completable f = new Callback.Completable();
+                session().sendPartialBinary(partialByte, isLast, f);
                 return f;
             }
 
             @Override
-            public Future<Void> sendText(String text) throws IOException {
-                CompletableFuture<Void> f = new CompletableFuture<>();
-                session().getRemote().sendString(text, new WriteCallbackImpl(f));
+            public Future<Void> sendText(String text) {
+                Callback.Completable f = new Callback.Completable();
+                session().sendText(text, f);
                 return f;
             }
 
             @Override
-            public Future<Void> sendPing(ByteBuffer applicationData) throws IOException {
-                CompletableFuture<Void> f = new CompletableFuture<>();
-                session().getRemote().sendPing(applicationData, new WriteCallbackImpl(f));
+            public Future<Void> sendPing(ByteBuffer applicationData) {
+                Callback.Completable f = new Callback.Completable();
+                session().sendPing(applicationData, f);
                 return f;
             }
 
             @Override
-            public void close() throws IOException {
+            public void close() {
                 session().close();
             }
 
@@ -127,54 +124,57 @@
         };
     }
 
-    private static final class WriteCallbackImpl implements WriteCallback {
-        private final CompletableFuture<Void> f;
-
-        WriteCallbackImpl(CompletableFuture<Void> f) {
-            this.f = f;
-        }
-
-        @Override
-        public void writeSuccess() {
-            f.complete(null);
-        }
-
-        @Override
-        public void writeFailed(Throwable x) {
-            f.completeExceptionally(x);
-        }
-    }
-
     private static Object createWebSocket(JettyServerUpgradeRequest req, JettyServerUpgradeResponse resp) {
         Listener listener = (Listener) req.getHttpServletRequest().getAttribute(ATTR_LISTENER);
         if (listener == null) {
             throw new IllegalStateException("missing listener attribute");
         }
-        return new WebSocketListener() {
-            @Override
-            public void onWebSocketBinary(byte[] payload, int offset, int length) {
-                listener.onWebSocketBinary(payload, offset, length);
+        return new SessionListener(listener);
+    }
+
+    public static class SessionListener implements Session.Listener.AutoDemanding {
+        private final Listener listener;
+
+        public SessionListener(Listener listener) {
+            this.listener = Objects.requireNonNull(listener);
             }
 
             @Override
-            public void onWebSocketText(String message) {
-                listener.onWebSocketText(message);
+        public void onWebSocketOpen(Session session) {
+            listener.onWebSocketConnect(session);
             }
 
             @Override
-            public void onWebSocketClose(int statusCode, String reason) {
-                listener.onWebSocketClose(statusCode, reason);
+        public void onWebSocketBinary(ByteBuffer payload, Callback callback) {
+            try {
+                int len = payload.remaining();
+                if (payload.hasArray()) {
+                    listener.onWebSocketBinary(payload.array(), payload.arrayOffset() + payload.position(), len);
+                    payload.position(payload.position() + len);
+                } else {
+                    byte[] bytes = new byte[len];
+                    payload.get(bytes);
+                    listener.onWebSocketBinary(bytes, 0, len);
+                }
+                callback.succeed();
+            } catch (Throwable t) {
+                callback.fail(t);
+            }
             }
 
             @Override
-            public void onWebSocketConnect(Session session) {
-                listener.onWebSocketConnect(session);
+        public void onWebSocketText(String message) {
+            listener.onWebSocketText(message);
             }
 
             @Override
             public void onWebSocketError(Throwable cause) {
                 listener.onWebSocketError(cause);
             }
-        };
+
+        @Override
+        public void onWebSocketClose(int statusCode, String reason) {
+            listener.onWebSocketClose(statusCode, reason);
+        }
     }
 }

void onWebSocketError(Throwable cause);

void onWebSocketBinary(byte[] payload, int offset, int length);
void onWebSocketBinary(ByteBuffer data);

This comment was marked as outdated.

return new Handler() {
@Override
public Future<Void> sendBinary(ByteBuffer data) {
Callback.Completable f = new Callback.Completable();
Copy link
Member Author

Choose a reason for hiding this comment

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

From the migration guide:

The Jetty 11 WriteCallback class has been renamed to just Callback in Jetty 12, because it is now also used when receiving binary data. Note that this Callback interface is a different interface from the org.eclipse.jetty.util.Callback interface, which cannot be used in the Jetty WebSocket APIs due to class loader visibility issues.

return new SessionListener(listener);
}

public static class SessionListener implements Session.Listener.AutoDemanding {
Copy link
Member Author

Choose a reason for hiding this comment

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

From the migration guide:

The various Jetty 11 WebSocketListener interfaces have been replaced by a single interface in Jetty 12, Session.Listener.AutoDemanding (for more information, refer to this section).

}

@Override
public void onWebSocketBinary(ByteBuffer payload, Callback callback) {
Copy link
Member Author

@basil basil Apr 16, 2025

Choose a reason for hiding this comment

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

Here the Jetty interface has switched from byte[] payload, int offset, int length to ByteBuffer, so unwrapping the byte buffer before calling up into the Jenkins APIs which match the old signature.

basil added a commit to basil/acceptance-test-harness that referenced this pull request Apr 17, 2025
@basil basil added ath-successful This PR has successfully passed the full acceptance-test-harness suite and removed needs-ath-build Needs to run through the full acceptance-test-harness suite labels Apr 17, 2025
@github-actions github-actions bot added the unresolved-merge-conflict There is a merge conflict with the target branch. label Apr 24, 2025
@github-actions
Copy link
Contributor

Please take a moment and address the merge conflicts of your pull request. Thanks!

@github-actions github-actions bot removed the unresolved-merge-conflict There is a merge conflict with the target branch. label Apr 24, 2025
@basil basil marked this pull request as ready for review April 24, 2025 18:06
@basil basil requested a review from a team April 25, 2025 17:45
Comment on lines +150 to +158
int len = payload.remaining();
if (payload.hasArray()) {
listener.onWebSocketBinary(payload.array(), payload.arrayOffset() + payload.position(), len);
payload.position(payload.position() + len);
} else {
byte[] bytes = new byte[len];
payload.get(bytes);
listener.onWebSocketBinary(bytes, 0, len);
}
Copy link
Member Author

Choose a reason for hiding this comment

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

Likely the most important part of this PR from a reviewers' perspective. Note that eventually, we can clean up this code and make the Jenkins interface take a raw ByteBuffer, which will be cleaner. But since that involves making breaking API changes, I chose not to tackle that as part of this PR to minimize risk and disruption. Once we are on EE 10, I can come back to clean this up, which will be a minor breaking change in and of itself.

@github-actions github-actions bot added the unresolved-merge-conflict There is a merge conflict with the target branch. label Apr 26, 2025
@github-actions
Copy link
Contributor

Please take a moment and address the merge conflicts of your pull request. Thanks!

Copy link

@A1exKH A1exKH left a comment

Choose a reason for hiding this comment

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

@basil please resolve conflicts for this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ath-successful This PR has successfully passed the full acceptance-test-harness suite on-hold This pull request depends on another event/release, and it cannot be merged right now pct-successful This PR has successfully passed the full plugin-compatibility-test suite rfe For changelog: Minor enhancement. use `major-rfe` for changes to be highlighted unresolved-merge-conflict There is a merge conflict with the target branch.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants