Issue #3428 - changes from review

Signed-off-by: Lachlan Roberts <lachlan@webtide.com>

jetty-websocket/websocket-javax-common/src/main/java/org/eclipse/jetty/websocket/javax/common/JavaxWebSocketCallingArgs.java

@@ -1,57 +0,0 @@
-//
-// ========================================================================
-// Copyright (c) 1995-2020 Mort Bay Consulting Pty Ltd and others.
-//
-// This program and the accompanying materials are made available under
-// the terms of the Eclipse Public License 2.0 which is available at
-// https://www.eclipse.org/legal/epl-2.0
-//
-// This Source Code may also be made available under the following
-// Secondary Licenses when the conditions for such availability set
-// forth in the Eclipse Public License, v. 2.0 are satisfied:
-// the Apache License v2.0 which is available at
-// https://www.apache.org/licenses/LICENSE-2.0
-//
-// SPDX-License-Identifier: EPL-2.0 OR Apache-2.0
-// ========================================================================
-//
-
-package org.eclipse.jetty.websocket.javax.common;
-
-import java.nio.ByteBuffer;
-import javax.websocket.PongMessage;
-import javax.websocket.Session;
-
-import org.eclipse.jetty.websocket.util.InvokerUtils;
-
-// The different kind of @OnMessage method parameter signatures expected.
-public class JavaxWebSocketCallingArgs
-{
-    static InvokerUtils.Arg[] getArgsFor(Class<?> objectType)
-    {
-        return new InvokerUtils.Arg[]{new InvokerUtils.Arg(Session.class), new InvokerUtils.Arg(objectType).required()};
-    }
-
-    static final InvokerUtils.Arg[] textPartialCallingArgs = new InvokerUtils.Arg[]{
-        new InvokerUtils.Arg(Session.class),
-        new InvokerUtils.Arg(String.class).required(),
-        new InvokerUtils.Arg(boolean.class).required()
-    };
-
-    static final InvokerUtils.Arg[] binaryPartialBufferCallingArgs = new InvokerUtils.Arg[]{
-        new InvokerUtils.Arg(Session.class),
-        new InvokerUtils.Arg(ByteBuffer.class).required(),
-        new InvokerUtils.Arg(boolean.class).required()
-    };
-
-    static final InvokerUtils.Arg[] binaryPartialArrayCallingArgs = new InvokerUtils.Arg[]{
-        new InvokerUtils.Arg(Session.class),
-        new InvokerUtils.Arg(byte[].class).required(),
-        new InvokerUtils.Arg(boolean.class).required()
-    };
-
-    static final InvokerUtils.Arg[] pongCallingArgs = new InvokerUtils.Arg[]{
-        new InvokerUtils.Arg(Session.class),
-        new InvokerUtils.Arg(PongMessage.class).required()
-    };
-}

jetty-websocket/websocket-javax-common/src/main/java/org/eclipse/jetty/websocket/javax/common/JavaxWebSocketFrameHandler.java

@@ -570,7 +570,7 @@ public class JavaxWebSocketFrameHandler implements FrameHandler
                         this.binarySink = null;
                         break;
                     default:
-                        throw new IllegalStateException();
+                        throw new IllegalStateException("Invalid MessageHandler type " + OpCode.name(key));
                 }
             }
         }

jetty-websocket/websocket-javax-common/src/main/java/org/eclipse/jetty/websocket/javax/common/JavaxWebSocketFrameHandlerFactory.java

@@ -25,6 +25,7 @@ import java.lang.invoke.MethodType;
 import java.lang.reflect.InvocationTargetException;
 import java.lang.reflect.Method;
 import java.lang.reflect.Modifier;
+import java.nio.ByteBuffer;
 import java.util.ArrayList;
 import java.util.List;
 import java.util.Map;
@@ -37,6 +38,7 @@ import javax.websocket.OnClose;
 import javax.websocket.OnError;
 import javax.websocket.OnMessage;
 import javax.websocket.OnOpen;
+import javax.websocket.PongMessage;
 import javax.websocket.Session;
 
 import org.eclipse.jetty.http.pathmap.UriTemplatePathSpec;
@@ -57,7 +59,6 @@ import org.eclipse.jetty.websocket.util.messages.PartialByteBufferMessageSink;
 import org.eclipse.jetty.websocket.util.messages.PartialStringMessageSink;
 
 import static java.nio.charset.StandardCharsets.UTF_8;
-import static org.eclipse.jetty.websocket.javax.common.JavaxWebSocketCallingArgs.getArgsFor;
 
 public abstract class JavaxWebSocketFrameHandlerFactory
 {
@@ -76,6 +77,34 @@ public abstract class JavaxWebSocketFrameHandlerFactory
         }
     }
 
