From fa954ab717a6c5da28c4f8034df3250576645a9e Mon Sep 17 00:00:00 2001 From: Andrew Steinborn Date: Mon, 25 May 2020 09:38:22 -0400 Subject: [PATCH 1/8] Confine preconnect validation/setup logic to event loop. --- .../proxy/connection/client/ConnectedPlayer.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) 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 fd6b9d71c..49ebc529a 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 @@ -733,7 +733,7 @@ public class ConnectedPlayer implements MinecraftConnectionAssociation, Player { // Otherwise, initiate the connection. ServerPreConnectEvent event = new ServerPreConnectEvent(ConnectedPlayer.this, toConnect); return server.getEventManager().fire(event) - .thenCompose(newEvent -> { + .thenComposeAsync(newEvent -> { Optional connectTo = newEvent.getResult().getServer(); if (!connectTo.isPresent()) { return CompletableFuture.completedFuture( @@ -754,7 +754,7 @@ public class ConnectedPlayer implements MinecraftConnectionAssociation, Player { server); connectionInFlight = con; return con.connect(); - }); + }, connection.eventLoop()); } @Override From b0f1398b454ef6a3ef8a1f6701373eac975f231e Mon Sep 17 00:00:00 2001 From: Andrew Steinborn Date: Mon, 25 May 2020 10:26:05 -0400 Subject: [PATCH 2/8] Further confinement of preconnect checks to event loop. --- .../connection/client/ConnectedPlayer.java | 67 ++++++++++--------- 1 file changed, 36 insertions(+), 31 deletions(-) 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 49ebc529a..09bfd08e2 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 @@ -1,5 +1,8 @@ package com.velocitypowered.proxy.connection.client; +import static com.velocitypowered.proxy.connection.util.ConnectionRequestResults.plainResult; +import static java.util.concurrent.CompletableFuture.completedFuture; + import com.google.common.base.Preconditions; import com.google.gson.JsonObject; import com.velocitypowered.api.event.connection.DisconnectEvent; @@ -33,7 +36,6 @@ import com.velocitypowered.proxy.connection.MinecraftConnectionAssociation; import com.velocitypowered.proxy.connection.backend.VelocityServerConnection; import com.velocitypowered.proxy.connection.forge.legacy.LegacyForgeConstants; import com.velocitypowered.proxy.connection.util.ConnectionMessages; -import com.velocitypowered.proxy.connection.util.ConnectionRequestResults; import com.velocitypowered.proxy.connection.util.ConnectionRequestResults.Impl; import com.velocitypowered.proxy.protocol.StateRegistry; import com.velocitypowered.proxy.protocol.packet.Chat; @@ -711,8 +713,8 @@ public class ConnectedPlayer implements MinecraftConnectionAssociation, Player { } private Optional checkServer(RegisteredServer server) { - Preconditions - .checkState(server instanceof VelocityRegisteredServer, "Not a valid Velocity server."); + Preconditions.checkArgument(server instanceof VelocityRegisteredServer, + "Not a valid Velocity server."); if (connectionInFlight != null || (connectedServer != null && !connectedServer.hasCompletedJoin())) { return Optional.of(ConnectionRequestBuilder.Status.CONNECTION_IN_PROGRESS); @@ -723,38 +725,41 @@ public class ConnectedPlayer implements MinecraftConnectionAssociation, Player { return Optional.empty(); } + private CompletableFuture> getInitialStatus() { + return CompletableFuture.supplyAsync(() -> checkServer(toConnect), connection.eventLoop()); + } + private CompletableFuture internalConnect() { - Optional initialCheck = checkServer(toConnect); - if (initialCheck.isPresent()) { - return CompletableFuture - .completedFuture(ConnectionRequestResults.plainResult(initialCheck.get(), toConnect)); - } - - // Otherwise, initiate the connection. - ServerPreConnectEvent event = new ServerPreConnectEvent(ConnectedPlayer.this, toConnect); - return server.getEventManager().fire(event) - .thenComposeAsync(newEvent -> { - Optional connectTo = newEvent.getResult().getServer(); - if (!connectTo.isPresent()) { - return CompletableFuture.completedFuture( - ConnectionRequestResults - .plainResult(ConnectionRequestBuilder.Status.CONNECTION_CANCELLED, toConnect) - ); + return this.getInitialStatus() + .thenCompose(initialCheck -> { + if (initialCheck.isPresent()) { + return completedFuture(plainResult(initialCheck.get(), toConnect)); } - RegisteredServer rs = connectTo.get(); - Optional lastCheck = checkServer(rs); - if (lastCheck.isPresent()) { - return CompletableFuture - .completedFuture(ConnectionRequestResults.plainResult(lastCheck.get(), rs)); - } + ServerPreConnectEvent event = new ServerPreConnectEvent(ConnectedPlayer.this, + toConnect); + return server.getEventManager().fire(event) + .thenComposeAsync(newEvent -> { + Optional newDest = newEvent.getResult().getServer(); + if (!newDest.isPresent()) { + return completedFuture( + plainResult(ConnectionRequestBuilder.Status.CONNECTION_CANCELLED, toConnect) + ); + } - VelocityRegisteredServer vrs = (VelocityRegisteredServer) rs; - VelocityServerConnection con = new VelocityServerConnection(vrs, ConnectedPlayer.this, - server); - connectionInFlight = con; - return con.connect(); - }, connection.eventLoop()); + RegisteredServer realDestination = newDest.get(); + Optional check = checkServer(realDestination); + if (check.isPresent()) { + return completedFuture(plainResult(check.get(), realDestination)); + } + + VelocityRegisteredServer vrs = (VelocityRegisteredServer) realDestination; + VelocityServerConnection con = new VelocityServerConnection(vrs, + ConnectedPlayer.this, server); + connectionInFlight = con; + return con.connect(); + }, connection.eventLoop()); + }); } @Override From 64c16e61d2f7dbe18ca60afa4650419938001e2e Mon Sep 17 00:00:00 2001 From: Andrew Steinborn Date: Mon, 25 May 2020 11:44:02 -0400 Subject: [PATCH 3/8] Reset in-flight connection only if the server disconnects the client. --- .../proxy/connection/client/ConnectedPlayer.java | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) 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 09bfd08e2..19f519591 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 @@ -766,11 +766,12 @@ public class ConnectedPlayer implements MinecraftConnectionAssociation, Player { public CompletableFuture connect() { return this.internalConnect() .whenCompleteAsync((status, throwable) -> { - if (status != null && !status.isSafe()) { - // If it's not safe to continue the connection we need to shut it down. - handleConnectionException(status.getAttemptedConnection(), throwable, true); - } else if ((status != null && !status.isSuccessful())) { - resetInFlightConnection(); + if (status != null && !status.isSuccessful()) { + if (!status.isSafe()) { + handleConnectionException(status.getAttemptedConnection(), throwable, false); + } else if (status.getStatus() == Status.SERVER_DISCONNECTED) { + resetInFlightConnection(); + } } }, connection.eventLoop()) .thenApply(x -> x); From 74ff56cbc9781851b88e1959ea93dda32a156530 Mon Sep 17 00:00:00 2001 From: Andrew Steinborn Date: Mon, 25 May 2020 11:49:45 -0400 Subject: [PATCH 4/8] Also reset when an exception is thrown. --- .../proxy/connection/client/ConnectedPlayer.java | 2 ++ 1 file changed, 2 insertions(+) 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 19f519591..4c5954cf6 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 @@ -772,6 +772,8 @@ public class ConnectedPlayer implements MinecraftConnectionAssociation, Player { } else if (status.getStatus() == Status.SERVER_DISCONNECTED) { resetInFlightConnection(); } + } else if (throwable != null) { + resetInFlightConnection(); } }, connection.eventLoop()) .thenApply(x -> x); From 942e2f2e1ad489561ce8ea2eb6ead5b4dea88f6b Mon Sep 17 00:00:00 2001 From: Andrew Steinborn Date: Mon, 25 May 2020 11:56:56 -0400 Subject: [PATCH 5/8] Better generic cleanup. --- .../proxy/connection/client/ConnectedPlayer.java | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) 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 4c5954cf6..e646a2179 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 @@ -757,11 +757,18 @@ public class ConnectedPlayer implements MinecraftConnectionAssociation, Player { VelocityServerConnection con = new VelocityServerConnection(vrs, ConnectedPlayer.this, server); connectionInFlight = con; - return con.connect(); + return con.connect().whenCompleteAsync((result, throwable) -> + this.cleanupIfRequired(con)); }, connection.eventLoop()); }); } + private void cleanupIfRequired(VelocityServerConnection establishedConnection) { + if (establishedConnection == connectionInFlight) { + resetInFlightConnection(); + } + } + @Override public CompletableFuture connect() { return this.internalConnect() @@ -769,11 +776,7 @@ public class ConnectedPlayer implements MinecraftConnectionAssociation, Player { if (status != null && !status.isSuccessful()) { if (!status.isSafe()) { handleConnectionException(status.getAttemptedConnection(), throwable, false); - } else if (status.getStatus() == Status.SERVER_DISCONNECTED) { - resetInFlightConnection(); } - } else if (throwable != null) { - resetInFlightConnection(); } }, connection.eventLoop()) .thenApply(x -> x); From ec1fc3944ddcb5dfc14d7ba3e680cacc98ee7a18 Mon Sep 17 00:00:00 2001 From: Andrew Steinborn Date: Mon, 25 May 2020 12:08:24 -0400 Subject: [PATCH 6/8] Make sure this runs on the event loop. --- .../proxy/connection/client/ConnectedPlayer.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 e646a2179..5f550e5a0 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 @@ -758,7 +758,7 @@ public class ConnectedPlayer implements MinecraftConnectionAssociation, Player { ConnectedPlayer.this, server); connectionInFlight = con; return con.connect().whenCompleteAsync((result, throwable) -> - this.cleanupIfRequired(con)); + this.cleanupIfRequired(con), connection.eventLoop()); }, connection.eventLoop()); }); } From 0cb4c021071a5545d28dbcde23c0876f84fb1590 Mon Sep 17 00:00:00 2001 From: Andrew Steinborn Date: Mon, 25 May 2020 13:09:04 -0400 Subject: [PATCH 7/8] Run all commands on a separate thread pool. --- .../proxy/command/VelocityCommand.java | 2 - .../client/ClientPlaySessionHandler.java | 58 +++++++++++-------- 2 files changed, 34 insertions(+), 26 deletions(-) diff --git a/proxy/src/main/java/com/velocitypowered/proxy/command/VelocityCommand.java b/proxy/src/main/java/com/velocitypowered/proxy/command/VelocityCommand.java index ec245152e..f27b773e4 100644 --- a/proxy/src/main/java/com/velocitypowered/proxy/command/VelocityCommand.java +++ b/proxy/src/main/java/com/velocitypowered/proxy/command/VelocityCommand.java @@ -11,7 +11,6 @@ import com.velocitypowered.api.plugin.PluginDescription; import com.velocitypowered.api.proxy.ProxyServer; import com.velocitypowered.api.util.ProxyVersion; import com.velocitypowered.proxy.VelocityServer; -import java.io.IOException; import java.util.Arrays; import java.util.List; import java.util.Locale; @@ -20,7 +19,6 @@ import java.util.stream.Collectors; import net.kyori.text.TextComponent; import net.kyori.text.event.ClickEvent; import net.kyori.text.event.HoverEvent; -import net.kyori.text.event.HoverEvent.Action; import net.kyori.text.format.TextColor; import net.kyori.text.format.TextDecoration; import org.apache.logging.log4j.LogManager; 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 90e7ee6bb..930dd00df 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 @@ -4,7 +4,7 @@ import static com.velocitypowered.api.network.ProtocolVersion.MINECRAFT_1_13; import static com.velocitypowered.api.network.ProtocolVersion.MINECRAFT_1_8; import static com.velocitypowered.proxy.protocol.util.PluginMessageUtil.constructChannelsPacket; -import com.velocitypowered.api.event.command.CommandExecuteEvent; +import com.velocitypowered.api.event.command.CommandExecuteEvent.CommandResult; import com.velocitypowered.api.event.connection.PluginMessageEvent; import com.velocitypowered.api.event.player.PlayerChatEvent; import com.velocitypowered.api.event.player.PlayerResourcePackStatusEvent; @@ -42,6 +42,7 @@ import java.util.List; import java.util.Optional; import java.util.Queue; import java.util.UUID; +import java.util.concurrent.CompletableFuture; import net.kyori.text.TextComponent; import net.kyori.text.format.TextColor; import org.apache.logging.log4j.LogManager; @@ -124,30 +125,18 @@ public class ClientPlaySessionHandler implements MinecraftSessionHandler { String msg = packet.getMessage(); if (msg.startsWith("/")) { - + String originalCommand = msg.substring(1); server.getCommandManager().callCommandEvent(player, msg.substring(1)) - .thenAcceptAsync(event -> { - CommandExecuteEvent.CommandResult commandResult = event.getResult(); - Optional eventCommand = event.getResult().getCommand(); - String command = eventCommand.orElse(event.getCommand()); - if (commandResult.isForwardToServer()) { - smc.write(Chat.createServerbound("/" + command)); - return; - } - if (commandResult.isAllowed()) { - try { - if (!server.getCommandManager().executeImmediately(player, command)) { - smc.write(Chat.createServerbound("/" + command)); - } - } catch (Exception e) { - logger.info("Exception occurred while running command for {}", player.getUsername(), - e); - player.sendMessage( - TextComponent.of("An error occurred while running this command.", - TextColor.RED)); - } - } - }, smc.eventLoop()); + .thenComposeAsync(event -> processCommandExecuteResult(originalCommand, + event.getResult())) + .exceptionally(e -> { + logger.info("Exception occurred while running command for {}", + player.getUsername(), e); + player.sendMessage( + TextComponent.of("An error occurred while running this command.", + TextColor.RED)); + return null; + }); } else { PlayerChatEvent event = new PlayerChatEvent(player, msg); server.getEventManager().fire(event) @@ -166,6 +155,27 @@ public class ClientPlaySessionHandler implements MinecraftSessionHandler { return true; } + private CompletableFuture processCommandExecuteResult(String originalCommand, + CommandResult result) { + if (result == CommandResult.denied()) { + return CompletableFuture.completedFuture(null); + } + + MinecraftConnection smc = player.ensureAndGetCurrentServer().ensureConnected(); + String commandToRun = result.getCommand().orElse(originalCommand); + if (result.isForwardToServer()) { + return CompletableFuture.runAsync(() -> smc.write(Chat.createServerbound("/" + + commandToRun)), smc.eventLoop()); + } else { + return server.getCommandManager().executeImmediatelyAsync(player, commandToRun) + .thenAcceptAsync(hasRun -> { + if (!hasRun) { + smc.write(Chat.createServerbound("/" + commandToRun)); + } + }, smc.eventLoop()); + } + } + @Override public boolean handle(TabCompleteRequest packet) { boolean isCommand = !packet.isAssumeCommand() && packet.getCommand().startsWith("/"); From abd81a0216ec191a81f1319cbe4bc8054f64b900 Mon Sep 17 00:00:00 2001 From: Andrew Steinborn Date: Mon, 25 May 2020 13:24:41 -0400 Subject: [PATCH 8/8] Fix Checkstyle errors. --- .../client/ClientPlaySessionHandler.java | 42 +++++++++---------- 1 file changed, 21 insertions(+), 21 deletions(-) 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 930dd00df..3affdf9ea 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 @@ -155,27 +155,6 @@ public class ClientPlaySessionHandler implements MinecraftSessionHandler { return true; } - private CompletableFuture processCommandExecuteResult(String originalCommand, - CommandResult result) { - if (result == CommandResult.denied()) { - return CompletableFuture.completedFuture(null); - } - - MinecraftConnection smc = player.ensureAndGetCurrentServer().ensureConnected(); - String commandToRun = result.getCommand().orElse(originalCommand); - if (result.isForwardToServer()) { - return CompletableFuture.runAsync(() -> smc.write(Chat.createServerbound("/" + - commandToRun)), smc.eventLoop()); - } else { - return server.getCommandManager().executeImmediatelyAsync(player, commandToRun) - .thenAcceptAsync(hasRun -> { - if (!hasRun) { - smc.write(Chat.createServerbound("/" + commandToRun)); - } - }, smc.eventLoop()); - } - } - @Override public boolean handle(TabCompleteRequest packet) { boolean isCommand = !packet.isAssumeCommand() && packet.getCommand().startsWith("/"); @@ -485,6 +464,27 @@ public class ClientPlaySessionHandler implements MinecraftSessionHandler { }, player.getConnection().eventLoop()); } + private CompletableFuture processCommandExecuteResult(String originalCommand, + CommandResult result) { + if (result == CommandResult.denied()) { + return CompletableFuture.completedFuture(null); + } + + MinecraftConnection smc = player.ensureAndGetCurrentServer().ensureConnected(); + String commandToRun = result.getCommand().orElse(originalCommand); + if (result.isForwardToServer()) { + return CompletableFuture.runAsync(() -> smc.write(Chat.createServerbound("/" + + commandToRun)), smc.eventLoop()); + } else { + return server.getCommandManager().executeImmediatelyAsync(player, commandToRun) + .thenAcceptAsync(hasRun -> { + if (!hasRun) { + smc.write(Chat.createServerbound("/" + commandToRun)); + } + }, smc.eventLoop()); + } + } + /** * Immediately send any queued messages to the server. */