[JENKINS-68933] Better WebSocket testing, removal of reflection by jglick · Pull Request #6780 · jenkinsci/jenkins

jglick · 2022-07-05T13:23:21Z

See JENKINS-68933 and #6694 (comment). Amending #4369.

Proposed changelog entries

N/A (internal changes only)

Maintainer checklist

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

There are at least 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 does not block the change
Changelog entries in the PR 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, upgrade-guide-needed label is set and there is a Proposed upgrade guidelines section in the PR title. (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).

jglick · 2022-07-05T13:40:03Z

Looking over jetty/jetty.project#6623 & jetty/jetty.project#3544 & jetty/jetty.project#5874. Does not look like this will be straightforward—the APIs have changed substantially, and until jenkins-test-harness is also on Jetty 10, (non-Real) JenkinsRule-based tests will still require the old APIs, so the reflection will need to support both.

timja · 2022-07-05T14:30:37Z

and until jenkins-test-harness is also on Jetty 10

could we just move it across? and maintain a backport line for fixes that are substantial enough?

jglick · 2022-07-05T14:43:34Z

Perhaps. First I want to try to get it to work on 10, and then I can look into making it still work on 9.

jglick · 2022-07-05T18:48:21Z

Rebased the test on top of the parent of #6694, where it passed.

jglick · 2022-07-05T21:23:31Z

Tests appear to fail here because the build is running PR-merge strategy. @timja @dduportal I thought we had decided to switch builds of this repo to use PR-head strategy? Maybe it was only bom?

timja · 2022-07-05T22:04:21Z

Tests appear to fail here because the build is running PR-merge strategy. @timja @dduportal I thought we had decided to switch builds of this repo to use PR-head strategy? Maybe it was only bom?

only bom was switched

…-JENKINS-68933

jglick · 2022-07-05T23:10:52Z

Split off #6785 to attempt (so far without success) to use this framework to support Jetty 10.

basil

Thanks Jesse, I had started working on a similar PR earlier today but you obviously got it done a lot faster. In the interest of saving time I took the liberty of pushing a few commits to your branch to fix static analysis issues. I also left a comment in #6785 (comment) explaining how to get this to work with Jetty 10.

basil · 2022-07-06T01:49:52Z

Well now there's a a javadoc: error - No public or protected classes found to document but once that is resolved I think this is good to go.

jglick · 2022-07-06T12:56:55Z

    <spotbugs.effort>Max</spotbugs.effort>
    <spotbugs.threshold>Medium</spotbugs.threshold>
-    <spotbugs.excludeFilterFile>${project.basedir}/../src/spotbugs/spotbugs-excludes.xml</spotbugs.excludeFilterFile>
+    <spotbugs.excludeFilterFile>${maven.multiModuleProjectDirectory}/src/spotbugs/spotbugs-excludes.xml</spotbugs.excludeFilterFile>


jglick · 2022-07-06T13:01:09Z

In the interest of saving time

Well, there is no rush on this one since it does not fix anything for users, it is just making it easier to work on this code.

I took the liberty of pushing a few commits to your branch to fix static analysis issues.

Thanks! I will check the Javadoc issue.

…-JENKINS-68933

jglick · 2022-07-06T13:22:22Z

+        <artifactId>maven-javadoc-plugin</artifactId>
+        <configuration>
+          <skip>true</skip>


Note that this should probably also be done in the cli and war modules.

jglick · 2022-07-06T13:56:16Z

+        };
+    }
+
+    private static Object createWebSocket(ServletUpgradeRequest req, ServletUpgradeResponse resp) {


Note the Object in the signature. Javadoc is super helpful

a websocket object to use

It is more or less through trial and error that I concluded (in #4369) that a WebSocketListener is expected; “normal usage” apparently means adding annotations to POJOs and just remembering what method signatures to use. I think this misconceived style is inherited from the javax.websocket API. (Which tries to make you declare WS-aware endpoints with more annotations, and fails to provide a simple Java API call to upgrade a connection, forcing us to rely on Jetty-specific APIs.)

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.

basil · 2022-07-06T16:40:52Z

This PR is now ready for merge. We will merge it after approximately 24 hours if there is no negative feedback. Please see the merge process documentation for more information about the merge process. Thanks!

jglick · 2022-07-06T16:48:27Z

      </dependency>
+      <dependency>
+        <groupId>org.kohsuke.metainf-services</groupId>
+        <artifactId>metainf-services</artifactId>


Note that this will cause that library to be managed for plugin POMs (like credentials or artifact-manager-s3) for the first time. Seems like a good thing; I am not sure why it was not already in the core BOM. But if this is a problem for any reason, it could trivially be reverted, and the version fixed in the new module.

Yeah, I don't see a problem with including it in the core BOM. This PR does also start shipping WEB-INF/lib/metainf-services-1.9.jar, which seems harmless, but I'm not entirely sure it's necessary either, since it seems to be more of a compile-time thing. No request for change or action item here, just noting the additional JAR.

FWIW there is an exclusion for this library elsewhere in the build: https://github.com/jenkinsci/jenkins/blob/644a261e78e9d80e2442787c8418cbdf283ac7e1/war/pom.xml#L92-L96=

jglick · 2022-07-06T16:52:07Z

+    <dependency>
+      <groupId>javax.servlet</groupId>
+      <artifactId>javax.servlet-api</artifactId>
+      <version>3.1.0</version>


Deliberately not using BOM version here.

Yeah, I saw that and I agree with using 3.1.0 because that is the lowest common denominator for Jetty 9 and Jetty 10. But the downside is this is adding the JAR to WEB-INF/lib/javax.servlet-api-3.1.0.jar in the WAR, which I think is undesirable because it is not actually used at runtime; rather, Winstone's copy is used. So to fix this I think we should exclude it when we pull in websocket-spi in core/pom.xml, which already has a provided dependency on servlet-api that is aligned with Jetty's.

…-JENKINS-68933

olamy

oh yes Jetty 10 API for websocket has changed a lot (it's supposed to be more easy to use but still a big change...)
good work here! (Hopefully we will not have to keep this jetty9 too long?)

jglick · 2022-09-07T15:19:46Z

+    private static final String ATTR_LISTENER = Jetty9Provider.class.getName() + ".listener";
+
+    // TODO does not seem possible to use HttpServletRequest.get/setAttribute for this
+    private static final Map<Listener, Session> sessions = new WeakHashMap<>();


regression: #7076

jglick requested review from basil, daniel-beck and timja July 5, 2022 13:23

jglick mentioned this pull request Jul 5, 2022

Revert #6694 #6781

Merged

12 tasks

[JENKINS-68933] Verify basic WebSocket connectivity in RealJenkinsRule

8a14f9d

jglick force-pushed the ws-JENKINS-68933 branch from fc7b2fd to 8a14f9d Compare July 5, 2022 18:47

jglick added 4 commits July 5, 2022 18:04

Removing use of reflection to access Jetty WebSocket types

c670842

Merge branch 'master' of https://github.com/jenkinsci/jenkins into ws…

ce2b909

…-JENKINS-68933

Removing javax.servlet-api from BOM

b6f67d1

Tweaking packaging

476a6fa

jglick changed the title ~~[JENKINS-68933] WebSocket broken after Jetty upgrade~~ [JENKINS-68933] Better WebSocket testing, removal of reflection Jul 5, 2022

jglick mentioned this pull request Jul 5, 2022

[JENKINS-68933] Update to Jetty 10, with WebSocket #6785

Closed

6 tasks

jglick marked this pull request as ready for review July 5, 2022 23:09

basil added 3 commits July 5, 2022 17:47

Checkstyle

944ad29

SpotBugs

d7438dc

Enforcer

a14541b

basil approved these changes Jul 6, 2022

View reviewed changes

timja approved these changes Jul 6, 2022

View reviewed changes

jglick commented Jul 6, 2022

View reviewed changes

jglick added 2 commits July 6, 2022 09:02

Merge branch 'master' of https://github.com/jenkinsci/jenkins into ws…

b7bc5c4

…-JENKINS-68933

Skip Javadoc on new modules

01e59fc

jglick commented Jul 6, 2022

View reviewed changes

basil added ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback skip-changelog Should not be shown in the changelog labels Jul 6, 2022

jglick commented Jul 6, 2022

View reviewed changes

jglick marked this pull request as draft July 6, 2022 17:24

Do not bundle javax.servlet-api: jenkinsci#6780 (comment)

e98f3cd

jglick marked this pull request as ready for review July 6, 2022 17:30

jglick mentioned this pull request Jul 6, 2022

[INFRA-1633] Stop building PR merges jenkins-infra/helpdesk#1355

Closed

basil pushed a commit to jglick/jenkins that referenced this pull request Jul 6, 2022

Do not bundle javax.servlet-api: jenkinsci#6780 (comment)

f190f5d

basil reviewed Jul 6, 2022

View reviewed changes

Comment thread websocket/jetty9/src/main/java/jenkins/websocket/Jetty9Provider.java

jglick added 2 commits July 6, 2022 17:23

Merge branch 'master' of https://github.com/jenkinsci/jenkins into ws…

9f57b6f

…-JENKINS-68933

Do not propagate winstone dep to test: jenkinsci#6785 (comment)

ac6d1ef

olamy approved these changes Jul 7, 2022

View reviewed changes

basil added 2 commits July 7, 2022 08:19

Merge remote-tracking branch 'origin/master' into ws-JENKINS-68933

b779a45

Update IntelliJ encodings

76f965f

basil merged commit 6de288f into jenkinsci:master Jul 7, 2022

jglick deleted the ws-JENKINS-68933 branch July 7, 2022 16:39

This was referenced Jul 7, 2022

Permissions for new WebSocket components jenkins-infra/repository-permissions-updater#2638

Merged

[JENKINS-68694] Winstone 6.1: Upgrade Jetty from 9.4.46.v20220331 to 10.0.11 #6801

Merged

Vlatombe mentioned this pull request Sep 7, 2022

[JENKINS-69543] Fix thread safety in websockets handling. #7076

Merged

13 tasks

jglick commented Sep 7, 2022

View reviewed changes

jglick mentioned this pull request Nov 18, 2022

[JENKINS-70103] WeakHashMap broken for looking up Jetty Session in WebSocket response #7412

Merged

6 tasks

basil mentioned this pull request Jun 20, 2023

Stop shipping metainf-services in the WAR #8174

Merged

jglick mentioned this pull request Sep 19, 2024

Test regressions after JTH bump #9762

Merged

Uh oh!

Uh oh!

Conversation

jglick commented Jul 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Proposed changelog entries

Maintainer checklist

Uh oh!

jglick commented Jul 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

timja commented Jul 5, 2022

Uh oh!

jglick commented Jul 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jglick commented Jul 5, 2022

Uh oh!

jglick commented Jul 5, 2022

Uh oh!

timja commented Jul 5, 2022

Uh oh!

jglick commented Jul 5, 2022

Uh oh!

basil left a comment

Choose a reason for hiding this comment

Uh oh!

basil commented Jul 6, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jglick commented Jul 6, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

basil commented Jul 6, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

olamy left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jglick commented Jul 5, 2022 •

edited

Loading

jglick commented Jul 5, 2022 •

edited

Loading

jglick commented Jul 5, 2022 •

edited

Loading