Fixes #11096 - IllegalAccessException when invoking WebSocket endpoint methods in Jetty 12

documentation/jetty-documentation/src/main/asciidoc/programming-guide/websocket.adoc

@@ -59,6 +59,9 @@ xref:pg-websocket-endpoints-listener[Listener endpoints] are notified of events
 
 xref:pg-websocket-endpoints-annotated[Annotated endpoints] are notified of events by invoking the correspondent method annotated with the correspondent annotation from the `+org.eclipse.jetty.websocket.api.annotations.*+` package.
 
+Jetty uses ``MethodHandle``s to instantiate WebSocket endpoints and invoke WebSocket event methods, so WebSocket endpoint classes and WebSocket event methods must be `public`.
+This guarantees that WebSocket endpoints can be accessed by the Jetty implementation without additional configuration also when your application uses the Java Module System (JPMS).
+
 For both types of WebSocket endpoints, only one thread at a time will be delivering frame or message events to the corresponding methods; the next frame or message event will not be delivered until the previous call to the corresponding method has exited, and if there is xref:pg-websocket-endpoints-demand[demand] for it.
 Endpoints will always be notified of message events in the same order they were received over the network.
 
@@ -152,29 +155,6 @@ include::{doc_code}/org/eclipse/jetty/docs/programming/WebSocketDocs.java[tags=s
 
 A WebSocket endpoint may annotate methods with `+org.eclipse.jetty.websocket.api.annotations.*+` annotations to receive WebSocket events.
 
-Jetty uses ``MethodHandle``s to instantiate WebSocket endpoints and invoke WebSocket event methods, so WebSocket endpoint classes and WebSocket event methods must be `public`.
-
-When using JPMS, you must ensure that the Jetty JPMS module `org.eclipse.jetty.websocket.common` can _read_ (in JPMS terms) the WebSocket endpoint classes in your JPMS module.
-
-For example, your application may use the Jetty WebSocket client so that the JPMS module that contains your WebSocket endpoint classes looks like this:
-
-[source,java]
-.module-info.java
-----
-module com.acme.websocket
-{
-    // The Jetty WebSocket client dependency.
-    requires org.eclipse.jetty.websocket.client;
-}
-----
-
-To ensure that Jetty _reads_ your JPMS module, you must start the JVM with the following option:
-
-[source]
-----
-$ java --add-reads org.eclipse.jetty.websocket.common=com.acme.websocket ...
-----
-
 Each annotated event method may take an optional `Session` argument as its first parameter:
 
 [source,java,indent=0]

jetty-core/jetty-websocket/jetty-websocket-jetty-common/src/main/java/org/eclipse/jetty/websocket/common/JettyWebSocketFrameHandlerFactory.java

@@ -162,7 +162,7 @@ private JettyWebSocketFrameHandlerMetadata createListenerMetadata(Class<?> endpo
         JettyWebSocketFrameHandlerMetadata metadata = new JettyWebSocketFrameHandlerMetadata();
         metadata.setAutoDemand(Session.Listener.AutoDemanding.class.isAssignableFrom(endpointClass));
 
-        MethodHandles.Lookup lookup = JettyWebSocketFrameHandlerFactory.getServerMethodHandleLookup();
+        MethodHandles.Lookup lookup = getApplicationMethodHandleLookup(endpointClass);
 
         Method openMethod = findMethod(endpointClass, "onWebSocketOpen", Session.class);
         if (openMethod != null)
@@ -244,19 +244,14 @@ private Method findMethod(Class<?> klass, String name, Class<?>... parameters)
         Method method = ReflectUtils.findMethod(klass, name, parameters);
         if (method == null)
             return null;
-        if (!isOverridden(method))
-            return null;
-        // The method is overridden, but it may be declared in a non-public
-        // class, for example an anonymous class, where it won't be accessible,
-        // therefore replace it with the accessible version from Session.Listener.
-        if (!Modifier.isPublic(klass.getModifiers()))
-            method = ReflectUtils.findMethod(Session.Listener.class, name, parameters);
-        return method;
+        if (isOverridden(method))
+            return method;
+        return null;
     }
 
     private boolean isOverridden(Method method)
     {
-        return method != null && method.getDeclaringClass() != Session.Listener.class;
+        return method.getDeclaringClass() != Session.Listener.class;
     }
 
     private JettyWebSocketFrameHandlerMetadata createAnnotatedMetadata(WebSocket anno, Class<?> endpointClass)
@@ -265,10 +260,9 @@ private JettyWebSocketFrameHandlerMetadata createAnnotatedMetadata(WebSocket ann
         metadata.setAutoDemand(anno.autoDemand());
 
         MethodHandles.Lookup lookup = getApplicationMethodHandleLookup(endpointClass);
-        Method onmethod;
 
         // OnWebSocketOpen [0..1]
-        onmethod = ReflectUtils.findAnnotatedMethod(endpointClass, OnWebSocketOpen.class);
+        Method onmethod = ReflectUtils.findAnnotatedMethod(endpointClass, OnWebSocketOpen.class);
         if (onmethod != null)
         {
             assertSignatureValid(endpointClass, onmethod, OnWebSocketOpen.class);

jetty-core/jetty-websocket/jetty-websocket-jetty-tests/src/test/java/org/eclipse/jetty/websocket/tests/CloseTrackingEndpoint.java

@@ -44,7 +44,7 @@ public class CloseTrackingEndpoint extends Session.Listener.AbstractAutoDemandin
     public String closeReason = null;
     public CountDownLatch closeLatch = new CountDownLatch(1);
     public AtomicInteger closeCount = new AtomicInteger(0);
-    public CountDownLatch connectLatch = new CountDownLatch(1);
+    public CountDownLatch openLatch = new CountDownLatch(1);
     public CountDownLatch errorLatch = new CountDownLatch(1);
 
     public LinkedBlockingQueue<String> messageQueue = new LinkedBlockingQueue<>();
@@ -94,7 +94,7 @@ public void onWebSocketOpen(Session session)
     {
         super.onWebSocketOpen(session);
         LOG.debug("onWebSocketOpen({})", session);
-        connectLatch.countDown();
+        openLatch.countDown();
     }
 
     @Override

jetty-core/jetty-websocket/jetty-websocket-jetty-tests/src/test/java/org/eclipse/jetty/websocket/tests/client/ClientConnectTest.java

@@ -89,7 +89,7 @@ private <E extends Throwable> E assertExpectedError(ExecutionException e, CloseT
         assertThat("Error", capcause, errorMatcher);
 
         // Validate that websocket didn't see an open event
-        assertThat("Open Latch", wsocket.connectLatch.getCount(), is(1L));
+        assertThat("Open Latch", wsocket.openLatch.getCount(), is(1L));
 
         // Return the captured cause
         return (E)capcause;

jetty-core/jetty-websocket/jetty-websocket-jetty-tests/src/test/java/org/eclipse/jetty/websocket/tests/client/ConnectFutureTest.java

@@ -113,7 +113,7 @@ public void testAbortDuringCreator() throws Exception
         assertTrue(connect.cancel(true));
         assertThrows(CancellationException.class, () -> connect.get(5, TimeUnit.SECONDS));
         exitCreator.countDown();
-        assertFalse(clientSocket.connectLatch.await(1, TimeUnit.SECONDS));
+        assertFalse(clientSocket.openLatch.await(1, TimeUnit.SECONDS));
 
         Throwable error = clientSocket.error.get();
         assertThat(error, instanceOf(UpgradeException.class));
@@ -155,7 +155,7 @@ public void onWebSocketSessionCreated(Session session)
         assertTrue(connect.cancel(true));
         assertThrows(CancellationException.class, () -> connect.get(5, TimeUnit.SECONDS));
         exitListener.countDown();
-        assertTrue(clientSocket.connectLatch.await(5, TimeUnit.SECONDS));
+        assertTrue(clientSocket.openLatch.await(5, TimeUnit.SECONDS));
         assertTrue(clientSocket.errorLatch.await(5, TimeUnit.SECONDS));
         assertThat(clientSocket.error.get(), instanceOf(CancellationException.class));
     }
@@ -194,7 +194,7 @@ public void onHandshakeResponse(Request request, Response response)
         assertTrue(connect.cancel(true));
         assertThrows(CancellationException.class, () -> connect.get(5, TimeUnit.SECONDS));
         exitListener.countDown();
-        assertTrue(clientSocket.connectLatch.await(5, TimeUnit.SECONDS));
+        assertTrue(clientSocket.openLatch.await(5, TimeUnit.SECONDS));
         assertTrue(clientSocket.errorLatch.await(5, TimeUnit.SECONDS));
         assertThat(clientSocket.error.get(), instanceOf(CancellationException.class));
     }
@@ -205,29 +205,14 @@ public void testAbortOnOpened() throws Exception
         start(wsHandler ->
             wsHandler.getServerWebSocketContainer().addMapping("/", (upgradeRequest, upgradeResponse, callback) -> new EchoSocket()));
 
-        CountDownLatch exitOnConnect = new CountDownLatch(1);
-        CloseTrackingEndpoint clientSocket = new CloseTrackingEndpoint()
-        {
-            @Override
-            public void onWebSocketOpen(Session session)
-            {
-                try
-                {
-                    super.onWebSocketOpen(session);
-                    exitOnConnect.await();
-                }
-                catch (InterruptedException e)
-                {
-                    throw new IllegalStateException(e);
-                }
-            }
-        };
+        CountDownLatch exitOnOpen = new CountDownLatch(1);
+        AwaitOnOpen clientSocket = new AwaitOnOpen(exitOnOpen);
 
         // Abort during the call to onOpened. This is after the connection upgrade, but before future completion.
         Future<Session> connect = client.connect(clientSocket, WSURI.toWebsocket(server.getURI()));
-        assertTrue(clientSocket.connectLatch.await(5, TimeUnit.SECONDS));
+        assertTrue(clientSocket.openLatch.await(5, TimeUnit.SECONDS));
         assertTrue(connect.cancel(true));
-        exitOnConnect.countDown();
+        exitOnOpen.countDown();
 
         // We got an error on the WebSocket endpoint and an error from the future.
         assertTrue(clientSocket.errorLatch.await(5, TimeUnit.SECONDS));
@@ -245,7 +230,7 @@ public void testAbortAfterCompletion() throws Exception
         Session session = connect.get(5, TimeUnit.SECONDS);
 
         // If we can send and receive messages the future has been completed.
-        assertTrue(clientSocket.connectLatch.await(5, TimeUnit.SECONDS));
+        assertTrue(clientSocket.openLatch.await(5, TimeUnit.SECONDS));
         Session session1 = clientSocket.getSession();
         session1.sendText("hello", Callback.NOOP);
         assertThat(clientSocket.messageQueue.poll(5, TimeUnit.SECONDS), Matchers.is("hello"));
@@ -339,29 +324,14 @@ public void testAbortWithExceptionAfterUpgrade() throws Exception
         start(wsHandler ->
             wsHandler.getServerWebSocketContainer().addMapping("/", (upgradeRequest, upgradeResponse, callback) -> new EchoSocket()));
 
-        CountDownLatch exitOnConnect = new CountDownLatch(1);
-        CloseTrackingEndpoint clientSocket = new CloseTrackingEndpoint()
-        {
-            @Override
-            public void onWebSocketOpen(Session session)
-            {
-                try
-                {
-                    super.onWebSocketOpen(session);
-                    exitOnConnect.await();
-                }
-                catch (InterruptedException e)
-                {
-                    throw new IllegalStateException(e);
-                }
-            }
-        };
+        CountDownLatch exitOnOpen = new CountDownLatch(1);
+        AwaitOnOpen clientSocket = new AwaitOnOpen(exitOnOpen);
 
         // Complete the CompletableFuture with an exception the during the call to onOpened.
         CompletableFuture<Session> connect = client.connect(clientSocket, WSURI.toWebsocket(server.getURI()));
-        assertTrue(clientSocket.connectLatch.await(5, TimeUnit.SECONDS));
+        assertTrue(clientSocket.openLatch.await(5, TimeUnit.SECONDS));
         assertTrue(connect.completeExceptionally(new WebSocketException("custom exception")));
-        exitOnConnect.countDown();
+        exitOnOpen.countDown();
 
         // Exception from the future is correct.
         ExecutionException futureError = assertThrows(ExecutionException.class, () -> connect.get(5, TimeUnit.SECONDS));
@@ -375,4 +345,28 @@ public void onWebSocketOpen(Session session)
         assertThat(endpointError, instanceOf(WebSocketException.class));
         assertThat(endpointError.getMessage(), is("custom exception"));
     }
+
+    public static class AwaitOnOpen extends CloseTrackingEndpoint
+    {
+        private final CountDownLatch exitOnOpen;
+
+        public AwaitOnOpen(CountDownLatch latch)
+        {
+            exitOnOpen = latch;
+        }
+
+        @Override
+        public void onWebSocketOpen(Session session)
+        {
+            try
+            {
+                super.onWebSocketOpen(session);
+                exitOnOpen.await();
+            }
+            catch (InterruptedException e)
+            {
+                throw new IllegalStateException(e);
+            }
+        }
+    }
 }

