[JENKINS-68933] Better WebSocket testing, removal of reflection (jenk…

…insci#6780)
janfaracik · Jul 7, 2022 · 6de288f · 6de288f
1 parent d9d951b
commit 6de288f
Show file tree

Hide file tree

Showing 13 changed files with 553 additions and 126 deletions.
diff --git a/.idea/encodings.xml b/.idea/encodings.xml
diff --git a/bom/pom.xml b/bom/pom.xml
@@ -296,6 +296,11 @@ THE SOFTWARE.
         <artifactId>j-interop</artifactId>
         <version>2.0.8-kohsuke-1</version>
       </dependency>
+      <dependency>
+        <groupId>org.kohsuke.metainf-services</groupId>
+        <artifactId>metainf-services</artifactId>
+        <version>1.9</version>
+      </dependency>
       <dependency>
         <groupId>org.kohsuke.stapler</groupId>
         <artifactId>json-lib</artifactId>

diff --git a/core/pom.xml b/core/pom.xml
@@ -328,6 +328,17 @@ THE SOFTWARE.
       <groupId>org.jenkins-ci</groupId>
       <artifactId>version-number</artifactId>
     </dependency>
+    <dependency>
+      <groupId>org.jenkins-ci.main</groupId>
+      <artifactId>websocket-spi</artifactId>
+      <version>${project.version}</version>
+      <exclusions>
+        <exclusion>
+          <groupId>javax.servlet</groupId>
+          <artifactId>javax.servlet-api</artifactId>
+        </exclusion>
+      </exclusions>
+    </dependency>
     <dependency>
       <groupId>org.jfree</groupId>
       <artifactId>jfreechart</artifactId>
@@ -375,10 +386,8 @@ THE SOFTWARE.
       <artifactId>j-interop</artifactId>
     </dependency>
     <dependency>
-      <!-- not in BOM, optional -->
       <groupId>org.kohsuke.metainf-services</groupId>
       <artifactId>metainf-services</artifactId>
-      <version>1.9</version>
       <optional>true</optional>
     </dependency>
     <dependency>

diff --git a/core/src/main/java/jenkins/websocket/WebSocketSession.java b/core/src/main/java/jenkins/websocket/WebSocketSession.java
@@ -25,7 +25,6 @@
 package jenkins.websocket;
 
 import java.io.IOException;
-import java.lang.reflect.Method;
 import java.nio.ByteBuffer;
 import java.util.concurrent.Future;
 import java.util.concurrent.ScheduledFuture;
@@ -60,48 +59,28 @@ public abstract class WebSocketSession {
 
     private static final Logger LOGGER = Logger.getLogger(WebSocketSession.class.getName());
 
-    private Object session;
-    // https://www.eclipse.org/jetty/javadoc/9.4.24.v20191120/org/eclipse/jetty/websocket/common/WebSocketRemoteEndpoint.html
-    private Object remoteEndpoint;
+    Provider.Handler handler;
     private ScheduledFuture<?> pings;
 
     protected WebSocketSession() {}
 
-    Object onWebSocketSomething(Object proxy, Method method, Object[] args) throws Exception {
-        switch (method.getName()) {
-        case "onWebSocketConnect":
-            this.session = args[0];
-            this.remoteEndpoint = session.getClass().getMethod("getRemote").invoke(args[0]);
-            if (PING_INTERVAL_SECONDS != 0) {
-                pings = Timer.get().scheduleAtFixedRate(() -> {
-                    try {
-                        remoteEndpoint.getClass().getMethod("sendPing", ByteBuffer.class).invoke(remoteEndpoint, ByteBuffer.wrap(new byte[0]));
-                    } catch (Exception x) {
-                        error(x);
-                        pings.cancel(true);
-                    }
-                }, PING_INTERVAL_SECONDS / 2, PING_INTERVAL_SECONDS, TimeUnit.SECONDS);
-            }
-            opened();
-            return null;
-        case "onWebSocketClose":
-            if (pings != null) {
-                pings.cancel(true);
-                // alternately, check Session.isOpen each time
-            }
-            closed((Integer) args[0], (String) args[1]);
-            return null;
-        case "onWebSocketError":
-            error((Throwable) args[0]);
-            return null;
-        case "onWebSocketBinary":
-            binary((byte[]) args[0], (Integer) args[1], (Integer) args[2]);
-            return null;
-        case "onWebSocketText":
-            text((String) args[0]);
-            return null;
-        default:
-            throw new AssertionError();
+    void startPings() {
+        if (PING_INTERVAL_SECONDS != 0) {
+            pings = Timer.get().scheduleAtFixedRate(() -> {
+                try {
+                    handler.sendPing(ByteBuffer.wrap(new byte[0]));
+                } catch (Exception x) {
+                    error(x);
+                    pings.cancel(true);
+                }
+            }, PING_INTERVAL_SECONDS / 2, PING_INTERVAL_SECONDS, TimeUnit.SECONDS);
+        }
+    }
+
+    void stopPings() {
+        if (pings != null) {
+            pings.cancel(true);
+            // alternately, check Session.isOpen each time
         }
     }
 
@@ -123,38 +102,20 @@ protected void text(String message) throws IOException {
         LOGGER.warning("unexpected text frame");
     }
 
-    @SuppressWarnings("unchecked")
     protected final Future<Void> sendBinary(ByteBuffer data) throws IOException {
-        try {
-            return (Future<Void>) remoteEndpoint.getClass().getMethod("sendBytesByFuture", ByteBuffer.class).invoke(remoteEndpoint, data);
-        } catch (Exception x) {
-            throw new IOException(x);
-        }
+        return handler.sendBinary(data);
     }
 
     protected final void sendBinary(ByteBuffer partialByte, boolean isLast) throws IOException {
-        try {
-            remoteEndpoint.getClass().getMethod("sendPartialBytes", ByteBuffer.class, boolean.class).invoke(remoteEndpoint, partialByte, isLast);
-        } catch (Exception x) {
-            throw new IOException(x);
-        }
+        handler.sendBinary(partialByte, isLast);
     }
 
-    @SuppressWarnings("unchecked")
     protected final Future<Void> sendText(String text) throws IOException {
-        try {
-            return (Future<Void>) remoteEndpoint.getClass().getMethod("sendStringByFuture", String.class).invoke(remoteEndpoint, text);
-        } catch (Exception x) {
-            throw new IOException(x);
-        }
+        return handler.sendText(text);
     }
 
     protected final void close() throws IOException {
-        try {
-            session.getClass().getMethod("close").invoke(session);
-        } catch (Exception x) {
-            throw new IOException(x);
-        }
+        handler.close();
     }
 
 }
diff --git a/core/src/main/java/jenkins/websocket/WebSockets.java b/core/src/main/java/jenkins/websocket/WebSockets.java
@@ -24,99 +24,101 @@
 
 package jenkins.websocket;
 
-import hudson.Extension;
-import hudson.ExtensionList;
-import java.lang.reflect.Method;
-import java.lang.reflect.Proxy;
+import java.io.IOException;
+import java.nio.channels.ClosedChannelException;
+import java.util.Iterator;
+import java.util.ServiceConfigurationError;
+import java.util.ServiceLoader;
 import java.util.logging.Level;
 import java.util.logging.Logger;
-import javax.servlet.ServletContext;
-import javax.servlet.http.HttpServletRequest;
-import javax.servlet.http.HttpServletResponse;
 import org.kohsuke.accmod.Restricted;
 import org.kohsuke.accmod.restrictions.Beta;
 import org.kohsuke.stapler.HttpResponse;
 import org.kohsuke.stapler.HttpResponses;
-import org.kohsuke.stapler.Stapler;
 
 /**
  * Support for serving WebSocket responses.
  * @since 2.216
  */
 @Restricted(Beta.class)
-@Extension
 public class WebSockets {
 
     private static final Logger LOGGER = Logger.getLogger(WebSockets.class.getName());
 
-    private static final String ATTR_SESSION = WebSockets.class.getName() + ".session";
+    private static final Provider provider = findProvider();
+
+    private static Provider findProvider() {
+        Iterator<Provider> it = ServiceLoader.load(Provider.class).iterator();
+        while (it.hasNext()) {
+            try {
+                return it.next();
+            } catch (ServiceConfigurationError x) {
+                LOGGER.log(Level.FINE, null, x);
+            }
+        }
+        return null;
+    }
 
     // TODO ability to handle subprotocols?
 
     public static HttpResponse upgrade(WebSocketSession session) {
+        if (provider == null) {
+            throw HttpResponses.notFound();
+        }
         return (req, rsp, node) -> {
             try {
-                Object factory = ExtensionList.lookupSingleton(WebSockets.class).init();
-                if (!((Boolean) webSocketServletFactoryClass.getMethod("isUpgradeRequest", HttpServletRequest.class, HttpServletResponse.class).invoke(factory, req, rsp))) {
-                    throw HttpResponses.errorWithoutStack(HttpServletResponse.SC_BAD_REQUEST, "only WS connections accepted here");
-                }
-                req.setAttribute(ATTR_SESSION, session);
-                if (!((Boolean) webSocketServletFactoryClass.getMethod("acceptWebSocket", HttpServletRequest.class, HttpServletResponse.class).invoke(factory, req, rsp))) {
-                    throw HttpResponses.errorWithoutStack(HttpServletResponse.SC_BAD_REQUEST, "did not manage to upgrade");
-                }
-            } catch (HttpResponses.HttpResponseException x) {
-                throw x;
+                session.handler = provider.handle(req, rsp, new Provider.Listener() {
+                    @Override
+                    public void onWebSocketConnect() {
+                        session.startPings();
+                        session.opened();
+                    }
+
+                    @Override
+                    public void onWebSocketClose(int statusCode, String reason) {
+                        session.stopPings();
+                        session.closed(statusCode, reason);
+                    }
+
+                    @Override
+                    public void onWebSocketError(Throwable cause) {
+                        if (cause instanceof ClosedChannelException) {
+                            onWebSocketClose(0, cause.toString());
+                        } else {
+                            session.error(cause);
+                        }
+                    }
+
+                    @Override
+                    public void onWebSocketBinary(byte[] payload, int offset, int length) {
+                        try {
+                            session.binary(payload, offset, length);
+                        } catch (IOException x) {
+                            session.error(x);
+                        }
+                    }
+
+                    @Override
+                    public void onWebSocketText(String message) {
+                        try {
+                            session.text(message);
+                        } catch (IOException x) {
+                            session.error(x);
+                        }
+                    }
+                });
             } catch (Exception x) {
                 LOGGER.log(Level.WARNING, null, x);
                 throw HttpResponses.error(x);
             }
-            // OK!
+            // OK, unless handler is null in which case we expect an error was already sent.
         };
     }
 
-    private static ClassLoader cl;
-    private static Class<?> webSocketServletFactoryClass;
-
-    private static synchronized void staticInit() throws Exception {
-        if (webSocketServletFactoryClass == null) {
-            cl = ServletContext.class.getClassLoader();
-            webSocketServletFactoryClass = cl.loadClass("org.eclipse.jetty.websocket.servlet.WebSocketServletFactory");
-        }
-    }
-
     public static boolean isSupported() {
-        try {
-            staticInit();
-            return true;
-        } catch (Exception x) {
-            LOGGER.log(Level.FINE, null, x);
-            return false;
-        }
+        return provider != null;
     }
 
-    private /*WebSocketServletFactory*/Object factory;
-
-    private synchronized Object init() throws Exception {
-        if (factory == null) {
-            staticInit();
-            Class<?> webSocketPolicyClass = cl.loadClass("org.eclipse.jetty.websocket.api.WebSocketPolicy");
-            factory = cl.loadClass("org.eclipse.jetty.websocket.servlet.WebSocketServletFactory$Loader")
-                    .getMethod("load", ServletContext.class, webSocketPolicyClass)
-                    .invoke(
-                            null,
-                            Stapler.getCurrent().getServletContext(),
-                            webSocketPolicyClass.getMethod("newServerPolicy").invoke(null));
-            webSocketServletFactoryClass.getMethod("start").invoke(factory);
-            Class<?> webSocketCreatorClass = cl.loadClass("org.eclipse.jetty.websocket.servlet.WebSocketCreator");
-            webSocketServletFactoryClass.getMethod("setCreator", webSocketCreatorClass).invoke(factory, Proxy.newProxyInstance(cl, new Class<?>[] {webSocketCreatorClass}, this::createWebSocket));
-        }
-        return factory;
-    }
-
-    private Object createWebSocket(Object proxy, Method method, Object[] args) throws Exception {
-        Object servletUpgradeRequest = args[0];
-        WebSocketSession session = (WebSocketSession) servletUpgradeRequest.getClass().getMethod("getServletAttribute", String.class).invoke(servletUpgradeRequest, ATTR_SESSION);
-        return Proxy.newProxyInstance(cl, new Class<?>[] {cl.loadClass("org.eclipse.jetty.websocket.api.WebSocketListener")}, session::onWebSocketSomething);
-    }
+    private WebSockets() {}
 
 }
diff --git a/pom.xml b/pom.xml
@@ -51,6 +51,8 @@ THE SOFTWARE.
 
   <modules>
     <module>bom</module>
+    <module>websocket/spi</module>
+    <module>websocket/jetty9</module>
     <module>core</module>
     <module>war</module>
     <module>test</module>
@@ -94,7 +96,7 @@ THE SOFTWARE.
 
     <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>
 
     <access-modifier.version>1.27</access-modifier.version>
     <bridge-method-injector.version>1.23</bridge-method-injector.version>

diff --git a/src/spotbugs/spotbugs-excludes.xml b/src/spotbugs/spotbugs-excludes.xml
@@ -658,6 +658,8 @@
           <Class name="jenkins.slaves.restarter.WinswSlaveRestarter"/>
           <Class name="jenkins.slaves.StandardOutputSwapper$ChannelSwapper"/>
           <Class name="jenkins.util.ProgressiveRendering"/>
+          <Class name="jenkins.websocket.Jetty9Provider"/>
+          <Class name="jenkins.websocket.Provider"/>
           <Class name="jenkins.websocket.WebSockets"/>
           <Class name="jenkins.websocket.WebSocketSession"/>
           <Class name="jenkins.widgets.RunListProgressiveRendering"/>