From f62d75989698b9efee51cdd8e59be0a4b587f3ed Mon Sep 17 00:00:00 2001 From: Andrew Steinborn Date: Thu, 17 Aug 2023 23:51:34 -0400 Subject: [PATCH] Do not track plugin channels registered per-player on the proxy (#591) We don't need to track this information since Velocity uses the JoinGame packet, which is about as good of a server rejoin mechanism we're likely to get in vanilla Minecraft. --- .../backend/BackendPlaySessionHandler.java | 9 +-- .../backend/TransitionSessionHandler.java | 7 -- .../client/ClientPlaySessionHandler.java | 9 --- .../connection/client/ConnectedPlayer.java | 13 ---- .../client/InitialConnectSessionHandler.java | 11 +-- .../proxy/util/collect/CappedSet.java | 70 ------------------ .../proxy/util/collect/CappedSetTest.java | 71 ------------------- 7 files changed, 3 insertions(+), 187 deletions(-) delete mode 100644 proxy/src/main/java/com/velocitypowered/proxy/util/collect/CappedSet.java delete mode 100644 proxy/src/test/java/com/velocitypowered/proxy/util/collect/CappedSetTest.java 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 3adf6ba25..ff777f5f9 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 @@ -198,13 +198,8 @@ public class BackendPlaySessionHandler implements MinecraftSessionHandler { return true; } - // We need to specially handle REGISTER and UNREGISTER packets. Later on, we'll write them to - // the client. - if (PluginMessageUtil.isRegister(packet)) { - serverConn.getPlayer().getKnownChannels().addAll(PluginMessageUtil.getChannels(packet)); - return false; - } else if (PluginMessageUtil.isUnregister(packet)) { - serverConn.getPlayer().getKnownChannels().removeAll(PluginMessageUtil.getChannels(packet)); + // Register and unregister packets are simply forwarded to the server as-is. + if (PluginMessageUtil.isRegister(packet) || PluginMessageUtil.isUnregister(packet)) { return false; } diff --git a/proxy/src/main/java/com/velocitypowered/proxy/connection/backend/TransitionSessionHandler.java b/proxy/src/main/java/com/velocitypowered/proxy/connection/backend/TransitionSessionHandler.java index 25cbbef63..5fe6886dc 100644 --- a/proxy/src/main/java/com/velocitypowered/proxy/connection/backend/TransitionSessionHandler.java +++ b/proxy/src/main/java/com/velocitypowered/proxy/connection/backend/TransitionSessionHandler.java @@ -36,7 +36,6 @@ import com.velocitypowered.proxy.protocol.packet.Disconnect; import com.velocitypowered.proxy.protocol.packet.JoinGame; import com.velocitypowered.proxy.protocol.packet.KeepAlive; import com.velocitypowered.proxy.protocol.packet.PluginMessage; -import com.velocitypowered.proxy.protocol.util.PluginMessageUtil; import java.io.IOException; import java.util.concurrent.CompletableFuture; import org.apache.logging.log4j.LogManager; @@ -180,12 +179,6 @@ public class TransitionSessionHandler implements MinecraftSessionHandler { return true; } - if (PluginMessageUtil.isRegister(packet)) { - serverConn.getPlayer().getKnownChannels().addAll(PluginMessageUtil.getChannels(packet)); - } else if (PluginMessageUtil.isUnregister(packet)) { - serverConn.getPlayer().getKnownChannels().removeAll(PluginMessageUtil.getChannels(packet)); - } - // We always need to handle plugin messages, for Forge compatibility. if (serverConn.getPhase().handle(serverConn, serverConn.getPlayer(), packet)) { // Handled, but check the server connection phase. 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 bc041d8ab..dec1b85df 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 @@ -156,7 +156,6 @@ public class ClientPlaySessionHandler implements MinecraftSessionHandler { if (!channels.isEmpty()) { PluginMessage register = constructChannelsPacket(player.getProtocolVersion(), channels); player.getConnection().write(register); - player.getKnownChannels().addAll(channels); } } @@ -295,7 +294,6 @@ public class ClientPlaySessionHandler implements MinecraftSessionHandler { packet.getChannel()); } else if (PluginMessageUtil.isRegister(packet)) { List channels = PluginMessageUtil.getChannels(packet); - player.getKnownChannels().addAll(channels); List channelIdentifiers = new ArrayList<>(); for (String channel : channels) { try { @@ -309,7 +307,6 @@ public class ClientPlaySessionHandler implements MinecraftSessionHandler { new PlayerChannelRegisterEvent(player, ImmutableList.copyOf(channelIdentifiers))); backendConn.write(packet.retain()); } else if (PluginMessageUtil.isUnregister(packet)) { - player.getKnownChannels().removeAll(PluginMessageUtil.getChannels(packet)); backendConn.write(packet.retain()); } else if (PluginMessageUtil.isMcBrand(packet)) { String brand = PluginMessageUtil.readBrandMessage(packet.content()); @@ -485,12 +482,6 @@ public class ClientPlaySessionHandler implements MinecraftSessionHandler { } serverBossBars.clear(); - // Tell the server about this client's plugin message channels. - ProtocolVersion serverVersion = serverMc.getProtocolVersion(); - if (!player.getKnownChannels().isEmpty()) { - serverMc.delayedWrite(constructChannelsPacket(serverVersion, player.getKnownChannels())); - } - // If we had plugin messages queued during login/FML handshake, send them now. PluginMessage pm; while ((pm = loginPluginMessages.poll()) != null) { diff --git a/proxy/src/main/java/com/velocitypowered/proxy/connection/client/ConnectedPlayer.java b/proxy/src/main/java/com/velocitypowered/proxy/connection/client/ConnectedPlayer.java index 579cd60ee..90464c9c8 100644 --- a/proxy/src/main/java/com/velocitypowered/proxy/connection/client/ConnectedPlayer.java +++ b/proxy/src/main/java/com/velocitypowered/proxy/connection/client/ConnectedPlayer.java @@ -77,12 +77,10 @@ import com.velocitypowered.proxy.tablist.VelocityTabList; import com.velocitypowered.proxy.tablist.VelocityTabListLegacy; import com.velocitypowered.proxy.util.ClosestLocaleMatcher; import com.velocitypowered.proxy.util.DurationUtils; -import com.velocitypowered.proxy.util.collect.CappedSet; import io.netty.buffer.ByteBufUtil; import io.netty.buffer.Unpooled; import java.net.InetSocketAddress; import java.util.ArrayDeque; -import java.util.Collection; import java.util.Collections; import java.util.List; import java.util.Locale; @@ -155,7 +153,6 @@ public class ConnectedPlayer implements MinecraftConnectionAssociation, Player, private final InternalTabList tabList; private final VelocityServer server; private ClientConnectionPhase connectionPhase; - private final Collection knownChannels; private final CompletableFuture teardownFuture = new CompletableFuture<>(); private @MonotonicNonNull List serversToTry = null; private @MonotonicNonNull Boolean previousResourceResponse; @@ -185,7 +182,6 @@ public class ConnectedPlayer implements MinecraftConnectionAssociation, Player, this.virtualHost = virtualHost; this.permissionFunction = PermissionFunction.ALWAYS_UNDEFINED; this.connectionPhase = connection.getType().getInitialClientPhase(); - this.knownChannels = CappedSet.create(MAX_PLUGIN_CHANNELS); this.onlineMode = onlineMode; if (connection.getProtocolVersion().compareTo(ProtocolVersion.MINECRAFT_1_19_3) >= 0) { @@ -1103,15 +1099,6 @@ public class ConnectedPlayer implements MinecraftConnectionAssociation, Player, this.connectionPhase = connectionPhase; } - /** - * Return all the plugin message channels "known" to the client. - * - * @return the channels - */ - public Collection getKnownChannels() { - return knownChannels; - } - @Override public @Nullable IdentifiedKey getIdentifiedKey() { return playerKey; diff --git a/proxy/src/main/java/com/velocitypowered/proxy/connection/client/InitialConnectSessionHandler.java b/proxy/src/main/java/com/velocitypowered/proxy/connection/client/InitialConnectSessionHandler.java index fcab2d62d..f264d262e 100644 --- a/proxy/src/main/java/com/velocitypowered/proxy/connection/client/InitialConnectSessionHandler.java +++ b/proxy/src/main/java/com/velocitypowered/proxy/connection/client/InitialConnectSessionHandler.java @@ -24,7 +24,6 @@ import com.velocitypowered.proxy.connection.MinecraftSessionHandler; import com.velocitypowered.proxy.connection.backend.BungeeCordMessageResponder; import com.velocitypowered.proxy.connection.backend.VelocityServerConnection; import com.velocitypowered.proxy.protocol.packet.PluginMessage; -import com.velocitypowered.proxy.protocol.util.PluginMessageUtil; import io.netty.buffer.ByteBufUtil; import io.netty.buffer.Unpooled; import org.apache.logging.log4j.LogManager; @@ -55,15 +54,7 @@ public class InitialConnectSessionHandler implements MinecraftSessionHandler { return true; } - if (PluginMessageUtil.isRegister(packet)) { - player.getKnownChannels().addAll(PluginMessageUtil.getChannels(packet)); - serverConn.ensureConnected().write(packet.retain()); - return true; - } else if (PluginMessageUtil.isUnregister(packet)) { - player.getKnownChannels().removeAll(PluginMessageUtil.getChannels(packet)); - serverConn.ensureConnected().write(packet.retain()); - return true; - } else if (BungeeCordMessageResponder.isBungeeCordMessage(packet)) { + if (BungeeCordMessageResponder.isBungeeCordMessage(packet)) { return true; } diff --git a/proxy/src/main/java/com/velocitypowered/proxy/util/collect/CappedSet.java b/proxy/src/main/java/com/velocitypowered/proxy/util/collect/CappedSet.java deleted file mode 100644 index 692910d57..000000000 --- a/proxy/src/main/java/com/velocitypowered/proxy/util/collect/CappedSet.java +++ /dev/null @@ -1,70 +0,0 @@ -/* - * Copyright (C) 2019-2023 Velocity Contributors - * - * This program is free software: you can redistribute it and/or modify - * it under the terms of the GNU General Public License as published by - * the Free Software Foundation, either version 3 of the License, or - * (at your option) any later version. - * - * This program is distributed in the hope that it will be useful, - * but WITHOUT ANY WARRANTY; without even the implied warranty of - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the - * GNU General Public License for more details. - * - * You should have received a copy of the GNU General Public License - * along with this program. If not, see . - */ - -package com.velocitypowered.proxy.util.collect; - -import com.google.common.base.Preconditions; -import com.google.common.collect.ForwardingSet; -import java.util.Collection; -import java.util.HashSet; -import java.util.Set; - -/** - * An unsynchronized collection that puts an upper bound on the size of the collection. - */ -public final class CappedSet extends ForwardingSet { - - private final Set delegate; - private final int upperSize; - - private CappedSet(Set delegate, int upperSize) { - this.delegate = delegate; - this.upperSize = upperSize; - } - - /** - * Creates a capped collection backed by a {@link HashSet}. - * - * @param maxSize the maximum size of the collection - * @param the type of elements in the collection - * @return the new collection - */ - public static Set create(int maxSize) { - return new CappedSet<>(new HashSet<>(), maxSize); - } - - @Override - protected Set delegate() { - return delegate; - } - - @Override - public boolean add(T element) { - if (this.delegate.size() >= upperSize) { - Preconditions.checkState(this.delegate.contains(element), - "collection is too large (%s >= %s)", - this.delegate.size(), this.upperSize); - return false; - } - return this.delegate.add(element); - } - - @Override - public boolean addAll(Collection collection) { - return this.standardAddAll(collection); - } -} diff --git a/proxy/src/test/java/com/velocitypowered/proxy/util/collect/CappedSetTest.java b/proxy/src/test/java/com/velocitypowered/proxy/util/collect/CappedSetTest.java deleted file mode 100644 index 2e118b4ac..000000000 --- a/proxy/src/test/java/com/velocitypowered/proxy/util/collect/CappedSetTest.java +++ /dev/null @@ -1,71 +0,0 @@ -/* - * Copyright (C) 2019-2021 Velocity Contributors - * - * This program is free software: you can redistribute it and/or modify - * it under the terms of the GNU General Public License as published by - * the Free Software Foundation, either version 3 of the License, or - * (at your option) any later version. - * - * This program is distributed in the hope that it will be useful, - * but WITHOUT ANY WARRANTY; without even the implied warranty of - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the - * GNU General Public License for more details. - * - * You should have received a copy of the GNU General Public License - * along with this program. If not, see . - */ - -package com.velocitypowered.proxy.util.collect; - -import static org.junit.jupiter.api.Assertions.assertEquals; -import static org.junit.jupiter.api.Assertions.assertFalse; -import static org.junit.jupiter.api.Assertions.assertThrows; -import static org.junit.jupiter.api.Assertions.assertTrue; - -import com.google.common.collect.ImmutableSet; -import java.util.Collection; -import java.util.Set; -import org.junit.jupiter.api.Test; - -class CappedSetTest { - - @Test - void basicVerification() { - Collection coll = CappedSet.create(1); - assertTrue(coll.add("coffee"), "did not add single item"); - assertThrows(IllegalStateException.class, () -> coll.add("tea"), - "item was added to collection although it is too full"); - assertEquals(1, coll.size(), "collection grew in size unexpectedly"); - } - - @Test - void testAddAll() { - Set doesFill1 = ImmutableSet.of("coffee", "tea"); - Set doesFill2 = ImmutableSet.of("chocolate"); - Set overfill = ImmutableSet.of("Coke", "Pepsi"); - - Collection coll = CappedSet.create(3); - assertTrue(coll.addAll(doesFill1), "did not add items"); - assertTrue(coll.addAll(doesFill2), "did not add items"); - assertThrows(IllegalStateException.class, () -> coll.addAll(overfill), - "items added to collection although it is too full"); - assertEquals(3, coll.size(), "collection grew in size unexpectedly"); - } - - @Test - void handlesSetBehaviorCorrectly() { - Set doesFill1 = ImmutableSet.of("coffee", "tea"); - Set doesFill2 = ImmutableSet.of("coffee", "chocolate"); - Set overfill = ImmutableSet.of("coffee", "Coke", "Pepsi"); - - Collection coll = CappedSet.create(3); - assertTrue(coll.addAll(doesFill1), "did not add items"); - assertTrue(coll.addAll(doesFill2), "did not add items"); - assertThrows(IllegalStateException.class, () -> coll.addAll(overfill), - "items added to collection although it is too full"); - - assertFalse(coll.addAll(doesFill1), "added items?!?"); - - assertEquals(3, coll.size(), "collection grew in size unexpectedly"); - } -} \ No newline at end of file