+    static InvokerUtils.Arg[] getArgsFor(Class<?> objectType)
+    {
+        return new InvokerUtils.Arg[]{new InvokerUtils.Arg(Session.class), new InvokerUtils.Arg(objectType).required()};
+    }
+
+    static final InvokerUtils.Arg[] textPartialCallingArgs = new InvokerUtils.Arg[]{
+        new InvokerUtils.Arg(Session.class),
+        new InvokerUtils.Arg(String.class).required(),
+        new InvokerUtils.Arg(boolean.class).required()
+    };
+
+    static final InvokerUtils.Arg[] binaryPartialBufferCallingArgs = new InvokerUtils.Arg[]{
+        new InvokerUtils.Arg(Session.class),
+        new InvokerUtils.Arg(ByteBuffer.class).required(),
+        new InvokerUtils.Arg(boolean.class).required()
+    };
+
+    static final InvokerUtils.Arg[] binaryPartialArrayCallingArgs = new InvokerUtils.Arg[]{
+        new InvokerUtils.Arg(Session.class),
+        new InvokerUtils.Arg(byte[].class).required(),
+        new InvokerUtils.Arg(boolean.class).required()
+    };
+
+    static final InvokerUtils.Arg[] pongCallingArgs = new InvokerUtils.Arg[]{
+        new InvokerUtils.Arg(Session.class),
+        new InvokerUtils.Arg(PongMessage.class).required()
+    };
+
     protected final JavaxWebSocketContainer container;
     protected final InvokerUtils.ParamIdentifier paramIdentifier;
 
@@ -327,7 +356,7 @@ public abstract class JavaxWebSocketFrameHandlerFactory
                                    Function<InvokerUtils.Arg[], MethodHandle> getMethodHandle)
     {
         // Partial Text Message.
-        MethodHandle methodHandle = getMethodHandle.apply(JavaxWebSocketCallingArgs.textPartialCallingArgs);
+        MethodHandle methodHandle = getMethodHandle.apply(textPartialCallingArgs);
         if (methodHandle != null)
         {
             msgMetadata.setSinkClass(PartialStringMessageSink.class);
@@ -337,7 +366,7 @@ public abstract class JavaxWebSocketFrameHandlerFactory
         }
 
         // Partial ByteBuffer Binary Message.
-        methodHandle = getMethodHandle.apply(JavaxWebSocketCallingArgs.binaryPartialBufferCallingArgs);
+        methodHandle = getMethodHandle.apply(binaryPartialBufferCallingArgs);
         if (methodHandle != null)
         {
             msgMetadata.setSinkClass(PartialByteBufferMessageSink.class);
@@ -347,7 +376,7 @@ public abstract class JavaxWebSocketFrameHandlerFactory
         }
 
         // Partial byte[] Binary Message.
-        methodHandle = getMethodHandle.apply(JavaxWebSocketCallingArgs.binaryPartialArrayCallingArgs);
+        methodHandle = getMethodHandle.apply(binaryPartialArrayCallingArgs);
         if (methodHandle != null)
         {
             msgMetadata.setSinkClass(PartialByteArrayMessageSink.class);
@@ -357,7 +386,7 @@ public abstract class JavaxWebSocketFrameHandlerFactory
         }
 
         // Pong Message.
-        MethodHandle pongHandle = getMethodHandle.apply(JavaxWebSocketCallingArgs.pongCallingArgs);
+        MethodHandle pongHandle = getMethodHandle.apply(pongCallingArgs);
         if (pongHandle != null)
         {
             metadata.setPongHandle(pongHandle, onMsg);

jetty-websocket/websocket-javax-common/src/main/java/org/eclipse/jetty/websocket/javax/common/decoders/AvailableDecoders.java

@@ -157,6 +157,7 @@ public class AvailableDecoders implements Iterable<RegisteredDecoder>
                 return;
         }
 
+        // TODO: explain ordering of Decoders and why this is added at position 0.
         registeredDecoders.add(0, new RegisteredDecoder(decoder, interfaceClass, objectType, config));
     }
 
@@ -178,7 +179,8 @@ public class AvailableDecoders implements Iterable<RegisteredDecoder>
     public List<RegisteredDecoder> getRegisteredDecoders(Class<? extends Decoder> interfaceType, Class<?> returnType)
     {
         return registeredDecoders.stream()
-            .filter(registered -> registered.interfaceType.equals(interfaceType) && registered.isType(returnType))
+            .filter(registered -> registered.interfaceType.equals(interfaceType))
+            .filter(registered -> registered.isType(returnType))
             .collect(Collectors.toList());
     }
 

jetty-websocket/websocket-javax-common/src/main/java/org/eclipse/jetty/websocket/javax/common/messages/AbstractDecodedMessageSink.java

