Upstream/Paper
1
0
Fork 0
mirror of https://github.com/PaperMC/Paper.git synced 2025-01-01 05:47:45 +01:00
Code Issues Projects Releases Wiki Activity

Fix packet duplicating at some points (#8566)

Browse Source 
Due to the weakly consistent of ConcurrentLinkedQueue iterator, at some points, packet will be resent twice times or more, causing some weird behaviors (e.g. kicked for illegal movement since the same ClientboundPlayerPositionPacket was sent two times). This changes for the patch add a flag for marking if the packet was consumed to prevent such issue and ensure consistently of the packet queue.
This commit is contained in:
sandtechnology 2022-11-28 00:36:35 +08:00
parent 59d8df6cba
commit e9b9c0b332
2 changed files with 45 additions and 17 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

11
patches/server/Allow-controlled-flushing-for-network-manager.patch
View File

@ -129,6 +129,8 @@ index 0000000000000000000000000000000000000000..00000000000000000000000000000000
return true; return true;
} }
@@ -0,0 +0,0 @@ public class Connection extends SimpleChannelInboundHandler<Packet<?>> {
Packet<?> packet = queued.packet; Packet<?> packet = queued.packet;
if (!packet.isReady()) { if (!packet.isReady()) {
+ // Paper start - make only one flush call per sendPacketQueue() call + // Paper start - make only one flush call per sendPacketQueue() call
@ -139,9 +141,10 @@ index 0000000000000000000000000000000000000000..00000000000000000000000000000000
return false; return false;
} else { } else {
iterator.remove(); iterator.remove();
- this.sendPacket(packet, queued.listener); if (queued.tryMarkConsumed()) { // Paper - try to mark isConsumed flag for de-duplicating packet
+ this.sendPacket(packet, queued.listener, (!iterator.hasNext() && (needsFlush || this.canFlush)) ? Boolean.TRUE : Boolean.FALSE); // Paper - make only one flush call per sendPacketQueue() call - this.sendPacket(packet, queued.listener);
+ hasWrotePacket = true; // Paper - make only one flush call per sendPacketQueue() call + this.sendPacket(packet, queued.listener, (!iterator.hasNext() && (needsFlush || this.canFlush)) ? Boolean.TRUE : Boolean.FALSE); // Paper - make only one flush call per sendPacketQueue() call
+ hasWrotePacket = true; // Paper - make only one flush call per sendPacketQueue() call
}
} }
} }
return true;

51
patches/server/Optimize-Network-Manager-and-add-advanced-packet-sup.patch
View File

