Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -267,7 +267,7 @@ index 35b8a2bddeefff2b1ba8ed75ec780c41d59ce92f..1baa8daf880c5f87f1d72ecf0e1b93c7
+ // Paper end - Use Velocity cipher
}
diff --git a/net/minecraft/network/Connection.java b/net/minecraft/network/Connection.java
index 8817a2d6500d1bdce96d87dc8f7f364ccfa390e5..19ec939529eb638bdc4d7fd9260f161fae118314 100644
index 57a0c5696a5ddacaa9908650a9f17ec0c6036c22..0449faff49b857eec4bc45c46f4b3348e7eb9532 100644
--- a/net/minecraft/network/Connection.java
+++ b/net/minecraft/network/Connection.java
@@ -728,11 +728,22 @@ public class Connection extends SimpleChannelInboundHandler<Packet<?>> {
Expand Down Expand Up @@ -335,10 +335,10 @@ index 309845b28306dfa5946e79b80e5b1dde82dd68cd..33e1cdb00d648c992af03a9768992531
.add(
new ServerBootstrap()
diff --git a/net/minecraft/server/network/ServerLoginPacketListenerImpl.java b/net/minecraft/server/network/ServerLoginPacketListenerImpl.java
index ec5bd4568872c48addd26dd652868e0e5df57038..31bc0a105152a7f24a4126542bea7dbebc3e037c 100644
index 7699f3022402b19ac57f40e586628c68bf646aad..4f00ad88fa268bd1a1ebd412cfa4d4c5e08f3ca8 100644
--- a/net/minecraft/server/network/ServerLoginPacketListenerImpl.java
+++ b/net/minecraft/server/network/ServerLoginPacketListenerImpl.java
@@ -245,11 +245,9 @@ public class ServerLoginPacketListenerImpl implements ServerLoginPacketListener,
@@ -242,11 +242,9 @@ public class ServerLoginPacketListenerImpl implements ServerLoginPacketListener,
}

SecretKey secretKey = packet.getSecretKey(_private);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,18 +6,18 @@ Subject: [PATCH] Optimise collision checking in player move packet handling
Move collision logic to just the hasNewCollision call instead of getCubes + hasNewCollision

diff --git a/net/minecraft/server/network/ServerGamePacketListenerImpl.java b/net/minecraft/server/network/ServerGamePacketListenerImpl.java
index 214613e9422606b7b1f37716fc060db63c38849a..e158d614abed8d16e80192c0c9abd8537c92b9dc 100644
index d418edb869e960d68f8788bd39b77fa7c47fc4a7..3c2e9e6a1218d38573405a2747bee2fd66dc12fc 100644
--- a/net/minecraft/server/network/ServerGamePacketListenerImpl.java
+++ b/net/minecraft/server/network/ServerGamePacketListenerImpl.java
@@ -624,6 +624,7 @@ public class ServerGamePacketListenerImpl
@@ -623,6 +623,7 @@ public class ServerGamePacketListenerImpl
}

rootVehicle.move(MoverType.PLAYER, new Vec3(d3, d4, d5));
+ final boolean didCollide = toX != rootVehicle.getX() || toY != rootVehicle.getY() || toZ != rootVehicle.getZ(); // Paper - needed here as the difference in Y can be reset - also note: this is only a guess at whether collisions took place, floating point errors can make this true when it shouldn't be...
double verticalDelta = d4;
d3 = d - rootVehicle.getX();
d4 = d1 - rootVehicle.getY();
@@ -635,12 +636,21 @@ public class ServerGamePacketListenerImpl
@@ -634,12 +635,21 @@ public class ServerGamePacketListenerImpl
d7 = d3 * d3 + d4 * d4 + d5 * d5;
boolean flag1 = false;
if (d7 > org.spigotmc.SpigotConfig.movedWronglyThreshold) { // Spigot
Expand All @@ -42,7 +42,7 @@ index 214613e9422606b7b1f37716fc060db63c38849a..e158d614abed8d16e80192c0c9abd853
rootVehicle.absSnapTo(x, y, z, f, f1);
this.send(ClientboundMoveVehiclePacket.fromEntity(rootVehicle));
rootVehicle.removeLatestMovementRecording();
@@ -719,9 +729,32 @@ public class ServerGamePacketListenerImpl
@@ -718,9 +728,32 @@ public class ServerGamePacketListenerImpl
}

private boolean noBlocksAround(Entity entity) {
Expand Down Expand Up @@ -78,7 +78,7 @@ index 214613e9422606b7b1f37716fc060db63c38849a..e158d614abed8d16e80192c0c9abd853
}

@Override
@@ -1503,7 +1536,7 @@ public class ServerGamePacketListenerImpl
@@ -1502,7 +1535,7 @@ public class ServerGamePacketListenerImpl
}
}

