Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

IllegalAccessException when invoking WebSocket end point methods in Jetty 12 #11096

Closed
unitydynamics opened this issue Dec 29, 2023 · 10 comments · Fixed by #11229
Closed

IllegalAccessException when invoking WebSocket end point methods in Jetty 12 #11096

unitydynamics opened this issue Dec 29, 2023 · 10 comments · Fixed by #11229
Labels
Bug For general bugs on Jetty side

Comments

@unitydynamics
Copy link

unitydynamics commented Dec 29, 2023

Jetty version(s)
Jetty 12.0.5

Jetty Environment
core

Java version/vendor (use: java -version)
OpenJDK 21.0.1+12-29

OS type/version
Ubuntu

Description
When using modules in Java 21 (must have a module-info.java). Jetty fails to load the web socket endpoint you've provided:

Exception in thread "main" java.lang.RuntimeException: Unable to access method public void org.example.TestWebSocketEndPoint.onWebSocketOpen(org.eclipse.jetty.websocket.api.Session)
	at org.eclipse.jetty.websocket.common@12.0.5/org.eclipse.jetty.websocket.common.JettyWebSocketFrameHandlerFactory.toMethodHandle(JettyWebSocketFrameHandlerFactory.java:156)
	at org.eclipse.jetty.websocket.common@12.0.5/org.eclipse.jetty.websocket.common.JettyWebSocketFrameHandlerFactory.createListenerMetadata(JettyWebSocketFrameHandlerFactory.java:170)
	at org.eclipse.jetty.websocket.common@12.0.5/org.eclipse.jetty.websocket.common.JettyWebSocketFrameHandlerFactory.createMetadata(JettyWebSocketFrameHandlerFactory.java:129)
	at org.eclipse.jetty.websocket.common@12.0.5/org.eclipse.jetty.websocket.common.JettyWebSocketFrameHandlerFactory.getMetadata(JettyWebSocketFrameHandlerFactory.java:119)
	at org.eclipse.jetty.websocket.common@12.0.5/org.eclipse.jetty.websocket.common.JettyWebSocketFrameHandlerFactory.newJettyFrameHandler(JettyWebSocketFrameHandlerFactory.java:140)
	at org.eclipse.jetty.websocket.client@12.0.5/org.eclipse.jetty.websocket.client.internal.JettyClientUpgradeRequest.<init>(JettyClientUpgradeRequest.java:56)
	at org.eclipse.jetty.websocket.client@12.0.5/org.eclipse.jetty.websocket.client.WebSocketClient.connect(WebSocketClient.java:123)
	at org.eclipse.jetty.websocket.client@12.0.5/org.eclipse.jetty.websocket.client.WebSocketClient.connect(WebSocketClient.java:103)
	at org.eclipse.jetty.websocket.client@12.0.5/org.eclipse.jetty.websocket.client.WebSocketClient.connect(WebSocketClient.java:89)
	at JettyTest/org.example.TestWebSocketClient.main(TestWebSocketClient.java:28)
