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.

Sign up for GitHub

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

Issue #4691 - use static methods with javadoc to get MethodHandle lookups #4694

Merged
merged 2 commits into from
Mar 24, 2020
Merged

Issue #4691 - use static methods with javadoc to get MethodHandle lookups #4694

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
04cc21f
Issue #4691 - use static methods with javadoc to get MethodHandle lookup
lachlan-roberts Mar 23, 2020
c0b6d9f
Issue #4691 - changes from review
lachlan-roberts Mar 24, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions ...on/src/main/java/org/eclipse/jetty/websocket/javax/common/JavaxWebSocketFrameHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -374,7 +374,7 @@ public <T> void addMessageHandler(JavaxWebSocketSession session, Class<T> clazz,
try
{
// TODO: move methodhandle lookup to container?
MethodHandles.Lookup lookup = MethodHandles.publicLookup();
MethodHandles.Lookup lookup = JavaxWebSocketFrameHandlerFactory.getServerMethodHandleLookup();
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you still need the TODO above?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we can remove it, I don't know why we would want to do this in the container, it seems better to contain the MethodHandle stuff in the FrameHandler.

MethodHandle partialMessageHandler = lookup
.findVirtual(MessageHandler.Partial.class, "onMessage", MethodType.methodType(void.class, Object.class, boolean.class));
partialMessageHandler = partialMessageHandler.bindTo(handler);
Expand Down Expand Up @@ -432,7 +432,7 @@ public <T> void addMessageHandler(JavaxWebSocketSession session, Class<T> clazz,
try
{
// TODO: move MethodHandle lookup to container?
MethodHandles.Lookup lookup = MethodHandles.publicLookup();
MethodHandles.Lookup lookup = JavaxWebSocketFrameHandlerFactory.getServerMethodHandleLookup();
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto.

MethodHandle wholeMsgMethodHandle = lookup.findVirtual(MessageHandler.Whole.class, "onMessage", MethodType.methodType(void.class, Object.class));
wholeMsgMethodHandle = wholeMsgMethodHandle.bindTo(handler);

Expand Down
59 changes: 52 additions & 7 deletions ...main/java/org/eclipse/jetty/websocket/javax/common/JavaxWebSocketFrameHandlerFactory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,7 +124,7 @@ public abstract class JavaxWebSocketFrameHandlerFactory
{
try
{
FILTER_RETURN_TYPE_METHOD = MethodHandles.lookup()
FILTER_RETURN_TYPE_METHOD = getServerMethodHandleLookup()
.findVirtual(JavaxWebSocketSession.class, "filterReturnType", MethodType.methodType(void.class, Object.class));
}
catch (Throwable e)
Expand Down Expand Up @@ -322,16 +322,17 @@ public static MessageSink createMessageSink(JavaxWebSocketSession session, Messa

try
{
MethodHandles.Lookup lookup = getServerMethodHandleLookup();
if (DecodedMessageSink.class.isAssignableFrom(msgMetadata.sinkClass))
{
MethodHandle ctorHandle = MethodHandles.lookup().findConstructor(msgMetadata.sinkClass,
MethodHandle ctorHandle = lookup.findConstructor(msgMetadata.sinkClass,
MethodType.methodType(void.class, CoreSession.class, msgMetadata.registeredDecoder.interfaceType, MethodHandle.class));
Decoder decoder = session.getDecoders().getInstanceOf(msgMetadata.registeredDecoder);
return (MessageSink)ctorHandle.invoke(session.getCoreSession(), decoder, msgMetadata.handle);
}
else
{
MethodHandle ctorHandle = MethodHandles.lookup().findConstructor(msgMetadata.sinkClass,
MethodHandle ctorHandle = lookup.findConstructor(msgMetadata.sinkClass,
MethodType.methodType(void.class, CoreSession.class, MethodHandle.class));
return (MessageSink)ctorHandle.invoke(session.getCoreSession(), msgMetadata.handle);
}
Expand Down Expand Up @@ -388,7 +389,7 @@ private MethodHandle toMethodHandle(MethodHandles.Lookup lookup, Method method)
protected JavaxWebSocketFrameHandlerMetadata createEndpointMetadata(Class<? extends javax.websocket.Endpoint> endpointClass, EndpointConfig endpointConfig)
{
JavaxWebSocketFrameHandlerMetadata metadata = new JavaxWebSocketFrameHandlerMetadata(endpointConfig);
MethodHandles.Lookup lookup = getMethodHandleLookup(endpointClass);
MethodHandles.Lookup lookup = getApplicationMethodHandleLookup(endpointClass);

Method openMethod = ReflectUtils.findMethod(endpointClass, "onOpen",
javax.websocket.Session.class, javax.websocket.EndpointConfig.class);
Expand All @@ -410,7 +411,7 @@ protected JavaxWebSocketFrameHandlerMetadata createEndpointMetadata(Class<? exte

protected JavaxWebSocketFrameHandlerMetadata discoverJavaxFrameHandlerMetadata(Class<?> endpointClass, JavaxWebSocketFrameHandlerMetadata metadata)
{
MethodHandles.Lookup lookup = getMethodHandleLookup(endpointClass);
MethodHandles.Lookup lookup = getApplicationMethodHandleLookup(endpointClass);
Method onmethod;

// OnOpen [0..1]
Expand Down Expand Up @@ -704,9 +705,53 @@ private void assertSignatureValid(Class<?> endpointClass, Method method, Class<?
}
}

private MethodHandles.Lookup getMethodHandleLookup(Class<?> endpointClass)
/**
* <p>
* Gives a {@link MethodHandles.Lookup} instance to be used to find methods in server classes.
* For lookups on application classes use {@link #getApplicationMethodHandleLookup(Class)} ()} instead.
Copy link
Contributor

Choose a reason for hiding this comment

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

There is an additional ()} in the javadoc.

* </p>
* <p>
* This uses the caller sensitive {@link MethodHandles#lookup()}, this will allow MethodHandle access
* to server classes we need to use and will give access permissions to private methods as well.
* </p>
*
* @return
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing description of what it returns.

*/
public static MethodHandles.Lookup getServerMethodHandleLookup()
{
return MethodHandles.lookup();
}

/**
* <p>
* Gives a {@link MethodHandles.Lookup} instance to be used to find public methods in application classes.
* For lookups on server classes use {@link #getServerMethodHandleLookup()} instead.
* </p>
* <p>
* This uses {@link MethodHandles#publicLookup()} as we only need access to public method of the lookupClass.
* To look up a method on the lookupClass, it must be public and the class must be accessible from this
* module, so if the lookupClass is in a JPMS module it must be exported so that the public methods
* of the lookupClass are accessible outside of the module.
* </p>
* <p>
* The {@link java.lang.invoke.MethodHandles.Lookup#in(Class)} allows us to search specifically
* in the endpoint Class to avoid any potential linkage errors which could occur if the same
* class is present in multiple web apps. Unlike using {@link MethodHandles#publicLookup()}
* using {@link MethodHandles#lookup()} with {@link java.lang.invoke.MethodHandles.Lookup#in(Class)}
* will cause the lookup to lose its public access to the lookup class if they are in different modules.
* </p>
* <p>
* {@link MethodHandles#privateLookupIn(Class, MethodHandles.Lookup)} is also unsuitable because it
* requires the caller module to read the target module, and the target module to open reflective
* access to the lookupClasses private methods. This is possible but requires extra configuration
* to provide private access which is not necessary for the purpose of accessing the public methods.
* </p>
* @param lookupClass the desired lookup class for the new lookup object.
* @return a lookup object to be used to find methods on the lookupClass.
*/
public static MethodHandles.Lookup getApplicationMethodHandleLookup(Class<?> lookupClass)
{
return MethodHandles.publicLookup().in(endpointClass);
return MethodHandles.publicLookup().in(lookupClass);
}

private static class DecodedArgs
Expand Down
4 changes: 2 additions & 2 deletions ...main/java/org/eclipse/jetty/websocket/javax/common/messages/DecodedBinaryMessageSink.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@
package org.eclipse.jetty.websocket.javax.common.messages;

import java.lang.invoke.MethodHandle;
import java.lang.invoke.MethodHandles;
import java.lang.invoke.MethodType;
import java.nio.ByteBuffer;
import javax.websocket.CloseReason;
Expand All @@ -28,6 +27,7 @@

import org.eclipse.jetty.websocket.core.CoreSession;
import org.eclipse.jetty.websocket.core.exception.CloseException;
import org.eclipse.jetty.websocket.javax.common.JavaxWebSocketFrameHandlerFactory;
import org.eclipse.jetty.websocket.util.messages.ByteBufferMessageSink;
import org.eclipse.jetty.websocket.util.messages.MessageSink;

Expand All @@ -44,7 +44,7 @@ public DecodedBinaryMessageSink(CoreSession session,
@Override
protected MethodHandle newRawMethodHandle() throws NoSuchMethodException, IllegalAccessException
{
return MethodHandles.lookup().findVirtual(DecodedBinaryMessageSink.class,
return JavaxWebSocketFrameHandlerFactory.getServerMethodHandleLookup().findVirtual(DecodedBinaryMessageSink.class,
"onWholeMessage", MethodType.methodType(void.class, ByteBuffer.class))
.bindTo(this);
}
Expand Down
4 changes: 2 additions & 2 deletions ...ava/org/eclipse/jetty/websocket/javax/common/messages/DecodedBinaryStreamMessageSink.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,14 +20,14 @@

import java.io.InputStream;
import java.lang.invoke.MethodHandle;
import java.lang.invoke.MethodHandles;
import java.lang.invoke.MethodType;
import javax.websocket.CloseReason;
import javax.websocket.DecodeException;
import javax.websocket.Decoder;

import org.eclipse.jetty.websocket.core.CoreSession;
import org.eclipse.jetty.websocket.core.exception.CloseException;
import org.eclipse.jetty.websocket.javax.common.JavaxWebSocketFrameHandlerFactory;
import org.eclipse.jetty.websocket.util.messages.InputStreamMessageSink;
import org.eclipse.jetty.websocket.util.messages.MessageSink;

Expand All @@ -44,7 +44,7 @@ public DecodedBinaryStreamMessageSink(CoreSession session,
@Override
protected MethodHandle newRawMethodHandle() throws NoSuchMethodException, IllegalAccessException
{
return MethodHandles.lookup().findVirtual(DecodedBinaryStreamMessageSink.class,
return JavaxWebSocketFrameHandlerFactory.getServerMethodHandleLookup().findVirtual(DecodedBinaryStreamMessageSink.class,
"onStreamStart", MethodType.methodType(void.class, InputStream.class))
.bindTo(this);
}
Expand Down
4 changes: 2 additions & 2 deletions ...c/main/java/org/eclipse/jetty/websocket/javax/common/messages/DecodedTextMessageSink.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,14 +19,14 @@
package org.eclipse.jetty.websocket.javax.common.messages;

import java.lang.invoke.MethodHandle;
import java.lang.invoke.MethodHandles;
import java.lang.invoke.MethodType;
import javax.websocket.CloseReason;
import javax.websocket.DecodeException;
import javax.websocket.Decoder;

import org.eclipse.jetty.websocket.core.CoreSession;
import org.eclipse.jetty.websocket.core.exception.CloseException;
import org.eclipse.jetty.websocket.javax.common.JavaxWebSocketFrameHandlerFactory;
import org.eclipse.jetty.websocket.util.messages.MessageSink;
import org.eclipse.jetty.websocket.util.messages.StringMessageSink;

Expand All @@ -43,7 +43,7 @@ public DecodedTextMessageSink(CoreSession session,
@Override
protected MethodHandle newRawMethodHandle() throws NoSuchMethodException, IllegalAccessException
{
return MethodHandles.lookup().findVirtual(DecodedTextMessageSink.class,
return JavaxWebSocketFrameHandlerFactory.getServerMethodHandleLookup().findVirtual(DecodedTextMessageSink.class,
"onWholeMessage", MethodType.methodType(void.class, String.class))
.bindTo(this);
}
Expand Down
4 changes: 2 additions & 2 deletions .../java/org/eclipse/jetty/websocket/javax/common/messages/DecodedTextStreamMessageSink.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,14 +20,14 @@

import java.io.Reader;
import java.lang.invoke.MethodHandle;
import java.lang.invoke.MethodHandles;
import java.lang.invoke.MethodType;
import javax.websocket.CloseReason;
import javax.websocket.DecodeException;
import javax.websocket.Decoder;

import org.eclipse.jetty.websocket.core.CoreSession;
import org.eclipse.jetty.websocket.core.exception.CloseException;
import org.eclipse.jetty.websocket.javax.common.JavaxWebSocketFrameHandlerFactory;
import org.eclipse.jetty.websocket.util.messages.MessageSink;
import org.eclipse.jetty.websocket.util.messages.ReaderMessageSink;

Expand All @@ -44,7 +44,7 @@ public DecodedTextStreamMessageSink(CoreSession session,
@Override
protected MethodHandle newRawMethodHandle() throws NoSuchMethodException, IllegalAccessException
{
return MethodHandles.lookup().findVirtual(DecodedTextStreamMessageSink.class,
return JavaxWebSocketFrameHandlerFactory.getServerMethodHandleLookup().findVirtual(DecodedTextStreamMessageSink.class,
"onStreamStart", MethodType.methodType(void.class, Reader.class))
.bindTo(this);
}
Expand Down
4 changes: 2 additions & 2 deletions .../test/java/org/eclipse/jetty/websocket/javax/common/messages/AbstractMessageSinkTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,11 +19,11 @@
package org.eclipse.jetty.websocket.javax.common.messages;

import java.lang.invoke.MethodHandle;
import java.lang.invoke.MethodHandles;
import java.lang.invoke.MethodType;
import java.util.function.Consumer;

import org.eclipse.jetty.websocket.javax.common.AbstractSessionTest;
import org.eclipse.jetty.websocket.javax.common.JavaxWebSocketFrameHandlerFactory;

public abstract class AbstractMessageSinkTest extends AbstractSessionTest
{
Expand All @@ -34,7 +34,7 @@ public <T> MethodHandle getAcceptHandle(Consumer<T> copy, Class<T> type)
Class<?> refc = copy.getClass();
String name = "accept";
MethodType methodType = MethodType.methodType(void.class, type);
MethodHandle handle = MethodHandles.lookup().findVirtual(refc, name, methodType);
MethodHandle handle = JavaxWebSocketFrameHandlerFactory.getServerMethodHandleLookup().findVirtual(refc, name, methodType);
return handle.bindTo(copy);
}
catch (NoSuchMethodException | IllegalAccessException e)
Expand Down
4 changes: 3 additions & 1 deletion .../src/main/java/org/eclipse/jetty/websocket/javax/tests/CompletableFutureMethodHandle.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
import java.lang.reflect.Method;
import java.util.concurrent.CompletableFuture;

import org.eclipse.jetty.websocket.javax.common.JavaxWebSocketFrameHandlerFactory;
import org.eclipse.jetty.websocket.util.InvokerUtils;
import org.eclipse.jetty.websocket.util.ReflectUtils;

Expand All @@ -31,7 +32,8 @@ public class CompletableFutureMethodHandle
public static <T> MethodHandle of(Class<T> type, CompletableFuture<T> future)
{
Method method = ReflectUtils.findMethod(CompletableFuture.class, "complete", type);
MethodHandle completeHandle = InvokerUtils.mutatedInvoker(MethodHandles.lookup(), CompletableFuture.class, method, new InvokerUtils.Arg(type));
MethodHandles.Lookup lookup = JavaxWebSocketFrameHandlerFactory.getServerMethodHandleLookup();
MethodHandle completeHandle = InvokerUtils.mutatedInvoker(lookup, CompletableFuture.class, method, new InvokerUtils.Arg(type));
return completeHandle.bindTo(future);
}
}
55 changes: 50 additions & 5 deletions ...n/src/main/java/org/eclipse/jetty/websocket/common/JettyWebSocketFrameHandlerFactory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,7 +157,8 @@ public static MessageSink createMessageSink(MethodHandle msgHandle, Class<? exte

try
{
MethodHandle ctorHandle = MethodHandles.lookup().findConstructor(sinkClass,
MethodHandles.Lookup lookup = JettyWebSocketFrameHandlerFactory.getServerMethodHandleLookup();
MethodHandle ctorHandle = lookup.findConstructor(sinkClass,
MethodType.methodType(void.class, CoreSession.class, MethodHandle.class));
return (MessageSink)ctorHandle.invoke(session.getCoreSession(), msgHandle);
}
Expand Down Expand Up @@ -194,7 +195,7 @@ private MethodHandle toMethodHandle(MethodHandles.Lookup lookup, Method method)
private JettyWebSocketFrameHandlerMetadata createListenerMetadata(Class<?> endpointClass)
{
JettyWebSocketFrameHandlerMetadata metadata = new JettyWebSocketFrameHandlerMetadata();
MethodHandles.Lookup lookup = getMethodHandleLookup(endpointClass);
MethodHandles.Lookup lookup = JettyWebSocketFrameHandlerFactory.getApplicationMethodHandleLookup(endpointClass);

Method openMethod = ReflectUtils.findMethod(endpointClass, "onWebSocketConnect", Session.class);
MethodHandle open = toMethodHandle(lookup, openMethod);
Expand Down Expand Up @@ -273,7 +274,7 @@ private JettyWebSocketFrameHandlerMetadata createAnnotatedMetadata(WebSocket ann
metadata.setIdleTimeout(Duration.ofMillis(max));
metadata.setBatchMode(anno.batchMode());

MethodHandles.Lookup lookup = getMethodHandleLookup(endpointClass);
MethodHandles.Lookup lookup = getApplicationMethodHandleLookup(endpointClass);
Method onmethod;

// OnWebSocketConnect [0..1]
Expand Down Expand Up @@ -456,9 +457,53 @@ private void assertSignatureValid(Class<?> endpointClass, Method method, Class<?
throw new InvalidSignatureException(err.toString());
}

private MethodHandles.Lookup getMethodHandleLookup(Class<?> endpointClass)
/**
* <p>
* Gives a {@link MethodHandles.Lookup} instance to be used to find methods in server classes.
* For lookups on application classes use {@link #getApplicationMethodHandleLookup(Class)} ()} instead.
* </p>
* <p>
* This uses the caller sensitive {@link MethodHandles#lookup()}, this will allow MethodHandle access
* to server classes we need to use and will give access permissions to private methods as well.
* </p>
*
* @return
Copy link
Contributor

Choose a reason for hiding this comment

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

To avoid duplicating these javadocs, I would rather @see JavaxWebSocketFrameHandlerFactory#getServerMethodHandleLookup().

So that if we improve the javadocs, we only do it in one place.

Check that we can actually do this @see because it will reference a class in a different module that is not dependent on this one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This can't be done, I think they will have to be duplicated.
Symbol 'org.eclipse.jetty.websocket.javax.common.JavaxWebSocketFrameHandlerFactory' is inaccessible from here.

*/
public static MethodHandles.Lookup getServerMethodHandleLookup()
{
return MethodHandles.publicLookup().in(endpointClass);
return MethodHandles.lookup();
}

/**
* <p>
* Gives a {@link MethodHandles.Lookup} instance to be used to find public methods in application classes.
* For lookups on server classes use {@link #getServerMethodHandleLookup()} instead.
* </p>
* <p>
* This uses {@link MethodHandles#publicLookup()} as we only need access to public method of the lookupClass.
* To look up a method on the lookupClass, it must be public and the class must be accessible from this
* module, so if the lookupClass is in a JPMS module it must be exported so that the public methods
* of the lookupClass are accessible outside of the module.
* </p>
* <p>
* The {@link java.lang.invoke.MethodHandles.Lookup#in(Class)} allows us to search specifically
* in the endpoint Class to avoid any potential linkage errors which could occur if the same
* class is present in multiple web apps. Unlike using {@link MethodHandles#publicLookup()}
* using {@link MethodHandles#lookup()} with {@link java.lang.invoke.MethodHandles.Lookup#in(Class)}
* will cause the lookup to lose its public access to the lookup class if they are in different modules.
* </p>
* <p>
* {@link MethodHandles#privateLookupIn(Class, MethodHandles.Lookup)} is also unsuitable because it
* requires the caller module to read the target module, and the target module to open reflective
* access to the lookupClasses private methods. This is possible but requires extra configuration
* to provide private access which is not necessary for the purpose of accessing the public methods.
* </p>
* @param lookupClass the desired lookup class for the new lookup object.
* @return a lookup object to be used to find methods on the lookupClass.
*/
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto.

public static MethodHandles.Lookup getApplicationMethodHandleLookup(Class<?> lookupClass)
{
return MethodHandles.publicLookup().in(lookupClass);
}

@Override
Expand Down
2 changes: 1 addition & 1 deletion ...jetty-common/src/test/java/org/eclipse/jetty/websocket/common/OutgoingMessageCapture.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ public class OutgoingMessageCapture extends CoreSession.Empty implements CoreSes

public OutgoingMessageCapture()
{
MethodHandles.Lookup lookup = MethodHandles.lookup();
MethodHandles.Lookup lookup = JettyWebSocketFrameHandlerFactory.getApplicationMethodHandleLookup(this.getClass());
try
{
MethodHandle text = lookup.findVirtual(this.getClass(), "onWholeText", MethodType.methodType(Void.TYPE, String.class));
Expand Down