jetty-core/jetty-websocket/jetty-websocket-jetty-tests/src/test/java/org/eclipse/jetty/websocket/tests/client/WebSocketClientTest.java

@@ -385,7 +385,7 @@ public void testLocalRemoteAddress() throws Exception
 
         try (Session ignored = future.get(5, TimeUnit.SECONDS))
         {
-            Assertions.assertTrue(cliSock.connectLatch.await(1, TimeUnit.SECONDS));
+            Assertions.assertTrue(cliSock.openLatch.await(1, TimeUnit.SECONDS));
 
             InetSocketAddress local = (InetSocketAddress)cliSock.getSession().getLocalSocketAddress();
             InetSocketAddress remote = (InetSocketAddress)cliSock.getSession().getRemoteSocketAddress();

jetty-core/jetty-websocket/jetty-websocket-jetty-tests/src/test/java/org/eclipse/jetty/websocket/tests/listeners/WebSocketListenerTest.java

@@ -16,16 +16,13 @@
 import java.net.URI;
 import java.nio.ByteBuffer;
 import java.util.List;
-import java.util.concurrent.BlockingQueue;
-import java.util.concurrent.CountDownLatch;
 import java.util.concurrent.TimeUnit;
 import java.util.stream.Collectors;
 import java.util.stream.Stream;
 
 import org.eclipse.jetty.server.Server;
 import org.eclipse.jetty.server.ServerConnector;
 import org.eclipse.jetty.server.handler.ContextHandler;
-import org.eclipse.jetty.util.BlockingArrayQueue;
 import org.eclipse.jetty.util.BufferUtil;
 import org.eclipse.jetty.websocket.api.Callback;
 import org.eclipse.jetty.websocket.api.Session;
@@ -42,7 +39,9 @@
 import org.junit.jupiter.params.provider.MethodSource;
 
 import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.instanceOf;
 import static org.hamcrest.Matchers.is;
+import static org.junit.jupiter.api.Assertions.assertThrows;
 import static org.junit.jupiter.api.Assertions.assertTrue;
 
 public class WebSocketListenerTest
@@ -130,44 +129,18 @@ public void testBinaryListeners(Class<?> clazz) throws Exception
     }
 
     @Test
