-
-
Notifications
You must be signed in to change notification settings - Fork 3.3k
Disconnect State Fixes #13616
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Disconnect State Fixes #13616
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -43,7 +43,7 @@ | |
| } | ||
| } | ||
|
|
||
| @@ -44,8 +_,29 @@ | ||
| @@ -44,8 +_,28 @@ | ||
| this.closed = true; | ||
| } | ||
|
|
||
|
|
@@ -69,7 +69,6 @@ | |
| public void handle() { | ||
| + packetProcessing.push(this.listener); // Paper - detailed watchdog information | ||
| + try { // Paper - detailed watchdog information | ||
| + if (this.listener instanceof net.minecraft.server.network.ServerCommonPacketListenerImpl serverCommonPacketListener && serverCommonPacketListener.processedDisconnect) return; // Paper - Don't handle sync packets for kicked players | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This one I am possibly thinking of keeping, I am not sure however. Because this means that some previously read packets that were captured BEFORE disconnect could possibly be dropped? This doesn't exactly make sense. |
||
| if (this.listener.shouldHandleMessage(this.packet)) { | ||
| try { | ||
| this.packet.handle(this.listener); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9,13 +9,12 @@ | |
| private final boolean transferred; | ||
| private long keepAliveTime; | ||
| private boolean keepAlivePending; | ||
| @@ -46,6 +_,17 @@ | ||
| @@ -46,6 +_,16 @@ | ||
| private boolean closed = false; | ||
| private int latency; | ||
| private volatile boolean suspendFlushingOnServerThread = false; | ||
| + // CraftBukkit start | ||
| + public final org.bukkit.craftbukkit.CraftServer cserver; | ||
| + public boolean processedDisconnect; | ||
| + // CraftBukkit end | ||
| + public final java.util.Map<java.util.UUID, net.kyori.adventure.resource.ResourcePackCallback> packCallbacks = new java.util.concurrent.ConcurrentHashMap<>(); // Paper - adventure resource pack callbacks | ||
| + private static final long KEEPALIVE_LIMIT = Long.getLong("paper.playerconnection.keepalive", 30) * 1000; // Paper - provide property to set keepalive limit | ||
|
|
@@ -210,7 +209,7 @@ | |
|
|
||
| public void send(Packet<?> packet, @Nullable ChannelFutureListener sendListener) { | ||
| + // CraftBukkit start | ||
| + if (packet == null || this.processedDisconnect) { // Spigot | ||
| + if (packet == null) { | ||
| + return; | ||
| + } else if (packet instanceof net.minecraft.network.protocol.game.ClientboundSetDefaultSpawnPositionPacket defaultSpawnPositionPacket && this instanceof ServerGamePacketListenerImpl serverGamePacketListener) { | ||
| + serverGamePacketListener.player.compassTarget = org.bukkit.craftbukkit.util.CraftLocation.toBukkit(defaultSpawnPositionPacket.respawnData().pos(), serverGamePacketListener.getPlayer().level()); | ||
|
|
@@ -219,7 +218,7 @@ | |
| if (packet.isTerminal()) { | ||
| this.close(); | ||
| } | ||
| @@ -174,19 +_,114 @@ | ||
| @@ -174,19 +_,112 @@ | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -241,7 +240,7 @@ | |
| - new ClientboundDisconnectPacket(disconnectionDetails.reason()), | ||
| - PacketSendListener.thenRun(() -> this.connection.disconnect(disconnectionDetails)) | ||
| + // CraftBukkit start - fire PlayerKickEvent | ||
| + if (this.processedDisconnect) { | ||
| + if (!this.connection.isConnected()) { | ||
| + return; | ||
| + } | ||
| + if (!this.cserver.isPrimaryThread()) { | ||
|
|
@@ -312,12 +311,10 @@ | |
| + new ClientboundDisconnectPacket(disconnectionDetails.reason()), | ||
| + PacketSendListener.thenRun(() -> this.connection.disconnect(disconnectionDetails)) | ||
| + ); | ||
| + this.onDisconnect(disconnectionDetails); | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. THIS IS REALLY BAD! We are allowing for the player to be removed while their connection is not yet closed. This is EXACTLY why the old boolean was needed, because it was always firing multiple times. |
||
| this.connection.setReadOnly(); | ||
| - this.server.executeBlocking(this.connection::handleDisconnection); | ||
| - } | ||
| + // CraftBukkit - Don't wait | ||
| + this.server.scheduleOnMain(this.connection::handleDisconnection); // Paper | ||
| + this.connection.killConnectionOnNextTick = true; // Paper - Force kill connection ticking. Let this close the connection | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is bugged in vanilla, and requires us to use our new killConnectionOnNextTick field. Basically, the vanilla behavior is broken, as handleDisconnection always will early return because it is waiting for the result of ClientboundDisconnectPacket before killing the connection. So, we instead mark the connection to be killed. The scheduleOnMain logic technically may have improved this, but this change can be dropped for this solution instead. |
||
| + } | ||
| + | ||
| + // Paper start - add proper async disconnect | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -100,7 +100,7 @@ | |
| public SocketAddress startMemoryChannel() { | ||
| ChannelFuture channelFuture; | ||
| synchronized (this.channels) { | ||
| @@ -141,6 +_,13 @@ | ||
| @@ -141,12 +_,26 @@ | ||
|
|
||
| public void tick() { | ||
| synchronized (this.connections) { | ||
|
|
@@ -114,6 +114,19 @@ | |
| Iterator<Connection> iterator = this.connections.iterator(); | ||
|
|
||
| while (iterator.hasNext()) { | ||
| Connection connection = iterator.next(); | ||
| if (!connection.isConnecting()) { | ||
| if (connection.isConnected()) { | ||
| + // Paper start - Force kill connection ticking | ||
| + // also call this multiple times, doesnt matter | ||
| + if (connection.killConnectionOnNextTick) { | ||
| + connection.handleDisconnection(); | ||
| + return; | ||
| + } | ||
| + // Paper end - Force kill connection ticking | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See above, but this is part of the vanilla bug fix. This allows us to say we are ready to kill the connection, and that if the connection is still alive, handle disconnection early. It is supposed to call this multiple times. |
||
| try { | ||
| connection.tick(); | ||
| } catch (Exception var7) { | ||
| @@ -160,6 +_,7 @@ | ||
| connection.setReadOnly(); | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -83,7 +83,7 @@ | |
|
|
||
| public ServerGamePacketListenerImpl(MinecraftServer server, Connection connection, ServerPlayer player, CommonListenerCookie cookie) { | ||
| super(server, connection, cookie); | ||
| @@ -276,11 +_,26 @@ | ||
| @@ -276,8 +_,22 @@ | ||
| player.connection = this; | ||
| player.getTextFilter().join(); | ||
| this.signedMessageDecoder = SignedMessageChain.Decoder.unsigned(player.getUUID(), server::enforceSecureProfile); | ||
|
|
@@ -108,10 +108,6 @@ | |
|
|
||
| @Override | ||
| public void tick() { | ||
| + if (this.isDisconnected()) return; // Paper | ||
| if (this.ackBlockChangesUpTo > -1) { | ||
| this.send(new ClientboundBlockChangedAckPacket(this.ackBlockChangesUpTo)); | ||
| this.ackBlockChangesUpTo = -1; | ||
| @@ -290,11 +_,13 @@ | ||
| this.keepConnectionAlive(); | ||
| this.chatSpamThrottler.tick(); | ||
|
|
@@ -1363,17 +1359,9 @@ | |
| return; | ||
| } | ||
| } | ||
| @@ -1395,24 +_,50 @@ | ||
|
|
||
| @@ -1396,23 +_,42 @@ | ||
| @Override | ||
| public void onDisconnect(DisconnectionDetails details) { | ||
| + // CraftBukkit start - Rarely it would send a disconnect line twice | ||
| + if (this.processedDisconnect) { | ||
| + return; | ||
| + } else { | ||
| + this.processedDisconnect = true; | ||
| + } | ||
| + // CraftBukkit end | ||
| LOGGER.info("{} lost connection: {}", this.player.getPlainTextName(), details.reason().getString()); | ||
| - this.removePlayerFromWorld(); | ||
| + // Paper start - Fix kick event leave message not being sent | ||
|
|
@@ -2682,7 +2670,7 @@ | |
| + } | ||
| + | ||
| + public final boolean isDisconnected() { | ||
| + return (!this.player.joining && !this.connection.isConnected()) || this.processedDisconnect; // Paper - Fix duplication bugs | ||
| + return (!this.player.joining && !this.connection.isConnected()); | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This one looks scary: However, processedDisconnect was only set to true when onDisconnect was invoked. A previous change could have caused a player to be disconnected whilst they were not disconnected, however, that is no longer the case. The connection WILL be closed when onDisconnect is called, thus, isDisconnected should remain true. |
||
| + } | ||
| + | ||
| + @Override | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See below:
But, this should only be called from netty threads. Please check if this safe. Because I am not sure of the exact issue of calling this from an event loop.