Issue #6328 - changes from review

Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
jetty · Jun 6, 2022 · 1dd20fd · 1dd20fd
1 parent 49adfe5
commit 1dd20fd
Show file tree

Hide file tree

Showing 14 changed files with 68 additions and 140 deletions.
diff --git a/...src/main/java/org/eclipse/jetty/websocket/core/internal/messages/AbstractMessageSink.java b/...src/main/java/org/eclipse/jetty/websocket/core/internal/messages/AbstractMessageSink.java
@@ -26,6 +26,6 @@ public abstract class AbstractMessageSink implements MessageSink
     public AbstractMessageSink(CoreSession session, MethodHolder methodHolder)
     {
         this.session = Objects.requireNonNull(session, "CoreSession");
-        this.methodHolder = Objects.requireNonNull(methodHolder, "MethodHandle");
+        this.methodHolder = Objects.requireNonNull(methodHolder, "MethodHolder");
     }
 }
diff --git a/.../main/java/org/eclipse/jetty/websocket/core/internal/messages/InputStreamMessageSink.java b/.../main/java/org/eclipse/jetty/websocket/core/internal/messages/InputStreamMessageSink.java
@@ -13,8 +13,6 @@
 
 package org.eclipse.jetty.websocket.core.internal.messages;
 
-import java.lang.invoke.MethodHandle;
-
 import org.eclipse.jetty.websocket.core.CoreSession;
 import org.eclipse.jetty.websocket.core.Frame;
 import org.eclipse.jetty.websocket.core.internal.util.MethodHolder;
@@ -26,12 +24,6 @@ public InputStreamMessageSink(CoreSession session, MethodHolder methodHolder)
         super(session, methodHolder);
     }
 
-    @Deprecated
-    public InputStreamMessageSink(CoreSession session, MethodHandle methodHandle)
-    {
-        this(session, MethodHolder.from(methodHandle));
-    }
-
     @Override
     public MessageSink newSink(Frame frame)
     {

diff --git a/...n/src/main/java/org/eclipse/jetty/websocket/core/internal/messages/ReaderMessageSink.java b/...n/src/main/java/org/eclipse/jetty/websocket/core/internal/messages/ReaderMessageSink.java
@@ -13,8 +13,6 @@
 
 package org.eclipse.jetty.websocket.core.internal.messages;
 
-import java.lang.invoke.MethodHandle;
-
 import org.eclipse.jetty.websocket.core.CoreSession;
 import org.eclipse.jetty.websocket.core.Frame;
 import org.eclipse.jetty.websocket.core.internal.util.MethodHolder;
@@ -26,12 +24,6 @@ public ReaderMessageSink(CoreSession session, MethodHolder methodHolder)
         super(session, methodHolder);
     }
 
-    @Deprecated
-    public ReaderMessageSink(CoreSession session, MethodHandle methodHandle)
-    {
-        this(session, MethodHolder.from(methodHandle));
-    }
-
     @Override
     public MessageReader newSink(Frame frame)
     {

diff --git a/...n/src/main/java/org/eclipse/jetty/websocket/core/internal/messages/StringMessageSink.java b/...n/src/main/java/org/eclipse/jetty/websocket/core/internal/messages/StringMessageSink.java
@@ -13,8 +13,6 @@
 
 package org.eclipse.jetty.websocket.core.internal.messages;
 
-import java.lang.invoke.MethodHandle;
-
 import org.eclipse.jetty.util.Callback;
 import org.eclipse.jetty.util.Utf8StringBuilder;
 import org.eclipse.jetty.websocket.core.CoreSession;
@@ -33,12 +31,6 @@ public StringMessageSink(CoreSession session, MethodHolder methodHolder)
         this.size = 0;
     }
 
-    @Deprecated
-    public StringMessageSink(CoreSession session, MethodHandle methodHandle)
-    {
-        this(session, MethodHolder.from(methodHandle));
-    }
-
     @Override
     public void accept(Frame frame, Callback callback)
     {

diff --git a/...mon/src/main/java/org/eclipse/jetty/websocket/core/internal/util/BindingMethodHolder.java b/...mon/src/main/java/org/eclipse/jetty/websocket/core/internal/util/BindingMethodHolder.java
@@ -52,13 +52,6 @@ public BindingMethodHolder filterReturnValue(MethodHandle filter)
         return this;
     }
 