-    public void testAnonymousListener() throws Exception
+    public void testAnonymousListener()
     {
-        CountDownLatch openLatch = new CountDownLatch(1);
-        CountDownLatch closeLatch = new CountDownLatch(1);
-        BlockingQueue<String> textMessages = new BlockingArrayQueue<>();
         Session.Listener clientEndpoint = new Session.Listener.AutoDemanding()
         {
             @Override
             public void onWebSocketOpen(Session session)
             {
-                openLatch.countDown();
-            }
-
-            @Override
-            public void onWebSocketText(String message)
-            {
-                textMessages.add(message);
-            }
-
-            @Override
-            public void onWebSocketClose(int statusCode, String reason)
-            {
-                closeLatch.countDown();
             }
         };
-
-        Session session = client.connect(clientEndpoint, serverUri.resolve("/echo")).get(5, TimeUnit.SECONDS);
-        assertTrue(openLatch.await(5, TimeUnit.SECONDS));
-
-        // Send and receive echo on client.
-        String payload = "hello world";
-        session.sendText(payload, Callback.NOOP);
-        String echoMessage = textMessages.poll(5, TimeUnit.SECONDS);
-        assertThat(echoMessage, is(payload));
-
-        // Close normally.
-        session.close(StatusCode.NORMAL, "standard close", Callback.NOOP);
-        assertTrue(closeLatch.await(5, TimeUnit.SECONDS));
+        Exception failure = assertThrows(Exception.class, () -> client.connect(clientEndpoint, serverUri.resolve("/echo")).get(5, TimeUnit.SECONDS));
+        // The endpoint class is not public.
+        assertThat(failure.getCause(), instanceOf(IllegalAccessException.class));
     }
 
     private List<Class<?>> getClassListFromArguments(Stream<Arguments> stream)