Caused by: java.lang.IllegalAccessException: access to public member failed: org.example.TestWebSocketEndPoint.onWebSocketOpen[Ljava.lang.Object;@19bb07ed/invokeVirtual, from class org.eclipse.jetty.websocket.common.JettyWebSocketFrameHandlerFactory (module org.eclipse.jetty.websocket.common)
	at java.base/java.lang.invoke.MemberName.makeAccessException(MemberName.java:894)
	at java.base/java.lang.invoke.MethodHandles$Lookup.checkAccess(MethodHandles.java:3987)
	at java.base/java.lang.invoke.MethodHandles$Lookup.checkMethod(MethodHandles.java:3923)
	at java.base/java.lang.invoke.MethodHandles$Lookup.getDirectMethodCommon(MethodHandles.java:4072)
	at java.base/java.lang.invoke.MethodHandles$Lookup.getDirectMethodNoSecurityManager(MethodHandles.java:4065)
	at java.base/java.lang.invoke.MethodHandles$Lookup.unreflect(MethodHandles.java:3451)
	at org.eclipse.jetty.websocket.common@12.0.5/org.eclipse.jetty.websocket.common.JettyWebSocketFrameHandlerFactory.toMethodHandle(JettyWebSocketFrameHandlerFactory.java:152)
	... 9 more

This error happens even when your endpoint class is in a package that is exported / opened to all modules in module-info.java:

module JettyTest {
    requires org.eclipse.jetty.websocket.client;
    requires org.eclipse.jetty.http2.client;
    requires org.eclipse.jetty.http2.client.transport;
    exports org.example;
    // exports org.example to org.eclipse.jetty.websocket.common; // also tested--does not work
    opens org.example;
    // opens org.example to org.eclipse.jetty.websocket.common; // also tested--does not work
}

Same error in Java 17.

Here is the dependency section my pom.xml:

    <dependencies>
        <dependency>
            <groupId>org.eclipse.jetty</groupId>
            <artifactId>jetty-client</artifactId>
            <version>12.0.5</version>
        </dependency>
        <dependency>
            <groupId>org.eclipse.jetty.http2</groupId>
            <artifactId>jetty-http2-client</artifactId>
            <version>12.0.5</version>
        </dependency>
        <dependency>
            <groupId>org.eclipse.jetty.http2</groupId>
            <artifactId>jetty-http2-client-transport</artifactId>
            <version>12.0.5</version>
        </dependency>
        <dependency>
            <groupId>org.eclipse.jetty.websocket</groupId>
            <artifactId>jetty-websocket-jetty-client</artifactId>
            <version>12.0.5</version>
        </dependency>
    </dependencies>

How to reproduce?
Minimal reproduction attached. This example code is a websocket client, but the error is identical when trying to start up a server for websockets using just the "core" libraries for Jetty 12. I'm hopeful that solving for one can lead to solving for both scenarios.

minimal_repro.zip

These are the two files:
TestWebSocketClient.java

package org.example;

import org.eclipse.jetty.client.HttpClient;
import org.eclipse.jetty.client.transport.HttpClientTransportDynamic;
import org.eclipse.jetty.http2.client.HTTP2Client;
import org.eclipse.jetty.http2.client.transport.ClientConnectionFactoryOverHTTP2;
import org.eclipse.jetty.websocket.api.Session;
import org.eclipse.jetty.websocket.client.WebSocketClient;

import java.net.URI;
import java.util.concurrent.CompletableFuture;

public class TestWebSocketClient {
    public static void main(String[] args) throws Exception {
        System.out.println("Java Runtime Version: " + Runtime.version());

        HTTP2Client http2Client = new HTTP2Client();
        HttpClient httpClient = new HttpClient(new HttpClientTransportDynamic(new ClientConnectionFactoryOverHTTP2.HTTP2(http2Client)));

        // Create and start WebSocketClient.
        WebSocketClient webSocketClient = new WebSocketClient(httpClient);
        webSocketClient.start();

        TestWebSocketEndPoint clientEndPoint = new TestWebSocketEndPoint();
        URI serverURI = URI.create("wss://localhost:5000/api/ws");

        // Connect the client EndPoint to the server.
        CompletableFuture<Session> clientSessionPromise = webSocketClient.connect(clientEndPoint, serverURI);
    }
}

TestWebSocketEndPoint.java

package org.example;

import org.eclipse.jetty.websocket.api.Callback;
import org.eclipse.jetty.websocket.api.Session;
import org.eclipse.jetty.websocket.api.annotations.WebSocket;

@WebSocket
public class TestWebSocketEndPoint implements Session.Listener {
    @Override
    public void onWebSocketOpen(Session session) {
        System.out.println("onWebSocketOpen");
        session.sendText("Hello!", Callback.NOOP);
    }

    @Override
    public void onWebSocketText(String message) {
        System.out.println("onWebSocketText: " + message);
    }

    @Override
    public void onWebSocketClose(int statusCode, String reason) {
        System.out.println("onWebSocketClose: " + statusCode + " -> " + reason);
    }
}

If you delete module-info.java in the minimal reproduction I've provided the example code works. In my large enterprise codebase I need to use modules and module security.

@unitydynamics unitydynamics added the Bug For general bugs on Jetty side label Dec 29, 2023
@sbordet
Copy link
Contributor

sbordet commented Jan 2, 2024

@unitydynamics you got the JPMS configuration the other way around.

You need to grant read to org.eclipse.jetty.websocket.common to your module, in the example JettyTest.
Jetty must be able to read your classes in order to call them.
Jetty cannot know what modules your classes will be in, so it can't add any configuration of this kind in its own module files.

At the same time, your module cannot tell to a Jetty module what it should read.

Therefore, you need to start the JVM with --add-reads org.eclipse.jetty.websocket.common=JettyTest.

Since your classes and methods are public, there is no need to open your module, you can remove that statement from your module-info.java.

We need to improve the documentation, so I opened #11213.

@sbordet sbordet added Question Documentation and removed Bug For general bugs on Jetty side labels Jan 2, 2024
@sbordet
Copy link
Contributor

sbordet commented Jan 2, 2024

FTR, this is clarified in JEP 261, section "Increasing readability".

@sbordet sbordet moved this to 🏗 In progress in Jetty 12.0.6 FROZEN Jan 2, 2024
@unitydynamics
Copy link
Author

unitydynamics commented Jan 3, 2024

Thank you @sbordet ! I've used Java since it's inception many decades ago and this is the first time I've run into a need for add-reads! I figured the proper exports and requires would be enough. I'm going to brush up on my jigsaw/module security knowledge! But I do wonder: our project is full of libraries that achieve similar objectives (e.g. dependency injection w/Guice) without an add-reads statement. As far as I can tell there aren't any anonymous modules involved...

In a neat an tidy world I would love to see all of our module security parameters in module-info.java!

@sbordet
Copy link
Contributor

sbordet commented Jan 3, 2024

But I do wonder: our project is full of libraries that achieve similar objectives (e.g. dependency injection w/Guice) without an add-reads statement.

I would be surprised if that works.
Guice will have the same problem: scanning classes looking for @Inject annotations, allocating instances, and invoking methods, so I would expect there is a need for a readability edge from Guice to your classes, which can only be added via command line.

@unitydynamics
Copy link
Author

unitydynamics commented Jan 3, 2024

minimal_repro2.zip
Surprise! Second minimal reproduction (with guice) attached. export in module-info.java is enough for Guice to do the first two things you mentioned (scanning classes for annotations and creating / allocating instances). Invoking methods might be the special case in which the "readability edge" is needed? I'm trying to scan through and see if we have any code where Jackson is invoking methods on de(serialization) (Jackson also doesn't need add-reads in our project).