Expand All @@ -87,15 +87,15 @@ index 214613e9422606b7b1f37716fc060db63c38849a..e158d614abed8d16e80192c0c9abd853
d3 = d - this.lastGoodX; // Paper - diff on change, used for checking large move vectors above
d4 = d1 - this.lastGoodY; // Paper - diff on change, used for checking large move vectors above
d5 = d2 - this.lastGoodZ; // Paper - diff on change, used for checking large move vectors above
@@ -1542,6 +1575,7 @@ public class ServerGamePacketListenerImpl
@@ -1541,6 +1574,7 @@ public class ServerGamePacketListenerImpl
boolean flag1 = this.player.verticalCollisionBelow;
this.player.move(MoverType.PLAYER, new Vec3(d3, d4, d5));
this.player.onGround = packet.isOnGround(); // CraftBukkit - SPIGOT-5810, SPIGOT-5835, SPIGOT-6828: reset by this.player.move
+ final boolean didCollide = toX != this.player.getX() || toY != this.player.getY() || toZ != this.player.getZ(); // Paper - needed here as the difference in Y can be reset - also note: this is only a guess at whether collisions took place, floating point errors can make this true when it shouldn't be...
// Paper start - prevent position desync
if (this.awaitingPositionFromClient != null) {
return; // ... thanks Mojang for letting move calls teleport across dimensions.
@@ -1575,7 +1609,17 @@ public class ServerGamePacketListenerImpl
@@ -1574,7 +1608,17 @@ public class ServerGamePacketListenerImpl
}

// Paper start - Add fail move event
Expand All @@ -114,7 +114,7 @@ index 214613e9422606b7b1f37716fc060db63c38849a..e158d614abed8d16e80192c0c9abd853
if (!allowMovement) {
io.papermc.paper.event.player.PlayerFailMoveEvent event = fireFailMove(io.papermc.paper.event.player.PlayerFailMoveEvent.FailReason.CLIPPED_INTO_BLOCK,
toX, toY, toZ, toYaw, toPitch, false);
@@ -1710,7 +1754,7 @@ public class ServerGamePacketListenerImpl
@@ -1709,7 +1753,7 @@ public class ServerGamePacketListenerImpl

private boolean updateAwaitingTeleport() {
if (this.awaitingPositionFromClient != null) {
Expand All @@ -123,7 +123,7 @@ index 214613e9422606b7b1f37716fc060db63c38849a..e158d614abed8d16e80192c0c9abd853
this.awaitingTeleportTime = this.tickCount;
this.teleport(
this.awaitingPositionFromClient.x,
@@ -1729,6 +1773,34 @@ public class ServerGamePacketListenerImpl
@@ -1728,6 +1772,34 @@ public class ServerGamePacketListenerImpl
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,7 @@ index 21d50675bfe90c2276890779eb23de58ac915b9a..7be34e37562875313b8e43357921b5fe
}
}
diff --git a/net/minecraft/server/network/ServerCommonPacketListenerImpl.java b/net/minecraft/server/network/ServerCommonPacketListenerImpl.java
index 079ab378920c0e52ef4e42ef20b37bd389f40870..b8a4b4cc02a2fc6b70f4b840796eed501aad6239 100644
index 045fe927ddf47191c9e8e3f0febe2cf2aaa90126..d083bb6f45ec2dadd5607d1dd1f6e13da2749b74 100644
--- a/net/minecraft/server/network/ServerCommonPacketListenerImpl.java
+++ b/net/minecraft/server/network/ServerCommonPacketListenerImpl.java
@@ -39,12 +39,13 @@ public abstract class ServerCommonPacketListenerImpl implements ServerCommonPack
Expand All @@ -121,7 +121,7 @@ index 079ab378920c0e52ef4e42ef20b37bd389f40870..b8a4b4cc02a2fc6b70f4b840796eed50
private volatile boolean suspendFlushingOnServerThread = false;
// CraftBukkit start
public final org.bukkit.craftbukkit.CraftServer cserver;
@@ -61,13 +62,14 @@ public abstract class ServerCommonPacketListenerImpl implements ServerCommonPack
@@ -60,13 +61,14 @@ public abstract class ServerCommonPacketListenerImpl implements ServerCommonPack
public ServerCommonPacketListenerImpl(MinecraftServer server, Connection connection, CommonListenerCookie cookie) {
this.server = server;
this.connection = connection;
Expand All @@ -137,7 +137,7 @@ index 079ab378920c0e52ef4e42ef20b37bd389f40870..b8a4b4cc02a2fc6b70f4b840796eed50
// Paper end
}

@@ -100,13 +102,41 @@ public abstract class ServerCommonPacketListenerImpl implements ServerCommonPack
@@ -99,13 +101,35 @@ public abstract class ServerCommonPacketListenerImpl implements ServerCommonPack

@Override
public void handleKeepAlive(ServerboundKeepAlivePacket packet) {
Expand Down Expand Up @@ -167,25 +167,19 @@ index 079ab378920c0e52ef4e42ef20b37bd389f40870..b8a4b4cc02a2fc6b70f4b840796eed50
+ if (ka.challengeId() == packet.getId()) {
+ itr.remove();
+
+ if (!this.processedDisconnect) {
+ LOGGER.info("Disconnecting {} for sending keepalive response ({}) out-of-order!", this.playerProfile().name(), packet.getId());
+ this.disconnectAsync(TIMEOUT_DISCONNECTION_MESSAGE, io.papermc.paper.connection.DisconnectionReason.TIMEOUT);
+ return;
+ }
+ break;
+ LOGGER.info("Disconnecting {} for sending keepalive response ({}) out-of-order!", this.playerProfile().name(), packet.getId());
+ this.disconnectAsync(TIMEOUT_DISCONNECTION_MESSAGE, io.papermc.paper.connection.DisconnectionReason.TIMEOUT);
+ return;
+ }
}
+
+ if (!this.processedDisconnect) {
+ LOGGER.info("Disconnecting {} for sending keepalive response ({}) without matching challenge!", this.playerProfile().name(), packet.getId());
+ this.disconnectAsync(TIMEOUT_DISCONNECTION_MESSAGE, io.papermc.paper.connection.DisconnectionReason.TIMEOUT);
+ return;
+ }
+ LOGGER.info("Disconnecting {} for sending keepalive response ({}) without matching challenge!", this.playerProfile().name(), packet.getId());
+ this.disconnectAsync(TIMEOUT_DISCONNECTION_MESSAGE, io.papermc.paper.connection.DisconnectionReason.TIMEOUT);
+ // Paper end - improve keepalives
}

@Override
@@ -233,20 +263,23 @@ public abstract class ServerCommonPacketListenerImpl implements ServerCommonPack
@@ -232,20 +256,23 @@ public abstract class ServerCommonPacketListenerImpl implements ServerCommonPack
protected void keepConnectionAlive() {
Profiler.get().push("keepAlive");
long millis = Util.getMillis();
Expand All @@ -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();
+
+ if ((currTime - this.keepAlive.lastKeepAliveTx) >= java.util.concurrent.TimeUnit.SECONDS.toNanos(1L)) {
Expand All @@ -223,7 +217,7 @@ index 079ab378920c0e52ef4e42ef20b37bd389f40870..b8a4b4cc02a2fc6b70f4b840796eed50
}
}

@@ -427,6 +460,6 @@ public abstract class ServerCommonPacketListenerImpl implements ServerCommonPack
@@ -424,6 +451,6 @@ public abstract class ServerCommonPacketListenerImpl implements ServerCommonPack
}

