Conversation
| + this.cserver.getPluginManager().callEvent(playerQuitEvent); | ||
| + player.getBukkitEntity().disconnect(); | ||
| + | ||
| + if (this.server.isSameThread()) player.doTick(); // SPIGOT-924 // Paper - Improved watchdog support; don't tick during emergency shutdowns |
There was a problem hiding this comment.
This change will require more testing. But, this prevents the final "drop" from occuring.
| @@ -80,14 +78,6 @@ | |||
| @Override | |||
| public boolean isAcceptingMessages() { | |||
| return this.connection.isConnected(); | |||
| @@ -95,6 +_,7 @@ | |||
| LOGGER.info("Disconnecting {}: {}", this.getUserName(), reason.getString()); | |||
| this.connection.send(new ClientboundLoginDisconnectPacket(reason)); | |||
| this.connection.disconnect(reason); | |||
| + this.disconnecting = true; // Paper - Fix disconnect still ticking login | |||
| } catch (Exception var3) { | |||
| LOGGER.error("Error whilst disconnecting player", (Throwable)var3); | |||
| } | |||
There was a problem hiding this comment.
This was required because of the fact that Connection#disconnect did not block. This would cause the netty thread to continue ticking whilst disconnected.
As mentioned before, this seems to only be called on netty threads, so it should be okay to let it block.
| + | ||
| + public final boolean isDisconnected() { | ||
| + return (!this.player.joining && !this.connection.isConnected()) || this.processedDisconnect; // Paper - Fix duplication bugs | ||
| + return (!this.player.joining && !this.connection.isConnected()); |
There was a problem hiding this comment.
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.
| + new ClientboundDisconnectPacket(disconnectionDetails.reason()), | ||
| + PacketSendListener.thenRun(() -> this.connection.disconnect(disconnectionDetails)) | ||
| + ); | ||
| + this.onDisconnect(disconnectionDetails); |
There was a problem hiding this comment.
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.
| - } | ||
| + // 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 |
There was a problem hiding this comment.
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.
| + connection.handleDisconnection(); | ||
| + return; | ||
| + } | ||
| + // Paper end - Force kill connection ticking |
There was a problem hiding this comment.
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.
| 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 |
There was a problem hiding this comment.
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.isConnected()) { | ||
| - this.channel.close().awaitUninterruptibly(); | ||
| + this.channel.close(); // We can't wait as this may be called from an event loop. |
There was a problem hiding this comment.
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.
| @@ -204,7 +198,7 @@ index 079ab378920c0e52ef4e42ef20b37bd389f40870..b8a4b4cc02a2fc6b70f4b840796eed50 | |||
| - this.keepAliveChallenge = millis; | |||
| - this.send(new ClientboundKeepAlivePacket(this.keepAliveChallenge)); | |||
| + // Paper start - improve keepalives | |||
| + if (this.checkIfClosed(millis) && !this.processedDisconnect) { | |||
| + if (this.checkIfClosed(millis)) { | |||
| + long currTime = System.nanoTime(); | |||
| + | |||
There was a problem hiding this comment.
This wont be called when player is getting disconnected anyways?
kennytv
added
the
status: blocked
Replaces the processedDisconnect flag and executeBlocking cleanup with a force kill flag processed by the connection listener tick. executeBlocking(connection::handleDisconnection) did not actually Work, as the the channel is still open at that point because the disconnect packet hasn't been flushed. Replaced with handleConnectionDisconnectOnNextTick, which the connection listener processes on the next tick. processedDisconnect was an old flag preventing double disconnect. The same protection is now provided by vanilla's disconnectionHandled check on Connection. Drops the SPIGOT-924 player.doTick() call on quit. This is an ancient spigot bug fix that causes issues and weird event fires whilst the player is disconnected. Removes the login side disconnecting flag and the matching tick guard, since the connection now closes properly on its own. The login disconnect path moves to disconnectAsync so it doesn't block on IO.
Owen1212055
force-pushed
the
feat/disconnect-state-fixes
branch
from
f63592f to
83b6b28
Compare
Owen1212055
removed
the
status: blocked
github-project-automation
Bot
moved this from Awaiting review
to Merged
in Paper PR Queue
2 participants
No description provided.