Conversation
This seems to be a change based on some ancient code. Execute blocking is a bit of a misleading name, but if the disconnect is currently on the main thread, we will execute it right away, else we will block the thread until its done. However, that method WILL never be called async due to the callsite (disconnect) delegating it to the main thread and blocking already. This now brings back the behavior of disconnect being processed right away, rather than later.
…nnect from being called multiple times. This is no longer an issue, as onDisconnect is now guarded by a vanilla disconnectionHandled check. However, this does require we properly wait for the connection to close. I am not sure about the best solution for that, as alternatively we can possibly treat the connection has inactive on a custom flag set. Also remove the hack needed on server login, as we now properly disconnect the connection.
This has two effects: - Most notably, the old executeBlocking did not actually work as the connection is typically still open at this stage. This is because it waits for disconnect to be sent before actually closing the connection. - We will now mark the connection to be killed on disconnect, where the forceKill will cause the connection to be cleaned up when its ticked. This seems like a vanilla bug and should be reported. This also removes the old ontick on disconnect. As that caused some things to incorrectly complete on tick anyways. The original bug is not applicable anymore.
| + 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?
1 participant
No description provided.