Cleaning up some stuff in the proxy implementation.

proxy/src/main/java/com/velocitypowered/proxy/VelocityServer.java

@@ -139,14 +139,13 @@ public class VelocityServer implements ProxyServer {
         scheduler = new VelocityScheduler(pluginManager, Sleeper.SYSTEM);
         loadPlugins();
 
-        // Post the first event
-        pluginManager.getPlugins().forEach(container -> {
-            container.getInstance().ifPresent(plugin -> eventManager.register(plugin, plugin));
-        });
         try {
+            // Go ahead and fire the proxy initialization event. We block since plugins should have a chance
+            // to fully initialize before we accept any connections to the server.
             eventManager.fire(new ProxyInitializeEvent()).get();
         } catch (InterruptedException | ExecutionException e) {
-            // Ignore, we don't care.
+            // Ignore, we don't care. InterruptedException is unlikely to happen (and if it does, you've got bigger
+            // issues) and there is almost no chance ExecutionException will be thrown.
         }
 
         this.cm.bind(configuration.getBind());
@@ -176,6 +175,11 @@ public class VelocityServer implements ProxyServer {
             logger.error("Couldn't load plugins", e);
         }
 
+        // Register the plugin main classes so that we may proceed with firing the proxy initialize event
+        pluginManager.getPlugins().forEach(container -> {
+            container.getInstance().ifPresent(plugin -> eventManager.register(plugin, plugin));
+        });
+
         logger.info("Loaded {} plugins", pluginManager.getPlugins().size());
     }
 
@@ -203,9 +207,11 @@ public class VelocityServer implements ProxyServer {
 
         eventManager.fire(new ProxyShutdownEvent());
         try {
-            eventManager.shutdown();
+            if (!eventManager.shutdown() || scheduler.shutdown()) {
+                logger.error("Your plugins took over 10 seconds to shut down.");
+            }
         } catch (InterruptedException e) {
-            logger.error("Your plugins took over 10 seconds to shut down.");
+            // Not much we can do about this...
         }
 
         shutdown = true;

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

@@ -25,7 +25,6 @@ public class BackendPlaySessionHandler implements MinecraftSessionHandler {
 
     @Override
     public void handle(MinecraftPacket packet) {
-        //Not handleable packets: Chat, TabCompleteResponse, Respawn, Scoreboard*
         if (!connection.getPlayer().isActive()) {
             // Connection was left open accidentally. Close it so as to avoid "You logged in from another location"
             // errors.

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

@@ -21,6 +21,10 @@ import java.util.concurrent.ScheduledFuture;
 import java.util.concurrent.ThreadLocalRandom;
 import java.util.concurrent.TimeUnit;
 
+/**
+ * Handles communication with the connected Minecraft client. This is effectively the primary nerve center that
+ * joins backend servers with players.
+ */
 public class ClientPlaySessionHandler implements MinecraftSessionHandler {
     private static final Logger logger = LogManager.getLogger(ClientPlaySessionHandler.class);
     private static final int MAX_PLUGIN_CHANNELS = 128;
@@ -53,6 +57,7 @@ public class ClientPlaySessionHandler implements MinecraftSessionHandler {
         }
 
         if (packet instanceof Chat) {
+            // Try to handle any commands on the proxy. If that fails, send it onto the client.
             Chat chat = (Chat) packet;
             String msg = ((Chat) packet).getMessage();
             if (msg.startsWith("/")) {
@@ -137,14 +142,15 @@ public class ClientPlaySessionHandler implements MinecraftSessionHandler {
             player.getConnection().delayedWrite(joinGame);
             idRemapper = EntityIdRemapper.getMapper(joinGame.getEntityId(), player.getConnection().getProtocolVersion());
         } else {
-            // In order to handle switching to another server we will need send three packets:
+            // Ah, this is the meat and potatoes of the whole venture!
+            //
+            // In order to handle switching to another server, you will need to send three packets:
             //
             // - The join game packet from the backend server
             // - A respawn packet with a different dimension
             // - Another respawn with the correct dimension
             //
-            // We can't simply ignore the packet with the different dimension. If you try to be smart about it it doesn't
-            // work.
+            // The two respawns with different dimensions are required, otherwise the client gets confused.
             //
             // Most notably, by having the client accept the join game packet, we can work around the need to perform
             // entity ID rewrites, eliminating potential issues from rewriting packets and improving compatibility with
@@ -197,7 +203,6 @@ public class ClientPlaySessionHandler implements MinecraftSessionHandler {
             }
 
             if (actuallyRegistered.size() > 0) {
-                logger.info("Rewritten register packet: {}", actuallyRegistered);
                 PluginMessage newRegisterPacket = PluginMessageUtil.constructChannelsPacket(packet.getChannel(), actuallyRegistered);
                 player.getConnectedServer().getMinecraftConnection().write(newRegisterPacket);
             }

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

@@ -14,6 +14,8 @@ import com.velocitypowered.proxy.protocol.MinecraftPacket;
 import com.velocitypowered.proxy.protocol.ProtocolConstants;
 import com.velocitypowered.proxy.protocol.StateRegistry;
 import com.velocitypowered.proxy.protocol.packet.*;
+import io.netty.buffer.ByteBuf;
+import io.netty.buffer.ByteBufUtil;
 import net.kyori.text.TextComponent;
 import net.kyori.text.TranslatableComponent;
 import net.kyori.text.format.TextColor;
@@ -71,6 +73,11 @@ public class HandshakeSessionHandler implements MinecraftSessionHandler {
         }
     }
 
+    @Override
+    public void handleUnknown(ByteBuf buf) {
+        throw new IllegalStateException("Unknown data " + ByteBufUtil.hexDump(buf));
+    }
+
     private void handleLegacy(MinecraftPacket packet) {
         if (packet instanceof LegacyPing) {
             VelocityConfiguration configuration = VelocityServer.getServer().getConfiguration();

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

@@ -16,6 +16,8 @@ import com.velocitypowered.proxy.connection.MinecraftConnection;
 import com.velocitypowered.proxy.connection.MinecraftSessionHandler;
 import com.velocitypowered.proxy.VelocityServer;
 import com.velocitypowered.proxy.util.EncryptionUtils;
+import io.netty.buffer.ByteBuf;
+import io.netty.buffer.ByteBufUtil;
 import io.netty.buffer.Unpooled;
 import net.kyori.text.TextComponent;
 import net.kyori.text.format.TextColor;
@@ -204,4 +206,9 @@ public class LoginSessionHandler implements MinecraftSessionHandler {
         inbound.setSessionHandler(new InitialConnectSessionHandler(player));
         player.createConnectionRequest(toTry.get()).fireAndForget();
     }
+
+    @Override
+    public void handleUnknown(ByteBuf buf) {
+        throw new IllegalStateException("Unknown data " + ByteBufUtil.hexDump(buf));
+    }
 }

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

@@ -28,7 +28,7 @@ public class StatusSessionHandler implements MinecraftSessionHandler {
 
     @Override
     public void handle(MinecraftPacket packet) {
-        Preconditions.checkArgument(packet instanceof StatusPing|| packet instanceof StatusRequest,
+        Preconditions.checkArgument(packet instanceof StatusPing || packet instanceof StatusRequest,
                 "Unrecognized packet type " + packet.getClass().getName());
 
         if (packet instanceof StatusPing) {

proxy/src/main/java/com/velocitypowered/proxy/plugin/VelocityEventManager.java

@@ -9,7 +9,7 @@ import com.velocitypowered.api.event.EventManager;
 import com.velocitypowered.api.event.PostOrder;
 import com.velocitypowered.api.event.Subscribe;
 import com.velocitypowered.api.plugin.PluginManager;
-import com.velocitypowered.proxy.util.concurrency.ThreadRecorderThreadFactory;
+import com.velocitypowered.proxy.util.concurrency.RecordingThreadFactory;
 import net.kyori.event.EventSubscriber;
 import net.kyori.event.PostResult;
 import net.kyori.event.SimpleEventBus;
@@ -35,12 +35,12 @@ public class VelocityEventManager implements EventManager {
             new ASMEventExecutorFactory<>(new PluginClassLoader(new URL[0])),
             new VelocityMethodScanner());
     private final ExecutorService service;
-    private final ThreadRecorderThreadFactory recordingThreadFactory;
+    private final RecordingThreadFactory recordingThreadFactory;
     private final PluginManager pluginManager;
 
     public VelocityEventManager(PluginManager pluginManager) {
         this.pluginManager = pluginManager;
-        this.recordingThreadFactory = new ThreadRecorderThreadFactory(new ThreadFactoryBuilder()
+        this.recordingThreadFactory = new RecordingThreadFactory(new ThreadFactoryBuilder()
                 .setNameFormat("Velocity Event Executor - #%d").setDaemon(true).build());
         this.service = Executors.newFixedThreadPool(Runtime.getRuntime().availableProcessors(), recordingThreadFactory);
     }
@@ -123,9 +123,9 @@ public class VelocityEventManager implements EventManager {
         bus.unregister(handler);
     }
 
-    public void shutdown() throws InterruptedException {
+    public boolean shutdown() throws InterruptedException {
         service.shutdown();
-        service.awaitTermination(10, TimeUnit.SECONDS);
+        return service.awaitTermination(10, TimeUnit.SECONDS);
     }
 
     private static class VelocityEventBus extends SimpleEventBus<Object> {

proxy/src/main/java/com/velocitypowered/proxy/plugin/VelocityPluginManager.java

@@ -1,6 +1,5 @@
 package com.velocitypowered.proxy.plugin;
 
-import com.google.common.base.Preconditions;
 import com.velocitypowered.api.plugin.PluginDescription;
 import com.velocitypowered.api.plugin.PluginContainer;
 import com.velocitypowered.api.plugin.PluginManager;

proxy/src/main/java/com/velocitypowered/proxy/protocol/remap/EntityIdRemapper.java

@@ -5,7 +5,7 @@ import io.netty.buffer.ByteBuf;
 
 /**
  * Represents a protocol-specific entity ID remapper for certain Minecraft packets. This is mostly required to support
- * old versions of Minecraft. For Minecraft 1.9 clients and above, Velocity can use a more efficient method based on
+ * old versions of Minecraft. For Minecraft 1.8 clients and above, Velocity can use a more efficient method based on
  * sending JoinGame packets multiple times.
  */
 public interface EntityIdRemapper {

proxy/src/main/java/com/velocitypowered/proxy/scheduler/VelocityScheduler.java

@@ -40,11 +40,12 @@ public class VelocityScheduler implements Scheduler {
         return new TaskBuilderImpl(plugin, runnable);
     }
 
-    public void shutdown() {
+    public boolean shutdown() throws InterruptedException {
         for (ScheduledTask task : ImmutableList.copyOf(tasksByPlugin.values())) {
             task.cancel();
         }
         taskService.shutdown();
+        return taskService.awaitTermination(10, TimeUnit.SECONDS);
     }
 
     private class TaskBuilderImpl implements TaskBuilder {

proxy/src/main/java/com/velocitypowered/proxy/util/concurrency/RecordingThreadFactory.java

@@ -2,19 +2,22 @@ package com.velocitypowered.proxy.util.concurrency;
 
 import com.google.common.annotations.VisibleForTesting;
 import com.google.common.base.Preconditions;
+import com.google.common.collect.MapMaker;
 
+import java.util.Collections;
 import java.util.Set;
 import java.util.concurrent.ConcurrentHashMap;
 import java.util.concurrent.ThreadFactory;
 
 /**
- * Represents a {@link ThreadFactory} that records the threads it has spawned.
+ * A {@link ThreadFactory} that records the threads it has created. Once a thread terminates, it is automatically removed
+ * from the recorder.
  */
-public class ThreadRecorderThreadFactory implements ThreadFactory {
+public class RecordingThreadFactory implements ThreadFactory {
     private final ThreadFactory backing;
-    private final Set<Thread> threads = ConcurrentHashMap.newKeySet();
+    private final Set<Thread> threads = Collections.newSetFromMap(new MapMaker().weakKeys().makeMap());
 
-    public ThreadRecorderThreadFactory(ThreadFactory backing) {
+    public RecordingThreadFactory(ThreadFactory backing) {
         this.backing = Preconditions.checkNotNull(backing, "backing");
     }
 

proxy/src/test/java/com/velocitypowered/proxy/util/concurrency/RecordingThreadFactoryTest.java

@@ -7,11 +7,11 @@ import java.util.concurrent.Executors;
 
 import static org.junit.jupiter.api.Assertions.*;
 
-class ThreadRecorderThreadFactoryTest {
+class RecordingThreadFactoryTest {
 
     @Test
     void newThread() throws Exception {
-        ThreadRecorderThreadFactory factory = new ThreadRecorderThreadFactory(Executors.defaultThreadFactory());
+        RecordingThreadFactory factory = new RecordingThreadFactory(Executors.defaultThreadFactory());
         CountDownLatch started = new CountDownLatch(1);
         CountDownLatch endThread = new CountDownLatch(1);
         factory.newThread(() -> {