protected CommonListenerCookie createCookie(ClientInformation clientInformation) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,11 +12,12 @@
private volatile @Nullable PacketListener disconnectListener;
private volatile @Nullable PacketListener packetListener;
private @Nullable DisconnectionDetails disconnectionDetails;
@@ -81,6 +_,43 @@
@@ -81,6 +_,44 @@
private boolean handlingFault;
private volatile @Nullable DisconnectionDetails delayedDisconnect;
@Nullable BandwidthDebugMonitor bandwidthDebugMonitor;
+ public String hostname = ""; // CraftBukkit - add field
+ public boolean killConnectionOnNextTick = false; // Paper - Force kill connection ticking
+ // Paper start - NetworkClient implementation
+ public int protocolVersion;
+ public java.net.InetSocketAddress virtualHost;
Expand Down Expand Up @@ -218,21 +219,14 @@
}

if (this.tickCount++ % 20 == 0) {
@@ -393,12 +_,13 @@
@@ -393,6 +_,7 @@
}

public void disconnect(DisconnectionDetails disconnectionDetails) {
+ this.preparing = false; // Spigot
if (this.channel == null) {
this.delayedDisconnect = disconnectionDetails;
}

if (this.isConnected()) {
- this.channel.close().awaitUninterruptibly();
+ this.channel.close(); // We can't wait as this may be called from an event loop.
Copy link
Member Author

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.

this.disconnectionDetails = disconnectionDetails;
}
}
@@ -533,6 +_,13 @@
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@
}
}

@@ -44,8 +_,29 @@
@@ -44,8 +_,28 @@
this.closed = true;
}

Expand All @@ -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
Copy link
Member Author

Choose a reason for hiding this comment

The 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);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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());
Expand All @@ -219,7 +218,7 @@
if (packet.isTerminal()) {
this.close();
}
@@ -174,19 +_,114 @@
@@ -174,19 +_,112 @@
}
}

Expand All @@ -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()) {
Expand Down Expand Up @@ -312,12 +311,10 @@
+ new ClientboundDisconnectPacket(disconnectionDetails.reason()),
+ PacketSendListener.thenRun(() -> this.connection.disconnect(disconnectionDetails))
+ );
+ this.onDisconnect(disconnectionDetails);
Copy link
Member Author

@Owen1212055 Owen1212055 Feb 8, 2026

Choose a reason for hiding this comment

The 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
Copy link
Member Author

Choose a reason for hiding this comment

The 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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,7 @@
public SocketAddress startMemoryChannel() {
ChannelFuture channelFuture;
synchronized (this.channels) {
@@ -141,6 +_,13 @@
@@ -141,12 +_,26 @@

public void tick() {
synchronized (this.connections) {
Expand All @@ -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
Copy link
Member Author

Choose a reason for hiding this comment

The 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();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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();
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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());
Copy link
Member Author

Choose a reason for hiding this comment

The 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
Expand Down
Loading
Loading