From 7b84da2fa709b7e83e170fcf68cacd45236a141a Mon Sep 17 00:00:00 2001 From: Andrew Steinborn Date: Tue, 21 Aug 2018 22:29:01 -0400 Subject: [PATCH] Cleaning up some stuff in the proxy implementation. --- .../velocitypowered/proxy/VelocityServer.java | 20 ++++++++++++------- .../backend/BackendPlaySessionHandler.java | 1 - .../client/ClientPlaySessionHandler.java | 13 ++++++++---- .../client/HandshakeSessionHandler.java | 7 +++++++ .../client/LoginSessionHandler.java | 7 +++++++ .../client/StatusSessionHandler.java | 2 +- .../proxy/plugin/VelocityEventManager.java | 10 +++++----- .../proxy/plugin/VelocityPluginManager.java | 1 - .../protocol/remap/EntityIdRemapper.java | 2 +- .../proxy/scheduler/VelocityScheduler.java | 3 ++- ...ctory.java => RecordingThreadFactory.java} | 11 ++++++---- ...t.java => RecordingThreadFactoryTest.java} | 4 ++-- 12 files changed, 54 insertions(+), 27 deletions(-) rename proxy/src/main/java/com/velocitypowered/proxy/util/concurrency/{ThreadRecorderThreadFactory.java => RecordingThreadFactory.java} (68%) rename proxy/src/test/java/com/velocitypowered/proxy/util/concurrency/{ThreadRecorderThreadFactoryTest.java => RecordingThreadFactoryTest.java} (86%) diff --git a/proxy/src/main/java/com/velocitypowered/proxy/VelocityServer.java b/proxy/src/main/java/com/velocitypowered/proxy/VelocityServer.java index 73a15ae9d..c474019e6 100644 --- a/proxy/src/main/java/com/velocitypowered/proxy/VelocityServer.java +++ b/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; diff --git a/proxy/src/main/java/com/velocitypowered/proxy/connection/backend/BackendPlaySessionHandler.java b/proxy/src/main/java/com/velocitypowered/proxy/connection/backend/BackendPlaySessionHandler.java index 3bf35fe17..b48aa2dcd 100644 --- a/proxy/src/main/java/com/velocitypowered/proxy/connection/backend/BackendPlaySessionHandler.java +++ b/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. diff --git a/proxy/src/main/java/com/velocitypowered/proxy/connection/client/ClientPlaySessionHandler.java b/proxy/src/main/java/com/velocitypowered/proxy/connection/client/ClientPlaySessionHandler.java index 32344337d..2f185f328 100644 --- a/proxy/src/main/java/com/velocitypowered/proxy/connection/client/ClientPlaySessionHandler.java +++ b/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); } diff --git a/proxy/src/main/java/com/velocitypowered/proxy/connection/client/HandshakeSessionHandler.java b/proxy/src/main/java/com/velocitypowered/proxy/connection/client/HandshakeSessionHandler.java index 8b089a6ee..9cfa0227f 100644 --- a/proxy/src/main/java/com/velocitypowered/proxy/connection/client/HandshakeSessionHandler.java +++ b/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(); diff --git a/proxy/src/main/java/com/velocitypowered/proxy/connection/client/LoginSessionHandler.java b/proxy/src/main/java/com/velocitypowered/proxy/connection/client/LoginSessionHandler.java index c912ba7f0..8fc74b6e0 100644 --- a/proxy/src/main/java/com/velocitypowered/proxy/connection/client/LoginSessionHandler.java +++ b/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)); + } } diff --git a/proxy/src/main/java/com/velocitypowered/proxy/connection/client/StatusSessionHandler.java b/proxy/src/main/java/com/velocitypowered/proxy/connection/client/StatusSessionHandler.java index bef061769..97d479dec 100644 --- a/proxy/src/main/java/com/velocitypowered/proxy/connection/client/StatusSessionHandler.java +++ b/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) { diff --git a/proxy/src/main/java/com/velocitypowered/proxy/plugin/VelocityEventManager.java b/proxy/src/main/java/com/velocitypowered/proxy/plugin/VelocityEventManager.java index fa828883e..3f84e6279 100644 --- a/proxy/src/main/java/com/velocitypowered/proxy/plugin/VelocityEventManager.java +++ b/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 { diff --git a/proxy/src/main/java/com/velocitypowered/proxy/plugin/VelocityPluginManager.java b/proxy/src/main/java/com/velocitypowered/proxy/plugin/VelocityPluginManager.java index 85ae7fbf4..074f8940b 100644 --- a/proxy/src/main/java/com/velocitypowered/proxy/plugin/VelocityPluginManager.java +++ b/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; diff --git a/proxy/src/main/java/com/velocitypowered/proxy/protocol/remap/EntityIdRemapper.java b/proxy/src/main/java/com/velocitypowered/proxy/protocol/remap/EntityIdRemapper.java index ad277d762..c112b344f 100644 --- a/proxy/src/main/java/com/velocitypowered/proxy/protocol/remap/EntityIdRemapper.java +++ b/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 { diff --git a/proxy/src/main/java/com/velocitypowered/proxy/scheduler/VelocityScheduler.java b/proxy/src/main/java/com/velocitypowered/proxy/scheduler/VelocityScheduler.java index 5c40a0817..5584e36c8 100644 --- a/proxy/src/main/java/com/velocitypowered/proxy/scheduler/VelocityScheduler.java +++ b/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 { diff --git a/proxy/src/main/java/com/velocitypowered/proxy/util/concurrency/ThreadRecorderThreadFactory.java b/proxy/src/main/java/com/velocitypowered/proxy/util/concurrency/RecordingThreadFactory.java similarity index 68% rename from proxy/src/main/java/com/velocitypowered/proxy/util/concurrency/ThreadRecorderThreadFactory.java rename to proxy/src/main/java/com/velocitypowered/proxy/util/concurrency/RecordingThreadFactory.java index f8a454e9a..ae289bb1c 100644 --- a/proxy/src/main/java/com/velocitypowered/proxy/util/concurrency/ThreadRecorderThreadFactory.java +++ b/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 threads = ConcurrentHashMap.newKeySet(); + private final Set threads = Collections.newSetFromMap(new MapMaker().weakKeys().makeMap()); - public ThreadRecorderThreadFactory(ThreadFactory backing) { + public RecordingThreadFactory(ThreadFactory backing) { this.backing = Preconditions.checkNotNull(backing, "backing"); } diff --git a/proxy/src/test/java/com/velocitypowered/proxy/util/concurrency/ThreadRecorderThreadFactoryTest.java b/proxy/src/test/java/com/velocitypowered/proxy/util/concurrency/RecordingThreadFactoryTest.java similarity index 86% rename from proxy/src/test/java/com/velocitypowered/proxy/util/concurrency/ThreadRecorderThreadFactoryTest.java rename to proxy/src/test/java/com/velocitypowered/proxy/util/concurrency/RecordingThreadFactoryTest.java index 72d026cf4..cac1ef295 100644 --- a/proxy/src/test/java/com/velocitypowered/proxy/util/concurrency/ThreadRecorderThreadFactoryTest.java +++ b/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(() -> {