Issue #6328 - changes from review

Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
jetty · Jun 8, 2022 · 72ba95d · 72ba95d
1 parent 9c3fdd3
commit 72ba95d
Show file tree

Hide file tree

Showing 5 changed files with 21 additions and 47 deletions.
diff --git a/...c/main/java/org/eclipse/jetty/websocket/core/internal/messages/ByteBufferMessageSink.java b/...c/main/java/org/eclipse/jetty/websocket/core/internal/messages/ByteBufferMessageSink.java
@@ -13,7 +13,6 @@
 
 package org.eclipse.jetty.websocket.core.internal.messages;
 
-import java.lang.invoke.MethodHandle;
 import java.nio.ByteBuffer;
 
 import org.eclipse.jetty.io.ByteBufferCallbackAccumulator;
@@ -34,12 +33,6 @@ public ByteBufferMessageSink(CoreSession session, MethodHolder methodHolder)
         super(session, methodHolder);
     }
 
-    @Deprecated
-    public ByteBufferMessageSink(CoreSession session, MethodHandle methodHandle)
-    {
-        this(session, MethodHolder.from(methodHandle));
-    }
-
     @Override
     public void accept(Frame frame, Callback callback)
     {

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
@@ -32,7 +32,7 @@
  */
 public interface MethodHolder
 {
-    String METHOD_HOLDER_BINDING_PROPERTY = "jetty.methodholder.binding";
+    String METHOD_HOLDER_BINDING_PROPERTY = "jetty.websocket.methodholder.binding";
 
     static MethodHolder from(MethodHandle methodHandle)
     {

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
@@ -22,14 +22,15 @@
 
 /**
  * This implementation of {@link MethodHolder} is not thread safe.
- * Mutual exclusion should be used when calling {@link #invoke(Object...)}.
+ * Mutual exclusion should be used when calling {@link #invoke(Object...)}, or this should only
+ * be invoked from a single thread.
  */
 class NonBindingMethodHolder implements MethodHolder
 {
     private final MethodHandle _methodHandle;
     private final Object[] _parameters;
-    private final List<Integer> _lookupTable = new LinkedList<>();
-    private List<MethodHandle> _returnFilters;
+    private final List<Integer> _unboundParamIndexes = new LinkedList<>();
+    private final List<MethodHandle> _returnFilters = new ArrayList<>();
 
     public NonBindingMethodHolder(MethodHandle methodHandle)
     {
@@ -38,37 +39,20 @@ public NonBindingMethodHolder(MethodHandle methodHandle)
         _parameters = new Object[numParams];
         for (int i = 0; i < numParams; i++)
         {
-            _lookupTable.add(i);
+            _unboundParamIndexes.add(i);
         }
     }
 
-    private void dropFromLookupTable(int index)
-    {
-        if (index < 0 || index >= _lookupTable.size())
-            throw new IndexOutOfBoundsException();
-        _lookupTable.remove(index);
-    }
-
-    private int getInternalIndex(int index)
-    {
-        if (index < 0 || index >= _lookupTable.size())
-            throw new IndexOutOfBoundsException();
-        return _lookupTable.get(index);
-    }
-
     @Override
     public Object invoke(Object... args) throws Throwable
     {
         try
         {
             insertArguments(args);
             Object o = _methodHandle.invokeWithArguments(_parameters);
-            if (_returnFilters != null)
+            for (MethodHandle filter : _returnFilters)
             {
-                for (MethodHandle filter : _returnFilters)
-                {
-                    o = filter.invoke(o);
-                }
+                o = filter.invoke(o);
             }
             return o;
         }
@@ -81,8 +65,8 @@ public Object invoke(Object... args) throws Throwable
     @Override
     public MethodHolder bindTo(Object arg, int idx)
     {
-        _parameters[getInternalIndex(idx)] = arg;
-        dropFromLookupTable(idx);
+        _parameters[_unboundParamIndexes.get(idx)] = arg;
+        _unboundParamIndexes.remove(idx);
         return this;
     }
 
@@ -92,22 +76,21 @@ public MethodHolder bindTo(Object arg)
         return bindTo(arg, 0);
     }
 
-    @SuppressWarnings({"UnusedReturnValue", "SameParameterValue"})
-    private MethodHolder insertArguments(Object... args)
+    private void insertArguments(Object... args)
     {
-        if (_lookupTable.size() != args.length)
-            throw new WrongMethodTypeException(String.format("Expected %s params but had %s", _lookupTable.size(), args.length));
+        if (_unboundParamIndexes.size() != args.length)
+            throw new WrongMethodTypeException(String.format("Expected %s params but had %s", _unboundParamIndexes.size(), args.length));
 
-        for (int i = 0; i < args.length; i++)
+        int argsIndex = 0;
+        for (int index : _unboundParamIndexes)
         {
-            _parameters[_lookupTable.get(i)] = args[i];
+            _parameters[index] = args[argsIndex++];
         }
-        return this;
     }
 
     private void clearArguments()
     {
-        for (int i : _lookupTable)
+        for (int i : _unboundParamIndexes)
         {
             _parameters[i] = null;
         }
@@ -116,21 +99,19 @@ private void clearArguments()
     @Override
     public MethodHolder filterReturnValue(MethodHandle filter)
     {
-        if (_returnFilters == null)
-            _returnFilters = new ArrayList<>();
         _returnFilters.add(filter);
         return this;
     }
 
     @Override
     public Class<?> parameterType(int idx)
     {
-        return _methodHandle.type().parameterType(getInternalIndex(idx));
+        return _methodHandle.type().parameterType(_unboundParamIndexes.get(idx));
     }
 
     @Override
     public Class<?> returnType()
     {
-        return ((_returnFilters == null) ? _methodHandle : _returnFilters.get(_returnFilters.size() - 1)).type().returnType();
+        return (_returnFilters.isEmpty() ? _methodHandle : _returnFilters.get(_returnFilters.size() - 1)).type().returnType();
     }
 }
diff --git a/...main/java/org/eclipse/jetty/websocket/javax/common/JavaxWebSocketFrameHandlerFactory.java b/...main/java/org/eclipse/jetty/websocket/javax/common/JavaxWebSocketFrameHandlerFactory.java
@@ -205,7 +205,7 @@ public static MessageSink createMessageSink(JavaxWebSocketSession session, Javax
         }
     }
 
-    public static MethodHolder wrapNonVoidReturnType(MethodHolder handle, JavaxWebSocketSession session)
+    static MethodHolder wrapNonVoidReturnType(MethodHolder handle, JavaxWebSocketSession session)
     {
         if (handle == null)
             return null;

diff --git a/...jetty-common/src/test/java/org/eclipse/jetty/websocket/common/OutgoingMessageCapture.java b/...jetty-common/src/test/java/org/eclipse/jetty/websocket/common/OutgoingMessageCapture.java
@@ -106,7 +106,7 @@ public void sendFrame(Frame frame, Callback callback, boolean batch)
                 String event = String.format("BINARY:fin=%b:len=%d", frame.isFin(), frame.getPayloadLength());
                 LOG.debug(event);
                 events.offer(event);
-                messageSink = new ByteBufferMessageSink(this, wholeBinaryHandle);
+                messageSink = new ByteBufferMessageSink(this, MethodHolder.from(wholeBinaryHandle));
                 break;
             }
             case OpCode.CONTINUATION: