From 7ffde5cb2437e2f1fd9b3fad73b6c780e155bac3 Mon Sep 17 00:00:00 2001 From: Aikar Date: Fri, 29 May 2020 20:36:42 -0400 Subject: [PATCH] Synchronize DataPaletteBlock instead of ReentrantLock Mojang has flaws in their logic about chunks being concurrently wrote to. So we constantly see crashes around multiple threads writing. Additionally, java has optimized synchronization so well that its in many times faster than trying to manage read wrote locks for low contention situations. And this is extremely a low contention situation. Fixes #3293 Fixes #2493 --- Spigot-Server-Patches/Anti-Xray.patch | 8 +- ...ead-in-DataPaletteBlock-lock-failure.patch | 48 --------- ...PaletteBlock-instead-of-ReentrantLoc.patch | 101 ++++++++++++++++++ 3 files changed, 105 insertions(+), 52 deletions(-) delete mode 100644 Spigot-Server-Patches/Log-other-thread-in-DataPaletteBlock-lock-failure.patch create mode 100644 Spigot-Server-Patches/Synchronize-DataPaletteBlock-instead-of-ReentrantLoc.patch diff --git a/Spigot-Server-Patches/Anti-Xray.patch b/Spigot-Server-Patches/Anti-Xray.patch index a0ae30003..0dbc07153 100644 --- a/Spigot-Server-Patches/Anti-Xray.patch +++ b/Spigot-Server-Patches/Anti-Xray.patch @@ -1119,7 +1119,7 @@ index 0000000000000000000000000000000000000000..00000000000000000000000000000000 private DataPalette h; private DataPalette getDataPalette() { return this.h; } // Paper - OBFHELPER private int i; private int getBitsPerObject() { return this.i; } // Paper - OBFHELPER @@ -0,0 +0,0 @@ public class DataPaletteBlock implements DataPaletteExpandable { - this.j.unlock(); + //this.j.unlock(); // Paper - disable this } - public DataPaletteBlock(DataPalette datapalette, RegistryBlockID registryblockid, Function function, Function function1, T t0) { @@ -1181,12 +1181,12 @@ index 0000000000000000000000000000000000000000..00000000000000000000000000000000 } - public void writeDataPaletteBlock(PacketDataSerializer packetDataSerializer) { this.b(packetDataSerializer); } // Paper - OBFHELPER -- public void b(PacketDataSerializer packetdataserializer) { +- public synchronized void b(PacketDataSerializer packetdataserializer) { // Paper - synchronize + // Paper start - Anti-Xray - Add chunk packet info + @Deprecated public void writeDataPaletteBlock(PacketDataSerializer packetDataSerializer) { this.b(packetDataSerializer); } // OBFHELPER // Notice for updates: Please make sure this method isn't used anywhere + @Deprecated public void b(PacketDataSerializer packetdataserializer) { this.writeDataPaletteBlock(packetdataserializer, null, 0); } // Notice for updates: Please make sure this method isn't used anywhere + public void writeDataPaletteBlock(PacketDataSerializer packetDataSerializer, ChunkPacketInfo chunkPacketInfo, int chunkSectionIndex) { this.b(packetDataSerializer, chunkPacketInfo, chunkSectionIndex); } // OBFHELPER -+ public void b(PacketDataSerializer packetdataserializer, ChunkPacketInfo chunkPacketInfo, int chunkSectionIndex) { ++ public synchronized void b(PacketDataSerializer packetdataserializer, ChunkPacketInfo chunkPacketInfo, int chunkSectionIndex) { // Paper - synchronize + // Paper end this.a(); packetdataserializer.writeByte(this.i); @@ -1203,7 +1203,7 @@ index 0000000000000000000000000000000000000000..00000000000000000000000000000000 this.b(); } - public void a(NBTTagList nbttaglist, long[] along) { + public synchronized void a(NBTTagList nbttaglist, long[] along) { // Paper - synchronize this.a(); - int i = Math.max(4, MathHelper.d(nbttaglist.size())); + // Paper - Anti-Xray - TODO: Should this.predefinedObjects.length just be added here (faster) or should the contents be compared to calculate the size (less RAM)? diff --git a/Spigot-Server-Patches/Log-other-thread-in-DataPaletteBlock-lock-failure.patch b/Spigot-Server-Patches/Log-other-thread-in-DataPaletteBlock-lock-failure.patch deleted file mode 100644 index 6f6068a58..000000000 --- a/Spigot-Server-Patches/Log-other-thread-in-DataPaletteBlock-lock-failure.patch +++ /dev/null @@ -1,48 +0,0 @@ -From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001 -From: Spottedleaf -Date: Fri, 21 Jun 2019 14:42:48 -0700 -Subject: [PATCH] Log other thread in DataPaletteBlock lock failure - - -diff --git a/src/main/java/com/destroystokyo/paper/util/ReentrantLockWithGetOwner.java b/src/main/java/com/destroystokyo/paper/util/ReentrantLockWithGetOwner.java -new file mode 100644 -index 0000000000000000000000000000000000000000..0000000000000000000000000000000000000000 ---- /dev/null -+++ b/src/main/java/com/destroystokyo/paper/util/ReentrantLockWithGetOwner.java -@@ -0,0 +0,0 @@ -+package com.destroystokyo.paper.util; -+ -+import java.util.concurrent.locks.ReentrantLock; -+ -+public class ReentrantLockWithGetOwner extends ReentrantLock { -+ -+ @Override -+ public Thread getOwner() { -+ return super.getOwner(); -+ } -+} -diff --git a/src/main/java/net/minecraft/server/DataPaletteBlock.java b/src/main/java/net/minecraft/server/DataPaletteBlock.java -index 0000000000000000000000000000000000000000..0000000000000000000000000000000000000000 100644 ---- a/src/main/java/net/minecraft/server/DataPaletteBlock.java -+++ b/src/main/java/net/minecraft/server/DataPaletteBlock.java -@@ -0,0 +0,0 @@ public class DataPaletteBlock implements DataPaletteExpandable { - protected DataBits a; protected DataBits getDataBits() { return this.a; } // Paper - OBFHELPER - private DataPalette h; private DataPalette getDataPalette() { return this.h; } // Paper - OBFHELPER - private int i; private int getBitsPerObject() { return this.i; } // Paper - OBFHELPER -- private final ReentrantLock j = new ReentrantLock(); -+ private final com.destroystokyo.paper.util.ReentrantLockWithGetOwner j = new com.destroystokyo.paper.util.ReentrantLockWithGetOwner(); private com.destroystokyo.paper.util.ReentrantLockWithGetOwner getLock() { return this.j; } // Paper - change type to ReentrantLockWithGetOwner // Paper - OBFHELPER - - public void a() { -- if (this.j.isLocked() && !this.j.isHeldByCurrentThread()) { -+ // Paper start - log other thread -+ Thread owningThread; -+ if (this.j.isLocked() && (owningThread = this.getLock().getOwner()) != null && owningThread != Thread.currentThread()) { -+ // Paper end - String s = (String) Thread.getAllStackTraces().keySet().stream().filter(Objects::nonNull).map((thread) -> { - return thread.getName() + ": \n\tat " + (String) Arrays.stream(thread.getStackTrace()).map(Object::toString).collect(Collectors.joining("\n\tat ")); - }).collect(Collectors.joining("\n")); -- CrashReport crashreport = new CrashReport("Writing into PalettedContainer from multiple threads", new IllegalStateException()); -+ CrashReport crashreport = new CrashReport("Writing into PalettedContainer from multiple threads (other thread: name: " + owningThread.getName() + ", class: " + owningThread.getClass().toString() + ")", new IllegalStateException()); // Paper - log other thread - CrashReportSystemDetails crashreportsystemdetails = crashreport.a("Thread dumps"); - - crashreportsystemdetails.a("Thread dumps", (Object) s); diff --git a/Spigot-Server-Patches/Synchronize-DataPaletteBlock-instead-of-ReentrantLoc.patch b/Spigot-Server-Patches/Synchronize-DataPaletteBlock-instead-of-ReentrantLoc.patch new file mode 100644 index 000000000..548f86bdf --- /dev/null +++ b/Spigot-Server-Patches/Synchronize-DataPaletteBlock-instead-of-ReentrantLoc.patch @@ -0,0 +1,101 @@ +From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001 +From: Aikar +Date: Fri, 29 May 2020 20:29:02 -0400 +Subject: [PATCH] Synchronize DataPaletteBlock instead of ReentrantLock + +Mojang has flaws in their logic about chunks being concurrently +wrote to. So we constantly see crashes around multiple threads writing. + +Additionally, java has optimized synchronization so well that its +in many times faster than trying to manage read wrote locks for low +contention situations. + +And this is extremely a low contention situation. + +diff --git a/src/main/java/net/minecraft/server/DataPaletteBlock.java b/src/main/java/net/minecraft/server/DataPaletteBlock.java +index 0000000000000000000000000000000000000000..0000000000000000000000000000000000000000 100644 +--- a/src/main/java/net/minecraft/server/DataPaletteBlock.java ++++ b/src/main/java/net/minecraft/server/DataPaletteBlock.java +@@ -0,0 +0,0 @@ public class DataPaletteBlock implements DataPaletteExpandable { + private int i; private int getBitsPerObject() { return this.i; } // Paper - OBFHELPER + private final ReentrantLock j = new ReentrantLock(); + +- public void a() { ++ public void a() { /* // Paper start - disable this - use proper synchronization + if (this.j.isLocked() && !this.j.isHeldByCurrentThread()) { + String s = (String) Thread.getAllStackTraces().keySet().stream().filter(Objects::nonNull).map((thread) -> { + return thread.getName() + ": \n\tat " + (String) Arrays.stream(thread.getStackTrace()).map(Object::toString).collect(Collectors.joining("\n\tat ")); +@@ -0,0 +0,0 @@ public class DataPaletteBlock implements DataPaletteExpandable { + throw new ReportedException(crashreport); + } else { + this.j.lock(); +- } ++ } */ // Paper end + } + + public void b() { +- this.j.unlock(); ++ //this.j.unlock(); // Paper - disable this + } + + public DataPaletteBlock(DataPalette datapalette, RegistryBlockID registryblockid, Function function, Function function1, T t0) { +@@ -0,0 +0,0 @@ public class DataPaletteBlock implements DataPaletteExpandable { + } + + @Override +- public int onResize(int i, T t0) { ++ public synchronized int onResize(int i, T t0) { // Paper - synchronize + this.a(); + DataBits databits = this.a; + DataPalette datapalette = this.h; +@@ -0,0 +0,0 @@ public class DataPaletteBlock implements DataPaletteExpandable { + } + + public T setBlock(int i, int j, int k, T t0) { +- this.a(); +- T t1 = this.a(b(i, j, k), t0); ++ //this.a(); // Paper - remove to reduce ops - synchronize handled below ++ return this.a(b(i, j, k), t0); // Paper + +- this.b(); +- return t1; ++ //this.b(); // Paper ++ //return t1; // PAper + } + + public T b(int i, int j, int k, T t0) { + return this.a(b(i, j, k), t0); + } + +- protected T a(int i, T t0) { ++ protected synchronized T a(int i, T t0) { // Paper - synchronize - writes + int j = this.h.a(t0); + int k = this.a.a(i, j); + T t1 = this.h.a(k); +@@ -0,0 +0,0 @@ public class DataPaletteBlock implements DataPaletteExpandable { + } + + public void writeDataPaletteBlock(PacketDataSerializer packetDataSerializer) { this.b(packetDataSerializer); } // Paper - OBFHELPER +- public void b(PacketDataSerializer packetdataserializer) { ++ public synchronized void b(PacketDataSerializer packetdataserializer) { // Paper - synchronize + this.a(); + packetdataserializer.writeByte(this.i); + this.h.b(packetdataserializer); +@@ -0,0 +0,0 @@ public class DataPaletteBlock implements DataPaletteExpandable { + this.b(); + } + +- public void a(NBTTagList nbttaglist, long[] along) { ++ public synchronized void a(NBTTagList nbttaglist, long[] along) { // Paper - synchronize + this.a(); + int i = Math.max(4, MathHelper.d(nbttaglist.size())); + +@@ -0,0 +0,0 @@ public class DataPaletteBlock implements DataPaletteExpandable { + this.b(); + } + +- public void a(NBTTagCompound nbttagcompound, String s, String s1) { ++ public synchronized void a(NBTTagCompound nbttagcompound, String s, String s1) { // Paper - synchronize + this.a(); + DataPaletteHash datapalettehash = new DataPaletteHash<>(this.d, this.i, this.c, this.e, this.f); + T t0 = this.g;