Merge pull request #919 from electronicboy/improve-keepalive-handling

allow keepalive to wait longer for a client response (#895)

Spigot-Server-Patches/IllegalPacketEvent.patch

@@ -6,7 +6,7 @@ Subject: [PATCH] IllegalPacketEvent
 Fired for invalid data from players that represents hacking attempts
 
 diff --git a/src/main/java/net/minecraft/server/PlayerConnection.java b/src/main/java/net/minecraft/server/PlayerConnection.java
-index 02b5e3cad..52e8458ab 100644
+index 9f05c7da2..a04df7d88 100644
 --- a/src/main/java/net/minecraft/server/PlayerConnection.java
 +++ b/src/main/java/net/minecraft/server/PlayerConnection.java
 @@ -0,0 +0,0 @@ import org.bukkit.inventory.CraftingInventory;

Spigot-Server-Patches/Increase-time-allowed-for-a-keepalive-reply.patch

@@ -0,0 +1,52 @@
+From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
+From: Shane Freeder <theboyetronic@gmail.com>
+Date: Sun, 15 Oct 2017 00:29:07 +0100
+Subject: [PATCH] Increase time allowed for a keepalive reply
+
+This patch intends to bump up the time that a client has to reply to the
+server back to 30 seconds as per pre 1.12.2, which allowed clients
+more than enough time to reply potentially allowing them to be less
+tempermental due to lag spikes on the network thread, e.g. that caused
+by plugins that are interacting with netty.
+
+diff --git a/src/main/java/net/minecraft/server/PlayerConnection.java b/src/main/java/net/minecraft/server/PlayerConnection.java
+index 26fbb30f9..5bb6d9fac 100644
+--- a/src/main/java/net/minecraft/server/PlayerConnection.java
++++ b/src/main/java/net/minecraft/server/PlayerConnection.java
+@@ -0,0 +0,0 @@ public class PlayerConnection implements PacketListenerPlayIn, ITickable {
+         }
+ 
+         this.minecraftServer.methodProfiler.a("keepAlive");
+-        long i = this.d();
+-
+-        if (i - this.f >= 25000L) { // CraftBukkit
+-            if (this.g) {
+-                this.disconnect(new ChatMessage("disconnect.timeout", new Object[0]));
+-            } else {
+-                this.g = true;
+-                this.f = i;
+-                this.h = i;
+-                this.sendPacket(new PacketPlayOutKeepAlive(this.h));
++        // Paper Start - give clients a longer time to respond to pings as per pre 1.12.2 timings
++        // This should effectively place the keepalive handling back to "as it was" before 1.12.2
++        long currentTime = this.getCurrentMillis();
++        long elapsedTime = currentTime - this.getLastPing();
++        if (this.isPendingPing()) {
++            // We're pending a ping from the client
++            if (elapsedTime >= 30000L) { // 30 seconds for a ping reply
++                PlayerConnection.LOGGER.warn("{} was kicked due to keepalive timeout!", this.player.getName()); // more info
++                this.disconnect(new ChatMessage("disconnect.timeout"));
++            }
++        } else {
++            if (elapsedTime >= 15000L) { // 15 seconds
++                this.setPendingPing(true);
++                this.setLastPing(currentTime);
++                this.setKeepAliveID(currentTime);
++                this.sendPacket(new PacketPlayOutKeepAlive(this.getKeepAliveID()));
+             }
+         }
++        // Paper end
+ 
+         this.minecraftServer.methodProfiler.b();
+         // CraftBukkit start
+--

Spigot-Server-Patches/MC-Utils.patch

@@ -273,4 +273,29 @@ index e0cb6aa6e..bc6383669 100644
      private byte type = 0;
  
      public NBTTagList() {}
+diff --git a/src/main/java/net/minecraft/server/PlayerConnection.java b/src/main/java/net/minecraft/server/PlayerConnection.java
+index aa9d024fa..516673c0e 100644
+--- a/src/main/java/net/minecraft/server/PlayerConnection.java
++++ b/src/main/java/net/minecraft/server/PlayerConnection.java
+@@ -0,0 +0,0 @@ public class PlayerConnection implements PacketListenerPlayIn, ITickable {
+     private final MinecraftServer minecraftServer;
+     public EntityPlayer player;
+     private int e;
+-    private long f;
+-    private boolean g;
+-    private long h;
++    private long f; private void setLastPing(long lastPing) { this.f = lastPing;}; private long getLastPing() { return this.f;}; // Paper - OBFHELPER
++    private boolean g; private void setPendingPing(boolean isPending) { this.g = isPending;}; private boolean isPendingPing() { return this.g;}; // Paper - OBFHELPER
++    private long h; private void setKeepAliveID(long keepAliveID) { this.h = keepAliveID;}; private long getKeepAliveID() {return this.h; };  // Paper - OBFHELPER
+     // CraftBukkit start - multithreaded fields
+     private volatile int chatThrottle;
+     private static final AtomicIntegerFieldUpdater chatSpamField = AtomicIntegerFieldUpdater.newUpdater(PlayerConnection.class, "chatThrottle");
+@@ -0,0 +0,0 @@ public class PlayerConnection implements PacketListenerPlayIn, ITickable {
+ 
+     }
+ 
++    private long getCurrentMillis() { return d(); } // Paper - OBFHELPER
+     private long d() {
+         return System.nanoTime() / 1000000L;
+     }
 --

Spigot-Server-Patches/Properly-fix-item-duplication-bug.patch

@@ -19,7 +19,7 @@ index 53147c6e2..5fbb99b7e 100644
  
      @Override
 diff --git a/src/main/java/net/minecraft/server/PlayerConnection.java b/src/main/java/net/minecraft/server/PlayerConnection.java
-index 52e8458ab..bcc6c9707 100644
+index a04df7d88..22ead6533 100644
 --- a/src/main/java/net/minecraft/server/PlayerConnection.java
 +++ b/src/main/java/net/minecraft/server/PlayerConnection.java
 @@ -0,0 +0,0 @@ public class PlayerConnection implements PacketListenerPlayIn, ITickable {

Spigot-Server-Patches/handle-PacketPlayInKeepAlive-async.patch

@@ -1,7 +1,7 @@
 From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
 From: Shane Freeder <theboyetronic@gmail.com>
 Date: Thu, 5 Oct 2017 01:54:07 +0100
-Subject: [PATCH] handle PacketPlayInKeepAlive async and revert keepalive limit
+Subject: [PATCH] handle PacketPlayInKeepAlive async
 
 In 1.12.2, Mojang moved the processing of PacketPlayInKeepAlive off the main
 thread, while entirely correct for the server, this causes issues with
@@ -11,24 +11,13 @@ In order to counteract some bad behavior, we will post handling of the
 disconnection to the main thread, but leave the actual processing of the packet
 off the main thread.
 
-We also revert the bump on the servers built in keepalive back to 15 seconds,
-this solves a read timeout in scenarios where the client isn't sending data to
-the server, e.g. spectating an entity.
+also adding some additional logging in order to help work out what is causing
+random disconnections for clients.
 
 diff --git a/src/main/java/net/minecraft/server/PlayerConnection.java b/src/main/java/net/minecraft/server/PlayerConnection.java
-index 69ed84af9..b18bf4245 100644
+index a07904143..26fbb30f9 100644
 --- a/src/main/java/net/minecraft/server/PlayerConnection.java
 +++ b/src/main/java/net/minecraft/server/PlayerConnection.java
-@@ -0,0 +0,0 @@ public class PlayerConnection implements PacketListenerPlayIn, ITickable {
- 
-         this.minecraftServer.methodProfiler.a("keepAlive");
-         long i = this.d();
--
--        if (i - this.f >= 25000L) { // CraftBukkit
-+        if (i - this.f >= 15000L) { // CraftBukkit // Paper - revert to 15
-             if (this.g) {
-                 this.disconnect(new ChatMessage("disconnect.timeout", new Object[0]));
-             } else {
 @@ -0,0 +0,0 @@ public class PlayerConnection implements PacketListenerPlayIn, ITickable {
      }
  
@@ -43,6 +32,8 @@ index 69ed84af9..b18bf4245 100644
          } else if (!this.player.getName().equals(this.minecraftServer.Q())) {
 -            this.disconnect(new ChatMessage("disconnect.timeout", new Object[0]));
 +            // Paper start - This needs to be handled on the main thread for plugins
++            PlayerConnection.LOGGER.warn("{} sent an invalid keepalive! pending keepalive: {} got id: {} expected id: {}",
++                    this.player.getName(), this.isPendingPing(), packetplayinkeepalive.a(), this.getKeepAliveID());
 +            minecraftServer.postToMainThread(() -> {
 +                    this.disconnect(new ChatMessage("disconnect.timeout", new Object[0]));
 +            });