@@ -33,14 +33,14 @@ import org.slf4j.LoggerFactory;
 
 public abstract class AbstractDecodedMessageSink implements MessageSink
 {
-    protected final Logger _logger;
+    private static final Logger LOG = LoggerFactory.getLogger(AbstractDecodedMessageSink.class);
-    protected final CoreSession _coreSession;
+
-    protected final MethodHandle _methodHandle;
+    private final CoreSession _coreSession;
-    protected final MessageSink _messageSink;
+    private final MethodHandle _methodHandle;
+    private final MessageSink _messageSink;
 
     public AbstractDecodedMessageSink(CoreSession coreSession, MethodHandle methodHandle)
     {
-        _logger = LoggerFactory.getLogger(getClass());
         _coreSession = coreSession;
         _methodHandle = methodHandle;
 
@@ -55,6 +55,16 @@ public abstract class AbstractDecodedMessageSink implements MessageSink
         }
     }
 
+    public CoreSession getCoreSession()
+    {
+        return _coreSession;
+    }
+
+    public MethodHandle getMethodHandle()
+    {
+        return _methodHandle;
+    }
+
     /**
      * @return a message sink which will first decode the message then pass it to {@link #_methodHandle}.
      * @throws Exception for any error in creating the message sink.
@@ -64,8 +74,8 @@ public abstract class AbstractDecodedMessageSink implements MessageSink
     @Override
     public void accept(Frame frame, Callback callback)
     {
-        if (_logger.isDebugEnabled())
+        if (LOG.isDebugEnabled())
-            _logger.debug("accepting frame {} for {}", frame, _messageSink);
+            LOG.debug("accepting frame {} for {}", frame, _messageSink);
         _messageSink.accept(frame, callback);
     }
 

jetty-websocket/websocket-javax-common/src/main/java/org/eclipse/jetty/websocket/javax/common/messages/DecodedBinaryMessageSink.java

@@ -32,9 +32,13 @@ import org.eclipse.jetty.websocket.javax.common.JavaxWebSocketFrameHandlerFactor
 import org.eclipse.jetty.websocket.javax.common.decoders.RegisteredDecoder;
 import org.eclipse.jetty.websocket.util.messages.ByteBufferMessageSink;
 import org.eclipse.jetty.websocket.util.messages.MessageSink;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
 
 public class DecodedBinaryMessageSink<T> extends AbstractDecodedMessageSink.Basic<Decoder.Binary<T>>
 {
+    private static final Logger LOG = LoggerFactory.getLogger(DecodedBinaryMessageSink.class);
+
     public DecodedBinaryMessageSink(CoreSession session, MethodHandle methodHandle, List<RegisteredDecoder> decoders)
     {
         super(session, methodHandle, decoders);
@@ -43,10 +47,10 @@ public class DecodedBinaryMessageSink<T> extends AbstractDecodedMessageSink.Basi
     @Override
     MessageSink getMessageSink() throws Exception
     {
-        MethodHandle methodHandle = JavaxWebSocketFrameHandlerFactory.getServerMethodHandleLookup().findVirtual(DecodedBinaryMessageSink.class,
+        MethodHandle methodHandle = JavaxWebSocketFrameHandlerFactory.getServerMethodHandleLookup()
-            "onWholeMessage", MethodType.methodType(void.class, ByteBuffer.class))
+            .findVirtual(DecodedBinaryMessageSink.class, "onWholeMessage", MethodType.methodType(void.class, ByteBuffer.class))
             .bindTo(this);
-        return new ByteBufferMessageSink(_coreSession, methodHandle);
+        return new ByteBufferMessageSink(getCoreSession(), methodHandle);
     }
 
     @SuppressWarnings("Duplicates")
@@ -59,7 +63,7 @@ public class DecodedBinaryMessageSink<T> extends AbstractDecodedMessageSink.Basi
                 try
                 {
                     T obj = decoder.decode(wholeMessage);
-                    _methodHandle.invoke(obj);
+                    getMethodHandle().invoke(obj);
                     return;
                 }
                 catch (DecodeException e)
@@ -73,6 +77,6 @@ public class DecodedBinaryMessageSink<T> extends AbstractDecodedMessageSink.Basi
             }
         }
 
-        _logger.warn("Message lost, willDecode() has returned false for all decoders in the decoder list.");
+        LOG.warn("Message lost, willDecode() has returned false for all decoders in the decoder list.");
     }
 }

jetty-websocket/websocket-javax-common/src/main/java/org/eclipse/jetty/websocket/javax/common/messages/DecodedBinaryStreamMessageSink.java

@@ -46,7 +46,7 @@ public class DecodedBinaryStreamMessageSink<T> extends AbstractDecodedMessageSin
         MethodHandle methodHandle = JavaxWebSocketFrameHandlerFactory.getServerMethodHandleLookup().findVirtual(DecodedBinaryStreamMessageSink.class,
             "onStreamStart", MethodType.methodType(void.class, InputStream.class))
             .bindTo(this);
-        return new InputStreamMessageSink(_coreSession, methodHandle);
+        return new InputStreamMessageSink(getCoreSession(), methodHandle);
     }
 
     @SuppressWarnings("Duplicates")
@@ -55,7 +55,7 @@ public class DecodedBinaryStreamMessageSink<T> extends AbstractDecodedMessageSin
         try
         {
             T obj = _decoder.decode(stream);
-            _methodHandle.invoke(obj);
+            getMethodHandle().invoke(obj);
         }
         catch (DecodeException e)
         {

jetty-websocket/websocket-javax-common/src/main/java/org/eclipse/jetty/websocket/javax/common/messages/DecodedTextMessageSink.java

@@ -31,9 +31,13 @@ import org.eclipse.jetty.websocket.javax.common.JavaxWebSocketFrameHandlerFactor
 import org.eclipse.jetty.websocket.javax.common.decoders.RegisteredDecoder;
 import org.eclipse.jetty.websocket.util.messages.MessageSink;
 import org.eclipse.jetty.websocket.util.messages.StringMessageSink;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
 
 public class DecodedTextMessageSink<T> extends AbstractDecodedMessageSink.Basic<Decoder.Text<T>>
 {
+    private static final Logger LOG = LoggerFactory.getLogger(DecodedTextMessageSink.class);
+
     public DecodedTextMessageSink(CoreSession session, MethodHandle methodHandle, List<RegisteredDecoder> decoders)
     {
         super(session, methodHandle, decoders);
@@ -45,7 +49,7 @@ public class DecodedTextMessageSink<T> extends AbstractDecodedMessageSink.Basic<
         MethodHandle methodHandle = JavaxWebSocketFrameHandlerFactory.getServerMethodHandleLookup()
             .findVirtual(getClass(), "onMessage", MethodType.methodType(void.class, String.class))
             .bindTo(this);
-        return new StringMessageSink(_coreSession, methodHandle);
+        return new StringMessageSink(getCoreSession(), methodHandle);
     }
 
     @SuppressWarnings("Duplicates")
@@ -58,7 +62,7 @@ public class DecodedTextMessageSink<T> extends AbstractDecodedMessageSink.Basic<
                 try
                 {
                     T obj = decoder.decode(wholeMessage);
-                    _methodHandle.invoke(obj);
+                    getMethodHandle().invoke(obj);
                     return;
                 }
                 catch (DecodeException e)
@@ -72,6 +76,6 @@ public class DecodedTextMessageSink<T> extends AbstractDecodedMessageSink.Basic<
             }
         }
 
-        _logger.warn("Message lost, willDecode() has returned false for all decoders in the decoder list.");
+        LOG.warn("Message lost, willDecode() has returned false for all decoders in the decoder list.");
     }
 }

jetty-websocket/websocket-javax-common/src/main/java/org/eclipse/jetty/websocket/javax/common/messages/DecodedTextStreamMessageSink.java

@@ -46,7 +46,7 @@ public class DecodedTextStreamMessageSink<T> extends AbstractDecodedMessageSink.
         MethodHandle methodHandle = JavaxWebSocketFrameHandlerFactory.getServerMethodHandleLookup().findVirtual(DecodedTextStreamMessageSink.class,
             "onStreamStart", MethodType.methodType(void.class, Reader.class))
             .bindTo(this);
-        return new ReaderMessageSink(_coreSession, methodHandle);
+        return new ReaderMessageSink(getCoreSession(), methodHandle);
     }
 
     @SuppressWarnings("Duplicates")
@@ -55,7 +55,7 @@ public class DecodedTextStreamMessageSink<T> extends AbstractDecodedMessageSink.
         try
         {
             T obj = _decoder.decode(reader);
-            _methodHandle.invoke(obj);
+            getMethodHandle().invoke(obj);
         }
         catch (DecodeException e)
         {

jetty-websocket/websocket-javax-common/src/test/java/org/eclipse/jetty/websocket/javax/common/messages/DecodedBinaryMessageSinkTest.java

@@ -132,7 +132,6 @@ public class DecodedBinaryMessageSinkTest extends AbstractMessageSinkTest
     @SuppressWarnings("Duplicates")
     public static class GmtDecoder implements Decoder.Binary<Calendar>
     {
-
         @Override
         public Calendar decode(ByteBuffer buffer) throws DecodeException
         {