From cb261c651338572bbba2568632fa96fcfeda5f89 Mon Sep 17 00:00:00 2001 From: Andrew Steinborn Date: Mon, 20 Jul 2020 21:55:33 -0400 Subject: [PATCH] Better handle not failing over on a read timeout The existing Velocity logic was pretty optimistic, hoping the player connection would stay alive long enough to accept a connection to another server. Now, if we notice a read timeout on the server end, we'll treat the disconnect as "unsafe" and disconnect the player immediately. I've added a configuration option to solve the issue in the way BungeeCord does it. This could cause issues with servers that extensively modify the server pipeline and could degrade the user experience, though. Let's try this more conservative and see if it helps, --- .../api/event/player/KickedFromServerEvent.java | 2 +- .../proxy/config/VelocityConfiguration.java | 12 ++++++++++++ .../backend/BackendPlaySessionHandler.java | 14 ++++++++++---- .../client/ClientPlaySessionHandler.java | 4 ++-- .../proxy/connection/client/ConnectedPlayer.java | 5 +++++ .../proxy/connection/util/ConnectionMessages.java | 4 +--- .../proxy/util/VelocityMessages.java | 4 +++- proxy/src/main/resources/default-velocity.toml | 6 ++++++ 8 files changed, 40 insertions(+), 11 deletions(-) diff --git a/api/src/main/java/com/velocitypowered/api/event/player/KickedFromServerEvent.java b/api/src/main/java/com/velocitypowered/api/event/player/KickedFromServerEvent.java index 597b017e1..f37560dd3 100644 --- a/api/src/main/java/com/velocitypowered/api/event/player/KickedFromServerEvent.java +++ b/api/src/main/java/com/velocitypowered/api/event/player/KickedFromServerEvent.java @@ -209,7 +209,7 @@ public final class KickedFromServerEvent implements return LegacyText3ComponentSerializer.get().serialize(message); } - public net.kyori.adventure.text.Component getMessageComponent() { + public net.kyori.adventure.text.@Nullable Component getMessageComponent() { return message; } diff --git a/proxy/src/main/java/com/velocitypowered/proxy/config/VelocityConfiguration.java b/proxy/src/main/java/com/velocitypowered/proxy/config/VelocityConfiguration.java index 32f5ca918..f0f4a99e4 100644 --- a/proxy/src/main/java/com/velocitypowered/proxy/config/VelocityConfiguration.java +++ b/proxy/src/main/java/com/velocitypowered/proxy/config/VelocityConfiguration.java @@ -342,6 +342,10 @@ public class VelocityConfiguration implements ProxyConfig { return advanced.isShowPingRequests(); } + public boolean isFailoverOnUnexpectedServerDisconnect() { + return advanced.isFailoverOnUnexpectedServerDisconnect(); + } + @Override public String toString() { return MoreObjects.toStringHelper(this) @@ -577,6 +581,7 @@ public class VelocityConfiguration implements ProxyConfig { private boolean tcpFastOpen = false; private boolean bungeePluginMessageChannel = true; private boolean showPingRequests = false; + private boolean failoverOnUnexpectedServerDisconnect = false; private Advanced() { } @@ -592,6 +597,8 @@ public class VelocityConfiguration implements ProxyConfig { this.tcpFastOpen = config.getOrElse("tcp-fast-open", false); this.bungeePluginMessageChannel = config.getOrElse("bungee-plugin-message-channel", true); this.showPingRequests = config.getOrElse("show-ping-requests", false); + this.failoverOnUnexpectedServerDisconnect = config + .getOrElse("failover-on-unexpected-server-disconnect", true); } } @@ -631,6 +638,10 @@ public class VelocityConfiguration implements ProxyConfig { return showPingRequests; } + public boolean isFailoverOnUnexpectedServerDisconnect() { + return failoverOnUnexpectedServerDisconnect; + } + @Override public String toString() { return "Advanced{" @@ -643,6 +654,7 @@ public class VelocityConfiguration implements ProxyConfig { + ", tcpFastOpen=" + tcpFastOpen + ", bungeePluginMessageChannel=" + bungeePluginMessageChannel + ", showPingRequests=" + showPingRequests + + ", failoverOnUnexpectedServerDisconnect=" + failoverOnUnexpectedServerDisconnect + '}'; } } 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 2926c52c3..7d653234a 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 @@ -29,6 +29,7 @@ import com.velocitypowered.proxy.protocol.util.PluginMessageUtil; import io.netty.buffer.ByteBuf; import io.netty.buffer.ByteBufUtil; import io.netty.buffer.Unpooled; +import io.netty.handler.timeout.ReadTimeoutException; public class BackendPlaySessionHandler implements MinecraftSessionHandler { @@ -205,7 +206,8 @@ public class BackendPlaySessionHandler implements MinecraftSessionHandler { @Override public void exception(Throwable throwable) { exceptionTriggered = true; - serverConn.getPlayer().handleConnectionException(serverConn.getServer(), throwable, true); + serverConn.getPlayer().handleConnectionException(serverConn.getServer(), throwable, + !(throwable instanceof ReadTimeoutException)); } public VelocityServer getServer() { @@ -216,9 +218,13 @@ public class BackendPlaySessionHandler implements MinecraftSessionHandler { public void disconnected() { serverConn.getServer().removePlayer(serverConn.getPlayer()); if (!serverConn.isGracefulDisconnect() && !exceptionTriggered) { - serverConn.getPlayer().handleConnectionException(serverConn.getServer(), - Disconnect.create(ConnectionMessages.UNEXPECTED_DISCONNECT, - ProtocolVersion.MINECRAFT_1_16), true); + if (server.getConfiguration().isFailoverOnUnexpectedServerDisconnect()) { + serverConn.getPlayer().handleConnectionException(serverConn.getServer(), + Disconnect.create(ConnectionMessages.INTERNAL_SERVER_CONNECTION_ERROR, + ProtocolVersion.MINECRAFT_1_16), true); + } else { + serverConn.getPlayer().disconnect(ConnectionMessages.INTERNAL_SERVER_CONNECTION_ERROR); + } } } } 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 bacb098e1..62a1f9225 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 @@ -31,6 +31,7 @@ import com.velocitypowered.proxy.protocol.packet.TabCompleteResponse; import com.velocitypowered.proxy.protocol.packet.TabCompleteResponse.Offer; import com.velocitypowered.proxy.protocol.packet.TitlePacket; import com.velocitypowered.proxy.protocol.util.PluginMessageUtil; +import com.velocitypowered.proxy.util.VelocityMessages; import io.netty.buffer.ByteBuf; import io.netty.buffer.ByteBufUtil; import io.netty.buffer.Unpooled; @@ -271,8 +272,7 @@ public class ClientPlaySessionHandler implements MinecraftSessionHandler { @Override public void exception(Throwable throwable) { - player.disconnect(TextComponent.of("Your connection has encountered an error. Try again later.", - NamedTextColor.RED)); + player.disconnect(VelocityMessages.GENERIC_CONNECTION_ERROR); } @Override 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 b9dd37992..407658528 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 @@ -566,6 +566,11 @@ public class ConnectedPlayer implements MinecraftConnectionAssociation, Player { // There can't be any connection in flight now. connectionInFlight = null; + if (!isActive()) { + // If the connection is no longer active, it makes no sense to try and recover it. + return; + } + if (event.getResult() instanceof DisconnectPlayer) { DisconnectPlayer res = (DisconnectPlayer) event.getResult(); disconnect(res.getReasonComponent()); diff --git a/proxy/src/main/java/com/velocitypowered/proxy/connection/util/ConnectionMessages.java b/proxy/src/main/java/com/velocitypowered/proxy/connection/util/ConnectionMessages.java index 99bb6350f..86f248297 100644 --- a/proxy/src/main/java/com/velocitypowered/proxy/connection/util/ConnectionMessages.java +++ b/proxy/src/main/java/com/velocitypowered/proxy/connection/util/ConnectionMessages.java @@ -10,9 +10,7 @@ public class ConnectionMessages { public static final TextComponent IN_PROGRESS = TextComponent .of("You are already connecting to a server!", NamedTextColor.RED); public static final TextComponent INTERNAL_SERVER_CONNECTION_ERROR = TextComponent - .of("Internal server connection error"); - public static final TextComponent UNEXPECTED_DISCONNECT = TextComponent - .of("Unexpectedly disconnected from server - crash?"); + .of("An internal server connection error occurred.", NamedTextColor.RED); private ConnectionMessages() { throw new AssertionError(); diff --git a/proxy/src/main/java/com/velocitypowered/proxy/util/VelocityMessages.java b/proxy/src/main/java/com/velocitypowered/proxy/util/VelocityMessages.java index 6990396f8..a85a8b588 100644 --- a/proxy/src/main/java/com/velocitypowered/proxy/util/VelocityMessages.java +++ b/proxy/src/main/java/com/velocitypowered/proxy/util/VelocityMessages.java @@ -15,11 +15,13 @@ public final class VelocityMessages { ) .build(); public static final Component NO_AVAILABLE_SERVERS = TextComponent - .of("No available servers", NamedTextColor.RED); + .of("There are no available servers.", NamedTextColor.RED); public static final Component ALREADY_CONNECTED = TextComponent .of("You are already connected to this proxy!", NamedTextColor.RED); public static final Component MOVED_TO_NEW_SERVER = TextComponent .of("The server you were on kicked you: ", NamedTextColor.RED); + public static final Component GENERIC_CONNECTION_ERROR = TextComponent + .of("An internal error occurred in your connection.", NamedTextColor.RED); private VelocityMessages() { throw new AssertionError(); diff --git a/proxy/src/main/resources/default-velocity.toml b/proxy/src/main/resources/default-velocity.toml index a83ddba94..a3c31db10 100644 --- a/proxy/src/main/resources/default-velocity.toml +++ b/proxy/src/main/resources/default-velocity.toml @@ -117,6 +117,12 @@ bungee-plugin-message-channel = true # Shows ping requests to the proxy from clients. show-ping-requests = false +# By default, Velocity will attempt to gracefully handle situations where the user unexpectedly +# loses connection to the server without an explicit disconnect message by attempting to fall the +# user back, except in the case of read timeouts. BungeeCord will disconnect the user instead. You +# can disable this setting to use the BungeeCord behavior. +failover-on-unexpected-server-disconnect = true + [query] # Whether to enable responding to GameSpy 4 query responses or not. enabled = false