@ -25,7 +25,7 @@ Also adds Netty Channel Flush Consolidation to reduce the amount of flushing
Also avoids spamming closed channel exception by rechecking closed state in dispatch Also avoids spamming closed channel exception by rechecking closed state in dispatch
and then catch exceptions and close if they fire. and then catch exceptions and close if they fire.
Part of this commit was authored by: Spottedleaf Part of this commit was authored by: Spottedleaf, sandtechnology
diff --git a/src/main/java/net/minecraft/network/Connection.java b/src/main/java/net/minecraft/network/Connection.java diff --git a/src/main/java/net/minecraft/network/Connection.java b/src/main/java/net/minecraft/network/Connection.java
index 0000000000000000000000000000000000000000..0000000000000000000000000000000000000000 100644 index 0000000000000000000000000000000000000000..0000000000000000000000000000000000000000 100644
@ -185,10 +185,6 @@ index 0000000000000000000000000000000000000000..00000000000000000000000000000000
} }
- private void flushQueue() { - private void flushQueue() {
- try { // Paper - add pending task queue
- if (this.channel != null && this.channel.isOpen()) {
- Queue queue = this.queue;
-
+ // Paper start - rewrite this to be safer if ran off main thread + // Paper start - rewrite this to be safer if ran off main thread
+ private boolean flushQueue() { // void -> boolean + private boolean flushQueue() { // void -> boolean
+ if (!isConnected()) { + if (!isConnected()) {
@ -198,19 +194,16 @@ index 0000000000000000000000000000000000000000..00000000000000000000000000000000
+ return processQueue(); + return processQueue();
+ } else if (isPending) { + } else if (isPending) {
+ // Should only happen during login/status stages + // Should only happen during login/status stages
synchronized (this.queue) { + synchronized (this.queue) {
- Connection.PacketHolder networkmanager_queuedpacket;
-
- while ((networkmanager_queuedpacket = (Connection.PacketHolder) this.queue.poll()) != null) {
- this.sendPacket(networkmanager_queuedpacket.packet, networkmanager_queuedpacket.listener);
- }
+ return this.processQueue(); + return this.processQueue();
+ } + }
+ } + }
+ return false; + return false;
+ } + }
+ private boolean processQueue() { + private boolean processQueue() {
+ try { // Paper - add pending task queue try { // Paper - add pending task queue
- if (this.channel != null && this.channel.isOpen()) {
- Queue queue = this.queue;
+ if (this.queue.isEmpty()) return true; + if (this.queue.isEmpty()) return true;
+ // If we are on main, we are safe here in that nothing else should be processing queue off main anymore + // If we are on main, we are safe here in that nothing else should be processing queue off main anymore
+ // But if we are not on main due to login/status, the parent is synchronized on packetQueue + // But if we are not on main due to login/status, the parent is synchronized on packetQueue
@ -223,12 +216,25 @@ index 0000000000000000000000000000000000000000..00000000000000000000000000000000
+ return true; + return true;
+ } + }
- synchronized (this.queue) {
- Connection.PacketHolder networkmanager_queuedpacket;
+ // Paper start - checking isConsumed flag and skipping packet sending
+ if (queued.isConsumed()) {
+ continue;
+ }
+ // Paper end - checking isConsumed flag and skipping packet sending
- while ((networkmanager_queuedpacket = (Connection.PacketHolder) this.queue.poll()) != null) {
- this.sendPacket(networkmanager_queuedpacket.packet, networkmanager_queuedpacket.listener);
+ Packet<?> packet = queued.packet; + Packet<?> packet = queued.packet;
+ if (!packet.isReady()) { + if (!packet.isReady()) {
+ return false; + return false;
+ } else { + } else {
+ iterator.remove(); + iterator.remove();
+ this.sendPacket(packet, queued.listener); + if (queued.tryMarkConsumed()) { // Paper - try to mark isConsumed flag for de-duplicating packet
+ this.sendPacket(packet, queued.listener);
}
-
} }
} }
+ return true; + return true;
@ -284,6 +290,25 @@ index 0000000000000000000000000000000000000000..00000000000000000000000000000000
// Paper start - Add PlayerConnectionCloseEvent // Paper start - Add PlayerConnectionCloseEvent
final PacketListener packetListener = this.getPacketListener(); final PacketListener packetListener = this.getPacketListener();
if (packetListener instanceof net.minecraft.server.network.ServerGamePacketListenerImpl) { if (packetListener instanceof net.minecraft.server.network.ServerGamePacketListenerImpl) {
@@ -0,0 +0,0 @@ public class Connection extends SimpleChannelInboundHandler<Packet<?>> {
@Nullable
final PacketSendListener listener;
+ // Paper start - isConsumed flag for the connection
+ private java.util.concurrent.atomic.AtomicBoolean isConsumed = new java.util.concurrent.atomic.AtomicBoolean(false);
+
+ public boolean tryMarkConsumed() {
+ return isConsumed.compareAndSet(false, true);
+ }
+
+ public boolean isConsumed() {
+ return isConsumed.get();
+ }
+ // Paper end - isConsumed flag for the connection
+
public PacketHolder(Packet<?> packet, @Nullable PacketSendListener callbacks) {
this.packet = packet;
this.listener = callbacks;
diff --git a/src/main/java/net/minecraft/network/protocol/Packet.java b/src/main/java/net/minecraft/network/protocol/Packet.java diff --git a/src/main/java/net/minecraft/network/protocol/Packet.java b/src/main/java/net/minecraft/network/protocol/Packet.java
index 0000000000000000000000000000000000000000..0000000000000000000000000000000000000000 100644 index 0000000000000000000000000000000000000000..0000000000000000000000000000000000000000 100644
--- a/src/main/java/net/minecraft/network/protocol/Packet.java --- a/src/main/java/net/minecraft/network/protocol/Packet.java