jetty-core/jetty-websocket/jetty-websocket-jetty-tests/src/test/java/org/eclipse/jetty/websocket/tests/server/AbstractCloseEndpoint.java

@@ -29,7 +29,7 @@
 public abstract class AbstractCloseEndpoint extends Session.Listener.AbstractAutoDemanding
 {
     public final Logger log;
-    public CountDownLatch connectLatch = new CountDownLatch(1);
+    public CountDownLatch openLatch = new CountDownLatch(1);
     public CountDownLatch closeLatch = new CountDownLatch(1);
     public String closeReason = null;
     public int closeStatusCode = -1;
@@ -45,7 +45,7 @@ public void onWebSocketOpen(Session sess)
     {
         super.onWebSocketOpen(sess);
         log.debug("onWebSocketOpen({})", sess);
-        connectLatch.countDown();
+        openLatch.countDown();
     }
 
     @Override

jetty-core/jetty-websocket/jetty-websocket-jetty-tests/src/test/java/org/eclipse/jetty/websocket/tests/server/DirectUpgradeTest.java

@@ -203,7 +203,7 @@ public boolean handle(Request request, Response response, Callback callback)
         assertEquals("HELLO", response.getContentAsString());
     }
 
-    private static class EchoListener implements Session.Listener
+    public static class EchoListener implements Session.Listener
     {
         private Session session;
 

jetty-core/jetty-websocket/jetty-websocket-jetty-tests/src/test/java/org/eclipse/jetty/websocket/tests/server/ServerCloseTest.java

@@ -269,7 +269,7 @@ public void testSecondCloseFromOnClosed() throws Exception
 
         // Hard close from the server. Server onClosed() will try to close again which should be a NOOP.
         AbstractCloseEndpoint serverEndpoint = serverEndpointCreator.pollLastCreated();
-        assertTrue(serverEndpoint.connectLatch.await(5, SECONDS));
+        assertTrue(serverEndpoint.openLatch.await(5, SECONDS));
         Session session = serverEndpoint.getSession();
         session.close(StatusCode.SHUTDOWN, "SHUTDOWN hard close", Callback.NOOP);
 
@@ -294,7 +294,7 @@ public void testSecondCloseFromOnClosedInNewThread() throws Exception
 
         // Hard close from the server. Server onClosed() will try to close again which should be a NOOP.
         AbstractCloseEndpoint serverEndpoint = serverEndpointCreator.pollLastCreated();
-        assertTrue(serverEndpoint.connectLatch.await(5, SECONDS));
+        assertTrue(serverEndpoint.openLatch.await(5, SECONDS));
         Session session = serverEndpoint.getSession();
         session.close(StatusCode.SHUTDOWN, "SHUTDOWN hard close", Callback.NOOP);
 

tests/jetty-testers/src/main/java/org/eclipse/jetty/tests/testers/MavenHelper.java

@@ -73,7 +73,8 @@ private DefaultRepositorySystemSession newRepositorySystemSession(RepositorySyst
 
     private static RemoteRepository newCentralRepository()
     {
-        return new RemoteRepository.Builder("central", "default", "https://repo.maven.apache.org/maven2/").build();
+        String centralRepository = System.getProperty("maven.repo.uri", "https://repo.maven.apache.org/maven2/");
+        return new RemoteRepository.Builder("central", "default", centralRepository).build();
     }
 
     private static class LogTransferListener extends AbstractTransferListener

tests/pom.xml

@@ -16,7 +16,7 @@
     <module>jetty-test-session-common</module>
     <module>test-distribution</module>
     <module>test-integration</module>
-    <!--<module>test-jpms</module>-->
+    <module>test-jpms</module>
   </modules>
   <properties>
     <maven.deploy.skip>true</maven.deploy.skip>