Along a similar vein, it looks like Guice Aspect Oriented Programming uses a trick: dynamically creating a subclass of your class (and then calling the super implementation in that new subclass). Maybe Jetty could do this as well?

@unitydynamics
Copy link
Author

unitydynamics commented Jan 3, 2024

I was about to write up an example of Jackson (which does everything--including method calls); but I think I know why these other libraries are working without add-reads and Jetty is not. It seems that both Guice and Jackson don't actually include a module-info.class. Thus they become "automatic modules" when used in JPMS. A consequence of this: automatic modules automatically get "reads any module". This simple quirk may very well explain why all modern Java libraries that use reflection have not adopted modules by incorporating module-info.java

So yes, we can put --add-reads into our VM options. But it seems an oversight in JPMS.... why not have a reads wildcard option for module-info.java? So strange. The only other mentions / discussions about add-reads by those working on Java specifications seems to indicate that it's primary use is for testing libraries. Still not completely convinced I understand it all.

@sbordet
Copy link
Contributor

sbordet commented Jan 3, 2024

You may actually have hit a Jetty bug...

Looking more closely, we have no problems when using annotations, but we do when using Session.Listener.

It seems a problem of the MethodHandles.Lookup object we use.
Stay tuned, I'll post when I have more information.

@sbordet
Copy link
Contributor

sbordet commented Jan 3, 2024

All right, this is the way it works.

If you use Jetty WebSocket annotations, we use an "application" MethodHandles.Lookup because there is no link with any server class such as Session.Listener.
This "application" lookup will not require --add-reads.

However, if you use Session.Listener, then we use a "server" MethodHandles.Lookup.
Apparently this is only necessary to support anonymous WebSocket endpoint classes, which are non-public.
This "server" lookup requires --add-reads.

I think that if we drop support for anonymous WebSocket endpoint classes, then we could use the "application" lookup for Session.Listener and you won't need --add-reads.

@lachlan-roberts thoughts?

@unitydynamics
Copy link
Author

unitydynamics commented Jan 3, 2024

As a user/outside observer... I prefer the ability to implement the interface (rather than use the annotation). And I would like to avoid the --add-reads! Thank you for the research @sbordet !

@sbordet
Copy link
Contributor

sbordet commented Jan 3, 2024

Guess who asked to support anonymous classes?
#5043
🤦🏼

@sbordet sbordet added Bug For general bugs on Jetty side and removed Question Documentation labels Jan 7, 2024
sbordet added a commit that referenced this issue Jan 7, 2024
…t methods in Jetty 12

Changed `JettyWebSocketFrameHandlerFactory` to use an application MethodHandle.Lookup (rather than a server one) when creating listener metadata.
This fixes the JPMS problem so that now JPMS applications do not need any additional configuration to invoke endpoints.
The (acceptable) downside is that anonymous inner classes (that are not public) cannot be used as listener endpoints.

This change affects core and EE10 WebSocket; EE9 and EE8 WebSocket have not been fixed (so they allow anonymous inner classes but are broken in JPMS).

Renamed "connectLatch" fields to "openLatch" in test classes.

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
sbordet added a commit that referenced this issue Jan 25, 2024
…t methods in Jetty 12 (#11229)

Changed `JettyWebSocketFrameHandlerFactory` to use an application MethodHandle.Lookup (rather than a server one) when creating listener metadata.
This fixes the JPMS problem so that now JPMS applications do not need any additional configuration to invoke endpoints.
The (acceptable) downside is that anonymous inner classes (that are not public) cannot be used as listener endpoints.

This change affects core and EE10 WebSocket; EE9 and EE8 WebSocket have not been fixed (so they allow anonymous inner classes but are broken in JPMS).

Renamed "connectLatch" fields to "openLatch" in test classes.

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
Signed-off-by: Olivier Lamy <olamy@apache.org>
Co-authored-by: Olivier Lamy <olamy@apache.org>
@github-project-automation github-project-automation bot moved this from 🏗 In progress to ✅ Done in Jetty 12.0.6 FROZEN Jan 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug For general bugs on Jetty side
Projects
No open projects
Status: ✅ Done
2 participants