Fix comments. We still seem to have a plugin message coming though on login,

can't see why...

proxy/src/main/java/com/velocitypowered/proxy/connection/ConnectionType.java

@@ -4,34 +4,12 @@ import com.velocitypowered.api.util.GameProfile;
 import com.velocitypowered.proxy.config.PlayerInfoForwarding;
 import com.velocitypowered.proxy.connection.backend.BackendConnectionPhase;
 import com.velocitypowered.proxy.connection.client.ClientConnectionPhase;
-import com.velocitypowered.proxy.connection.forge.legacy.LegacyForgeConnectionType;
-import com.velocitypowered.proxy.connection.util.ConnectionTypeImpl;
 
 /**
  * The types of connection that may be selected.
  */
 public interface ConnectionType {
 
-  /**
-   * Indicates that the connection has yet to reach the
-   * point where we have a definitive answer as to what
-   * type of connection we have.
-   */
-  ConnectionType UNDETERMINED =
-      new ConnectionTypeImpl(ClientConnectionPhase.VANILLA, BackendConnectionPhase.UNKNOWN);
-
-  /**
-   * Indicates that the connection is a Vanilla connection.
-   */
-  ConnectionType VANILLA =
-      new ConnectionTypeImpl(ClientConnectionPhase.VANILLA, BackendConnectionPhase.VANILLA);
-
-  /**
-   * Indicates that the connection is a 1.8-1.12 Forge
-   * connection.
-   */
-  ConnectionType LEGACY_FORGE = new LegacyForgeConnectionType();
-
   /**
    * The initial {@link ClientConnectionPhase} for this connection type.
    *

proxy/src/main/java/com/velocitypowered/proxy/connection/ConnectionTypes.java

@@ -0,0 +1,36 @@
+package com.velocitypowered.proxy.connection;
+
+import com.velocitypowered.proxy.connection.backend.BackendConnectionPhases;
+import com.velocitypowered.proxy.connection.client.ClientConnectionPhases;
+import com.velocitypowered.proxy.connection.forge.legacy.LegacyForgeConnectionType;
+import com.velocitypowered.proxy.connection.util.ConnectionTypeImpl;
+
+/**
+ * The connection types supported by Velocity.
+ */
+public final class ConnectionTypes {
+
+  /**
+   * Indicates that the connection has yet to reach the
+   * point where we have a definitive answer as to what
+   * type of connection we have.
+   */
+  public static final ConnectionType UNDETERMINED =
+      new ConnectionTypeImpl(ClientConnectionPhases.VANILLA, BackendConnectionPhases.UNKNOWN);
+
+  /**
+   * Indicates that the connection is a Vanilla connection.
+   */
+  public static final ConnectionType VANILLA =
+      new ConnectionTypeImpl(ClientConnectionPhases.VANILLA, BackendConnectionPhases.VANILLA);
+
+  /**
+   * Indicates that the connection is a 1.8-1.12 Forge
+   * connection.
+   */
+  public static final ConnectionType LEGACY_FORGE = new LegacyForgeConnectionType();
+
+  private ConnectionTypes() {
+    throw new AssertionError();
+  }
+}

proxy/src/main/java/com/velocitypowered/proxy/connection/MinecraftConnection.java

@@ -57,7 +57,7 @@ public class MinecraftConnection extends ChannelInboundHandlerAdapter {
   private ProtocolVersion nextProtocolVersion;
   private @Nullable MinecraftConnectionAssociation association;
   private final VelocityServer server;
-  private ConnectionType connectionType = ConnectionType.UNDETERMINED;
+  private ConnectionType connectionType = ConnectionTypes.UNDETERMINED;
 
   public MinecraftConnection(Channel channel, VelocityServer server) {
     this.channel = channel;

proxy/src/main/java/com/velocitypowered/proxy/connection/backend/BackendConnectionPhase.java

@@ -12,30 +12,6 @@ import com.velocitypowered.proxy.protocol.packet.PluginMessage;
  */
 public interface BackendConnectionPhase {
 
-  /**
-   * The backend connection is vanilla.
-   */
-  BackendConnectionPhase VANILLA = new BackendConnectionPhase() {};
-
-  /**
-   * The backend connection is unknown at this time.
-   */
-  BackendConnectionPhase UNKNOWN = new BackendConnectionPhase() {
-    @Override
-    public boolean consideredComplete() {
-      return false;
-    }
-
-    @Override
-    public boolean handle(VelocityServerConnection serverConn,
-                          ConnectedPlayer player,
-                          PluginMessage message) {
-      // The connection may be legacy forge. If so, the Forge handler will deal with this
-      // for us. Otherwise, we have nothing to do.
-      return LegacyForgeHandshakeBackendPhase.NOT_STARTED.handle(serverConn, player, message);
-    }
-  };
-
   /**
    * Handle a plugin message in the context of
    * this phase.

proxy/src/main/java/com/velocitypowered/proxy/connection/backend/BackendConnectionPhases.java

@@ -0,0 +1,42 @@
+package com.velocitypowered.proxy.connection.backend;
+
+import com.velocitypowered.proxy.connection.client.ConnectedPlayer;
+import com.velocitypowered.proxy.connection.forge.legacy.LegacyForgeHandshakeBackendPhase;
+import com.velocitypowered.proxy.protocol.packet.PluginMessage;
+
+/**
+ * Contains Vanilla {@link BackendConnectionPhase}s.
+ *
+ * <p>See {@link LegacyForgeHandshakeBackendPhase} for Legacy Forge
+ * versions</p>
+ */
+public final class BackendConnectionPhases {
+
+  /**
+   * The backend connection is vanilla.
+   */
+  public static final BackendConnectionPhase VANILLA = new BackendConnectionPhase() {};
+
+  /**
+   * The backend connection is unknown at this time.
+   */
+  public static final BackendConnectionPhase UNKNOWN = new BackendConnectionPhase() {
+    @Override
+    public boolean consideredComplete() {
+      return false;
+    }
+
+    @Override
+    public boolean handle(VelocityServerConnection serverConn,
+                          ConnectedPlayer player,
+                          PluginMessage message) {
+      // The connection may be legacy forge. If so, the Forge handler will deal with this
+      // for us. Otherwise, we have nothing to do.
+      return LegacyForgeHandshakeBackendPhase.NOT_STARTED.handle(serverConn, player, message);
+    }
+  };
+
+  private BackendConnectionPhases() {
+    throw new AssertionError();
+  }
+}

proxy/src/main/java/com/velocitypowered/proxy/connection/backend/VelocityServerConnection.java

@@ -17,7 +17,7 @@ import com.velocitypowered.api.proxy.server.ServerInfo;
 import com.velocitypowered.api.network.ProtocolVersion;
 import com.velocitypowered.proxy.VelocityServer;
 import com.velocitypowered.proxy.config.PlayerInfoForwarding;
-import com.velocitypowered.proxy.connection.ConnectionType;
+import com.velocitypowered.proxy.connection.ConnectionTypes;
 import com.velocitypowered.proxy.connection.MinecraftConnection;
 import com.velocitypowered.proxy.connection.MinecraftConnectionAssociation;
 import com.velocitypowered.proxy.connection.client.ConnectedPlayer;
@@ -47,7 +47,7 @@ public class VelocityServerConnection implements MinecraftConnectionAssociation,
   private final VelocityServer server;
   private @Nullable MinecraftConnection connection;
   private boolean gracefulDisconnect = false;
-  private BackendConnectionPhase connectionPhase = BackendConnectionPhase.UNKNOWN;
+  private BackendConnectionPhase connectionPhase = BackendConnectionPhases.UNKNOWN;
   private long lastPingId;
   private long lastPingSent;
 
@@ -138,7 +138,7 @@ public class VelocityServerConnection implements MinecraftConnectionAssociation,
     handshake.setProtocolVersion(proxyPlayer.getConnection().getNextProtocolVersion());
     if (forwardingMode == PlayerInfoForwarding.LEGACY) {
       handshake.setServerAddress(createLegacyForwardingAddress());
-    } else if (proxyPlayer.getConnection().getType() == ConnectionType.LEGACY_FORGE) {
+    } else if (proxyPlayer.getConnection().getType() == ConnectionTypes.LEGACY_FORGE) {
       handshake.setServerAddress(handshake.getServerAddress() + LegacyForgeConstants.HANDSHAKE_HOSTNAME_TOKEN);
     } else {
       handshake.setServerAddress(registeredServer.getServerInfo().getAddress().getHostString());
@@ -204,11 +204,11 @@ public class VelocityServerConnection implements MinecraftConnectionAssociation,
   }
 
   public void completeJoin() {
-    if (connectionPhase == BackendConnectionPhase.UNKNOWN) {
+    if (connectionPhase == BackendConnectionPhases.UNKNOWN) {
       // Now we know
-      connectionPhase = BackendConnectionPhase.VANILLA;
+      connectionPhase = BackendConnectionPhases.VANILLA;
       if (connection != null) {
-        connection.setType(ConnectionType.VANILLA);
+        connection.setType(ConnectionTypes.VANILLA);
       }
     }
   }

proxy/src/main/java/com/velocitypowered/proxy/connection/client/ClientConnectionPhase.java

@@ -11,11 +11,6 @@ import com.velocitypowered.proxy.protocol.packet.PluginMessage;
  */
 public interface ClientConnectionPhase {
 
-  /**
-   * The client is connecting with a vanilla client (as far as we can tell).
-   */
-  ClientConnectionPhase VANILLA = new ClientConnectionPhase() {};
-
   /**
    * Handle a plugin message in the context of
    * this phase.

proxy/src/main/java/com/velocitypowered/proxy/connection/client/ClientConnectionPhases.java

@@ -0,0 +1,19 @@
+package com.velocitypowered.proxy.connection.client;
+
+/**
+ * The vanilla {@link ClientConnectionPhase}s.
+ *
+ * <p>See {@link com.velocitypowered.proxy.connection.forge.legacy.LegacyForgeHandshakeClientPhase}
+ * for Legacy Forge phases</p>
+ */
+public final class ClientConnectionPhases {
+
+  /**
+   * The client is connecting with a vanilla client (as far as we can tell).
+   */
+  public static final ClientConnectionPhase VANILLA = new ClientConnectionPhase() {};
+
+  private ClientConnectionPhases() {
+    throw new AssertionError();
+  }
+}

proxy/src/main/java/com/velocitypowered/proxy/connection/client/ClientPlaySessionHandler.java

@@ -3,13 +3,13 @@ package com.velocitypowered.proxy.connection.client;
 import com.velocitypowered.api.event.connection.PluginMessageEvent;
 import com.velocitypowered.api.event.player.PlayerChatEvent;
 import com.velocitypowered.api.proxy.messages.ChannelIdentifier;
-import com.velocitypowered.api.util.ModInfo;
 import com.velocitypowered.api.network.ProtocolVersion;
 import com.velocitypowered.proxy.VelocityServer;
 import com.velocitypowered.proxy.connection.MinecraftConnection;
 import com.velocitypowered.proxy.connection.MinecraftSessionHandler;
 import com.velocitypowered.proxy.connection.backend.VelocityServerConnection;
 import com.velocitypowered.proxy.protocol.MinecraftPacket;
+import com.velocitypowered.proxy.protocol.StateRegistry;
 import com.velocitypowered.proxy.protocol.packet.BossBar;
 import com.velocitypowered.proxy.protocol.packet.Chat;
 import com.velocitypowered.proxy.protocol.packet.ClientSettings;
@@ -158,7 +158,9 @@ public class ClientPlaySessionHandler implements MinecraftSessionHandler {
     VelocityServerConnection serverConn = player.getConnectedServer();
     MinecraftConnection backendConn = serverConn != null ? serverConn.getConnection() : null;
     if (serverConn != null && backendConn != null) {
-      if (PluginMessageUtil.isRegister(packet)) {
+      if (backendConn.getState() != StateRegistry.PLAY) {
+        logger.warn("Plugin message was sent while the backend was in PLAY. Channel: {}. Packet discarded.");
+      } else if (PluginMessageUtil.isRegister(packet)) {
         List<String> actuallyRegistered = new ArrayList<>();
         List<String> channels = PluginMessageUtil.getChannels(packet);
         for (String channel : channels) {

proxy/src/main/java/com/velocitypowered/proxy/connection/client/ConnectedPlayer.java

@@ -66,8 +66,8 @@ public class ConnectedPlayer implements MinecraftConnectionAssociation, Player {
   private static final PlainComponentSerializer PASS_THRU_TRANSLATE = new PlainComponentSerializer(
       c -> "", TranslatableComponent::key);
   static final PermissionProvider DEFAULT_PERMISSIONS = s -> PermissionFunction.ALWAYS_UNDEFINED;
-  private static final Random RANDOM = new Random();
 
+  private static final ThreadLocal<Random> threadLocalRandom = ThreadLocal.withInitial(Random::new);
   private static final Logger logger = LogManager.getLogger(ConnectedPlayer.class);
 
   private final MinecraftConnection connection;
@@ -479,7 +479,7 @@ public class ConnectedPlayer implements MinecraftConnectionAssociation, Player {
   public void sendKeepAlive() {
     if (connection.getState() == StateRegistry.PLAY) {
       KeepAlive keepAlive = new KeepAlive();
-      keepAlive.setRandomId(RANDOM.nextLong());
+      keepAlive.setRandomId(threadLocalRandom.get().nextLong());
       connection.write(keepAlive);
     }
   }

proxy/src/main/java/com/velocitypowered/proxy/connection/client/HandshakeSessionHandler.java

@@ -11,6 +11,7 @@ import com.velocitypowered.proxy.VelocityServer;
 import com.velocitypowered.proxy.config.PlayerInfoForwarding;
 import com.velocitypowered.proxy.config.VelocityConfiguration;
 import com.velocitypowered.proxy.connection.ConnectionType;
+import com.velocitypowered.proxy.connection.ConnectionTypes;
 import com.velocitypowered.proxy.connection.MinecraftConnection;
 import com.velocitypowered.proxy.connection.MinecraftSessionHandler;
 import com.velocitypowered.proxy.connection.forge.legacy.LegacyForgeConstants;
@@ -128,11 +129,11 @@ public class HandshakeSessionHandler implements MinecraftSessionHandler {
     // Determine if we're using Forge (1.8 to 1.12, may not be the case in 1.13).
     if (handshake.getServerAddress().endsWith(LegacyForgeConstants.HANDSHAKE_HOSTNAME_TOKEN)
         && handshake.getProtocolVersion() < ProtocolConstants.MINECRAFT_1_13) {
-      return ConnectionType.LEGACY_FORGE;
+      return ConnectionTypes.LEGACY_FORGE;
     } else {
       // For later: See if we can determine Forge 1.13+ here, else this will need to be UNDETERMINED
       // until later in the cycle (most likely determinable during the LOGIN phase)
-      return ConnectionType.VANILLA;
+      return ConnectionTypes.VANILLA;
     }
   }
 

proxy/src/main/java/com/velocitypowered/proxy/connection/forge/legacy/LegacyForgeConnectionType.java

@@ -2,11 +2,11 @@ package com.velocitypowered.proxy.connection.forge.legacy;
 
 import com.velocitypowered.api.util.GameProfile;
 import com.velocitypowered.proxy.config.PlayerInfoForwarding;
-import com.velocitypowered.proxy.connection.ConnectionType;
+import com.velocitypowered.proxy.connection.ConnectionTypes;
 import com.velocitypowered.proxy.connection.util.ConnectionTypeImpl;
 
 /**
- * Contains extra logic for {@link ConnectionType#LEGACY_FORGE}
+ * Contains extra logic for {@link ConnectionTypes#LEGACY_FORGE}
  */
 public class LegacyForgeConnectionType extends ConnectionTypeImpl {
 

proxy/src/main/java/com/velocitypowered/proxy/connection/forge/legacy/LegacyForgeHandshakeBackendPhase.java

@@ -1,7 +1,8 @@
 package com.velocitypowered.proxy.connection.forge.legacy;
 
-import com.velocitypowered.proxy.connection.ConnectionType;
+import com.velocitypowered.proxy.connection.ConnectionTypes;
 import com.velocitypowered.proxy.connection.backend.BackendConnectionPhase;
+import com.velocitypowered.proxy.connection.backend.BackendConnectionPhases;
 import com.velocitypowered.proxy.connection.backend.VelocityServerConnection;
 import com.velocitypowered.proxy.connection.client.ConnectedPlayer;
 import com.velocitypowered.proxy.protocol.packet.PluginMessage;
@@ -14,7 +15,7 @@ import javax.annotation.Nullable;
 public enum LegacyForgeHandshakeBackendPhase implements BackendConnectionPhase {
 
   /**
-   * Dummy phase for use with {@link BackendConnectionPhase#UNKNOWN}
+   * Dummy phase for use with {@link BackendConnectionPhases#UNKNOWN}
    */
   NOT_STARTED(LegacyForgeConstants.SERVER_HELLO_DISCRIMINATOR) {
     @Override
@@ -38,7 +39,7 @@ public enum LegacyForgeHandshakeBackendPhase implements BackendConnectionPhase {
       // We must always reset the handshake before a modded connection is established if
       // we haven't done so already.
       if (connection.getConnection() != null) {
-        connection.getConnection().setType(ConnectionType.LEGACY_FORGE);
+        connection.getConnection().setType(ConnectionTypes.LEGACY_FORGE);
       }
       connection.getPlayer().sendLegacyForgeHandshakeResetPacket();
     }

proxy/src/main/java/com/velocitypowered/proxy/connection/forge/legacy/LegacyForgeHandshakeClientPhase.java

@@ -7,8 +7,8 @@ import com.velocitypowered.proxy.connection.client.ClientConnectionPhase;
 import com.velocitypowered.proxy.connection.client.ClientPlaySessionHandler;
 import com.velocitypowered.proxy.connection.client.ConnectedPlayer;
 import com.velocitypowered.proxy.protocol.packet.PluginMessage;
-import javax.annotation.Nullable;
 import java.util.List;
+import javax.annotation.Nullable;
 
 /**
  * Allows for simple tracking of the phase that the Legacy
@@ -36,6 +36,16 @@ public enum LegacyForgeHandshakeClientPhase implements ClientConnectionPhase {
       // Forge client that we must reset on the next switch.
       player.setPhase(LegacyForgeHandshakeClientPhase.COMPLETE);
     }
+
+    @Override
+    boolean onHandle(ConnectedPlayer player,
+                     ClientPlaySessionHandler handler,
+                     PluginMessage message,
+                     MinecraftConnection backendConn) {
+      // If we stay in this phase, we do nothing because it means the packet wasn't handled.
+      // Returning false indicates this
+      return false;
+    }
   },
 
   /**
@@ -63,14 +73,19 @@ public enum LegacyForgeHandshakeClientPhase implements ClientConnectionPhase {
     }
 
     @Override
-    void preWrite(ConnectedPlayer player, PluginMessage packet) {
+    boolean onHandle(ConnectedPlayer player,
+                  ClientPlaySessionHandler handler,
+                  PluginMessage message,
+                  MinecraftConnection backendConn) {
       // Read the mod list if we haven't already.
       if (!player.getModInfo().isPresent()) {
-        List<ModInfo.Mod> mods = LegacyForgeUtil.readModList(packet);
+        List<ModInfo.Mod> mods = LegacyForgeUtil.readModList(message);
         if (!mods.isEmpty()) {
           player.setModInfo(new ModInfo("FML", mods));
         }
       }
+
+      return super.onHandle(player, handler, message, backendConn);
     }
   },
 
@@ -133,10 +148,17 @@ public enum LegacyForgeHandshakeClientPhase implements ClientConnectionPhase {
     }
 
     @Override
-    void postWrite(ConnectedPlayer player, ClientPlaySessionHandler handler) {
+    boolean onHandle(ConnectedPlayer player,
+                     ClientPlaySessionHandler handler,
+                     PluginMessage message,
+                     MinecraftConnection backendConn) {
+      super.onHandle(player, handler, message, backendConn);
+
       // just in case the timing is awful
       player.sendKeepAlive();
       handler.flushQueuedMessages();
+
+      return true;
     }
   };
 
@@ -170,17 +192,8 @@ public enum LegacyForgeHandshakeClientPhase implements ClientConnectionPhase {
         // Update phase on player
         player.setPhase(newPhase);
 
-        // Perform tasks before sending the packet on to the server.
+        // Perform phase handling
-        newPhase.preWrite(player, message);
+        return newPhase.onHandle(player, handler, message, backendConn);
-
-        // Send the packet on to the server.
-        backendConn.write(message);
-
-        // Perform tasks after sending the packet on, such as keep alives.
-        newPhase.postWrite(player, handler);
-
-        // We handled the packet, nothing else needs to.
-        return true;
       }
     }
 
@@ -188,33 +201,33 @@ public enum LegacyForgeHandshakeClientPhase implements ClientConnectionPhase {
     return false;
   }
 
+  /**
+   * Handles the phase tasks
+   *
+   * @param player The player
+   * @param handler The {@link ClientPlaySessionHandler} that is handling
+   *                packets
+   * @param message The message to handle
+   * @param backendConn The backend connection to write to, if required.
+   *
+   * @return true if handled, false otherwise.
+   */
+  boolean onHandle(ConnectedPlayer player,
+                   ClientPlaySessionHandler handler,
+                   PluginMessage message,
+                   MinecraftConnection backendConn) {
+    // Send the packet on to the server.
+    backendConn.write(message);
+
+    // We handled the packet. No need to continue processing.
+    return true;
+  }
+
   @Override
   public boolean consideredComplete() {
     return false;
   }
 
-  /**
-   * Actions to occur before the handled packet is sent on to
-   * the server.
-   *
-   * @param player The player to act on
-   * @param packet The packet that was sent
-   */
-  void preWrite(ConnectedPlayer player, PluginMessage packet) {
-    // usually nothing to do.
-  }
-
-  /**
-   * Actions to occur after the handled packet is sent on to the
-   * server.
-   *
-   * @param player The player
-   * @param handler The {@link ClientPlaySessionHandler} to act with
-   */
-  void postWrite(ConnectedPlayer player, ClientPlaySessionHandler handler) {
-    // usually nothing to do
-  }
-
   /**
    * Gets the next phase, if any (will return self if we are at the end
    * of the handshake).