-    @Override
-    public BindingMethodHolder changeReturnType(Class<Object> objectClass)
-    {
-        _methodHandle = _methodHandle.asType(_methodHandle.type().changeReturnType(objectClass));
-        return this;
-    }
-
     @Override
     public Class<?> parameterType(int idx)
     {

diff --git a/...ore-common/src/main/java/org/eclipse/jetty/websocket/core/internal/util/MethodHolder.java b/...ore-common/src/main/java/org/eclipse/jetty/websocket/core/internal/util/MethodHolder.java
@@ -19,17 +19,25 @@
  * An interface for managing invocations of methods whose arguments may need to be augmented, by
  * binding in certain parameters ahead of time.
  *
- * This differs from {@link MethodHandle}s that any calls the methods such as {@link #bindTo(Object)}
- * will change the instance of {@link MethodHolder} it is called on.
+ * Implementations may use various invocation mechanisms, including:
+ * <ul>
+ *  <li>direct method invocation on an held object</li>
+ *  <li>calling a method pointer</li>
+ *  <li>calling a MethodHandle bound to the known arguments</li>
+ *  <li>calling a MethodHandle without binding to the known arguments</li>
+ * </ul>
  *
  * Implementations of this may not be thread safe, so the caller must use some external mutual exclusion
  * unless they are using a specific implementation known to be thread-safe.
  */
 public interface MethodHolder
 {
+    String METHOD_HOLDER_BINDING_PROPERTY = "jetty.methodholder.binding";
+
     static MethodHolder from(MethodHandle methodHandle)
     {
-        return from(methodHandle, false);
+        String property = System.getProperty(METHOD_HOLDER_BINDING_PROPERTY);
+        return from(methodHandle, Boolean.parseBoolean(property));
     }
 
     static MethodHolder from(MethodHandle methodHandle, boolean binding)
@@ -56,11 +64,6 @@ default MethodHolder filterReturnValue(MethodHandle filter)
         throw new UnsupportedOperationException();
     }
 
-    default MethodHolder changeReturnType(Class<Object> objectClass)
-    {
-        throw new UnsupportedOperationException();
-    }
-
     default Class<?> parameterType(int idx)
     {
         throw new UnsupportedOperationException();

diff --git a/.../src/main/java/org/eclipse/jetty/websocket/core/internal/util/NonBindingMethodHolder.java b/.../src/main/java/org/eclipse/jetty/websocket/core/internal/util/NonBindingMethodHolder.java
@@ -15,6 +15,8 @@
 
 import java.lang.invoke.MethodHandle;
 import java.lang.invoke.WrongMethodTypeException;
+import java.util.ArrayList;
+import java.util.List;
 import java.util.Objects;
 
 /**
@@ -26,9 +28,9 @@ class NonBindingMethodHolder implements MethodHolder
     private final MethodHandle _methodHandle;
     private final Object[] _parameters;
     private final Boolean[] _boundParams;
+    // private final int[] _lookups;
     private final int _numParams;
-    private Class<Object> _returnType;
-    private MethodHandle _returnFilter;
+    private List<MethodHandle> _returnFilters;
 
     public NonBindingMethodHolder(MethodHandle methodHandle)
     {
@@ -57,11 +59,23 @@ private int getInternalIndex(int idx)
     @Override
     public Object invoke(Object... args) throws Throwable
     {
-        insertArguments(false, 0, args);
-        Object o = _methodHandle.invokeWithArguments(_parameters);
-        if (_returnFilter != null)
-            o = _returnFilter.invoke(o);
-        return (_returnType == null) ? o : _returnType.cast(o);
+        try
+        {
+            insertArguments(false, 0, args);
+            Object o = _methodHandle.invokeWithArguments(_parameters);
+            if (_returnFilters != null)
+            {
+                for (MethodHandle filter : _returnFilters)
+                {
+                    o = filter.invoke(o);
+                }
+            }
+            return o;
+        }
+        finally
+        {
+            clearArguments();
+        }
     }
 
     @Override
@@ -82,6 +96,7 @@ public MethodHolder bindTo(Object arg)
     @SuppressWarnings({"UnusedReturnValue", "SameParameterValue"})
     private MethodHolder insertArguments(boolean bind, int idx, Object... args)
     {
+        // TODO: try this with lookup table.
         int index = 0;
         int argsIndex = 0;
         for (int i = 0; i < _numParams; i++)
@@ -109,17 +124,23 @@ private MethodHolder insertArguments(boolean bind, int idx, Object... args)
         return this;
     }
 
-    @Override
-    public MethodHolder filterReturnValue(MethodHandle filter)
+    private void clearArguments()
     {
-        _returnFilter = filter;
-        return this;
+        for (int i = 0; i < _numParams; i++)
+        {
+            if (_boundParams[i] != Boolean.TRUE)
+            {
+                _parameters[i] = null;
+            }
+        }
     }
 
     @Override
-    public MethodHolder changeReturnType(Class<Object> objectClass)
+    public MethodHolder filterReturnValue(MethodHandle filter)
     {
-        _returnType = objectClass;
+        if (_returnFilters == null)
+            _returnFilters = new ArrayList<>();
+        _returnFilters.add(filter);
         return this;
     }
 
@@ -132,6 +153,6 @@ public Class<?> parameterType(int idx)
     @Override
     public Class<?> returnType()
     {
-        return _returnType;
+        return ((_returnFilters == null) ? _methodHandle : _returnFilters.get(_returnFilters.size() - 1)).type().returnType();
     }
 }
diff --git a/...core-tests/src/test/java/org/eclipse/jetty/websocket/core/util/StringMessageSinkTest.java b/...core-tests/src/test/java/org/eclipse/jetty/websocket/core/util/StringMessageSinkTest.java
@@ -28,6 +28,7 @@
 import org.eclipse.jetty.websocket.core.OpCode;
 import org.eclipse.jetty.websocket.core.exception.MessageTooLargeException;
 import org.eclipse.jetty.websocket.core.internal.messages.StringMessageSink;
+import org.eclipse.jetty.websocket.core.internal.util.MethodHolder;
 import org.junit.jupiter.api.Test;
 
 import static org.hamcrest.MatcherAssert.assertThat;
@@ -44,7 +45,7 @@ public class StringMessageSinkTest
     @Test
     public void testMaxMessageSize() throws Exception
     {
-        StringMessageSink messageSink = new StringMessageSink(coreSession, endpoint.getMethodHandle());
+        StringMessageSink messageSink = new StringMessageSink(coreSession, MethodHolder.from(endpoint.getMethodHandle()));
         ByteBuffer utf8Payload = BufferUtil.toBuffer(new byte[]{(byte)0xF0, (byte)0x90, (byte)0x8D, (byte)0x88});
 
         FutureCallback callback = new FutureCallback();
@@ -60,7 +61,7 @@ public void testMaxMessageSize() throws Exception
     @Test
     public void testValidUtf8() throws Exception
     {
-        StringMessageSink messageSink = new StringMessageSink(coreSession, endpoint.getMethodHandle());
+        StringMessageSink messageSink = new StringMessageSink(coreSession, MethodHolder.from(endpoint.getMethodHandle()));
         ByteBuffer utf8Payload = BufferUtil.toBuffer(new byte[]{(byte)0xF0, (byte)0x90, (byte)0x8D, (byte)0x88});
 
         FutureCallback callback = new FutureCallback();
@@ -73,7 +74,7 @@ public void testValidUtf8() throws Exception
     @Test
     public void testUtf8Continuation() throws Exception
     {
-        StringMessageSink messageSink = new StringMessageSink(coreSession, endpoint.getMethodHandle());
+        StringMessageSink messageSink = new StringMessageSink(coreSession, MethodHolder.from(endpoint.getMethodHandle()));
         ByteBuffer firstUtf8Payload = BufferUtil.toBuffer(new byte[]{(byte)0xF0, (byte)0x90});
         ByteBuffer continuationUtf8Payload = BufferUtil.toBuffer(new byte[]{(byte)0x8D, (byte)0x88});
 
@@ -91,7 +92,7 @@ public void testUtf8Continuation() throws Exception
     @Test
     public void testInvalidSingleFrameUtf8() throws Exception
     {
-        StringMessageSink messageSink = new StringMessageSink(coreSession, endpoint.getMethodHandle());
+        StringMessageSink messageSink = new StringMessageSink(coreSession, MethodHolder.from(endpoint.getMethodHandle()));
         ByteBuffer invalidUtf8Payload = BufferUtil.toBuffer(new byte[]{(byte)0xF0, (byte)0x90, (byte)0x8D});
 
         FutureCallback callback = new FutureCallback();
@@ -106,7 +107,7 @@ public void testInvalidSingleFrameUtf8() throws Exception
     @Test
     public void testInvalidMultiFrameUtf8() throws Exception
     {
-        StringMessageSink messageSink = new StringMessageSink(coreSession, endpoint.getMethodHandle());
+        StringMessageSink messageSink = new StringMessageSink(coreSession, MethodHolder.from(endpoint.getMethodHandle()));
         ByteBuffer firstUtf8Payload = BufferUtil.toBuffer(new byte[]{(byte)0xF0, (byte)0x90});
         ByteBuffer continuationUtf8Payload = BufferUtil.toBuffer(new byte[]{(byte)0x8D});
 

diff --git a/...c/main/java/org/eclipse/jetty/websocket/javax/common/JavaxMessagePartialMethodHolder.java b/...c/main/java/org/eclipse/jetty/websocket/javax/common/JavaxMessagePartialMethodHolder.java
@@ -13,7 +13,6 @@
 
 package org.eclipse.jetty.websocket.javax.common;
 
-import java.lang.invoke.MethodHandle;
 import java.lang.invoke.WrongMethodTypeException;
 import javax.websocket.MessageHandler;
 
@@ -38,30 +37,6 @@ public Object invoke(Object... args) throws Throwable
         return null;
     }
 
-    @Override
-    public MethodHolder bindTo(Object arg)
-    {
-        throw new UnsupportedOperationException();
-    }
-
-    @Override
-    public MethodHolder bindTo(Object arg, int idx)
-    {
-        throw new UnsupportedOperationException();
-    }
-
-    @Override
-    public MethodHolder filterReturnValue(MethodHandle filter)
-    {
-        throw new UnsupportedOperationException();
-    }
-
-    @Override
-    public MethodHolder changeReturnType(Class<Object> objectClass)
-    {
-        throw new UnsupportedOperationException();
-    }
-
     @Override
     public Class<?> parameterType(int idx)
     {

diff --git a/...src/main/java/org/eclipse/jetty/websocket/javax/common/JavaxMessageWholeMethodHolder.java b/...src/main/java/org/eclipse/jetty/websocket/javax/common/JavaxMessageWholeMethodHolder.java
@@ -13,7 +13,6 @@
 
 package org.eclipse.jetty.websocket.javax.common;
 
-import java.lang.invoke.MethodHandle;
 import java.lang.invoke.WrongMethodTypeException;
 import javax.websocket.MessageHandler;
 
@@ -38,30 +37,6 @@ public Object invoke(Object... args) throws Throwable
         return null;
     }
 
-    @Override
-    public MethodHolder bindTo(Object arg)
-    {
-        throw new UnsupportedOperationException();
-    }
-
-    @Override
-    public MethodHolder bindTo(Object arg, int idx)
-    {
-        throw new UnsupportedOperationException();
-    }
-
-    @Override
-    public MethodHolder filterReturnValue(MethodHandle filter)
-    {
-        throw new UnsupportedOperationException();
-    }
-
-    @Override
-    public MethodHolder changeReturnType(Class<Object> objectClass)
-    {
-        throw new UnsupportedOperationException();
-    }
-
     @Override
     public Class<?> parameterType(int idx)
     {

diff --git a/...main/java/org/eclipse/jetty/websocket/javax/common/JavaxWebSocketFrameHandlerFactory.java b/...main/java/org/eclipse/jetty/websocket/javax/common/JavaxWebSocketFrameHandlerFactory.java
@@ -57,21 +57,6 @@
 
 public abstract class JavaxWebSocketFrameHandlerFactory
 {
-    private static final MethodHandle FILTER_RETURN_TYPE_METHOD;
-
-    static
-    {
-        try
-        {
-            FILTER_RETURN_TYPE_METHOD = getServerMethodHandleLookup()
-                .findVirtual(JavaxWebSocketSession.class, "filterReturnType", MethodType.methodType(void.class, Object.class));
-        }
-        catch (Throwable e)
-        {
-            throw new RuntimeException(e);
-        }
-    }
-
     static InvokerUtils.Arg[] getArgsFor(Class<?> objectType)
     {
         return new InvokerUtils.Arg[]{new InvokerUtils.Arg(Session.class), new InvokerUtils.Arg(objectType).required()};
@@ -228,15 +213,11 @@ public static MethodHolder wrapNonVoidReturnType(MethodHolder handle, JavaxWebSo
         if (handle.returnType() == Void.TYPE)
             return handle;
 
-        // Technique from  https://stackoverflow.com/questions/48505787/methodhandle-with-general-non-void-return-filter
-
-        // Change the return type of the to be Object so it will match exact with JavaxWebSocketSession.filterReturnType(Object)
-        handle = handle.changeReturnType(Object.class);
-
-        // Filter the method return type to a call to JavaxWebSocketSession.filterReturnType() bound to this session
-        handle = handle.filterReturnValue(FILTER_RETURN_TYPE_METHOD.bindTo(session));
-
-        return handle;
+        return args ->
+        {
+            session.filterReturnType(handle.invoke(args));
+            return null;
+        };
     }
 
     private MethodHandle toMethodHandle(MethodHandles.Lookup lookup, Method method)