[#485] Fix concurrency crashes in chunk obfuscation and proximity checks

.github/workflows/buildtools.sh

@@ -19,27 +19,27 @@ build () {
 checkVersion () {
   echo Checking version $1
 
-  if [ ! -d ~/.m2/repository/org/spigotmc/spigot/$1-R0.1-SNAPSHOT ]; then
-    build $1 $2
+  if [ ! -d ~/.m2/repository/org/spigotmc/spigot/$1-$2-SNAPSHOT ]; then
+    build $1 $3
   fi
 }
 
-checkVersion "1.16.5" "8"
-checkVersion "1.17.1" "17"
-checkVersion "1.18.1" "17"
-checkVersion "1.18.2" "17"
-checkVersion "1.19.2" "17"
-checkVersion "1.19.3" "17"
-checkVersion "1.19.4" "17"
-checkVersion "1.20.1" "17"
-checkVersion "1.20.2" "17"
-checkVersion "1.20.4" "17"
-checkVersion "1.20.6" "21"
-checkVersion "1.21.1" "21"
-checkVersion "1.21.3" "21"
-checkVersion "1.21.4" "21"
-checkVersion "1.21.5" "21"
-checkVersion "1.21.8" "21"
-checkVersion "1.21.10" "21"
-checkVersion "1.21.11" "21"
-checkVersion "26.1" "25"
+checkVersion "1.16.5" "R0.1" "8"
+checkVersion "1.17.1" "R0.1" "17"
+checkVersion "1.18.1" "R0.1" "17"
+checkVersion "1.18.2" "R0.1" "17"
+checkVersion "1.19.2" "R0.1" "17"
+checkVersion "1.19.3" "R0.1" "17"
+checkVersion "1.19.4" "R0.1" "17"
+checkVersion "1.20.1" "R0.1" "17"
+checkVersion "1.20.2" "R0.1" "17"
+checkVersion "1.20.4" "R0.1" "17"
+checkVersion "1.20.6" "R0.1" "21"
+checkVersion "1.21.1" "R0.1" "21"
+checkVersion "1.21.3" "R0.1" "21"
+checkVersion "1.21.4" "R0.1" "21"
+checkVersion "1.21.5" "R0.1" "21"
+checkVersion "1.21.8" "R0.1" "21"
+checkVersion "1.21.10" "R0.1" "21"
+checkVersion "1.21.11" "R0.2" "21"
+checkVersion "26.1.2" "R0.1" "25"

orebfuscator-core/src/main/java/dev/imprex/orebfuscator/OrebfuscatorDumpFile.java

@@ -59,6 +59,7 @@ private void initialize() {
     set("versions.orebfuscator", orebfuscator.orebfuscatorVersion().toString());
 
     orebfuscator.systemMonitor().dump(createSection("system"));
+    orebfuscator.executor().dump(createSection("executor"));
 
     var statistics = createSection("statistics");
     orebfuscator.statisticsRegistry().entries().stream()

orebfuscator-core/src/main/java/dev/imprex/orebfuscator/obfuscation/DeobfuscationWorker.java

@@ -79,7 +79,7 @@ public void deobfuscate(WorldAccessor world, @Nullable List<BlockPos> blocks) {
       return;
     }
 
-    var timer = statistics.debofuscation.start();
+    var timer = statistics.deobfuscation.start();
     try {
       this.deobfuscateNow(world, blocks);
     } finally {

orebfuscator-core/src/main/java/dev/imprex/orebfuscator/player/OrebfuscatorPlayer.java

@@ -4,6 +4,7 @@
 import java.util.Map;
 import java.util.Objects;
 import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.atomic.AtomicBoolean;
 import java.util.concurrent.atomic.AtomicReference;
 import org.jspecify.annotations.NullMarked;
 import org.jspecify.annotations.Nullable;
@@ -22,6 +23,8 @@ public class OrebfuscatorPlayer {
 
   private final AtomicReference<@Nullable WorldAccessor> world = new AtomicReference<>();
   private final Map<Long, OrebfuscatorPlayerChunk> chunks = new ConcurrentHashMap<>();
+
+  private final AtomicBoolean pendingUpdate = new AtomicBoolean(false);
 
   private volatile long latestUpdateTimestamp = System.currentTimeMillis();
   private volatile EntityPose location = EntityPose.ZERO;
@@ -45,6 +48,13 @@ public boolean needsProximityUpdate(boolean rotation) {
     }
 
     long timestamp = System.currentTimeMillis();
+    if (this.pendingUpdate.compareAndSet(true, false)) {
+      this.location = player.pose();
+      this.latestUpdateTimestamp = timestamp;
+
+      return true;
+    }
+
     if (this.config.hasProximityPlayerCheckInterval()
         && timestamp - this.latestUpdateTimestamp > this.config.proximityPlayerCheckInterval()) {
 
@@ -106,6 +116,13 @@ public void addChunk(WorldAccessor world, int chunkX, int chunkZ, List<Proximity
     if (Objects.equals(this.world.getAcquire(), world)) {
       long key = ChunkAccessor.chunkCoordsToLong(chunkX, chunkZ);
       this.chunks.put(key, new OrebfuscatorPlayerChunk(chunkX, chunkZ, blocks));
+
+      int dChunkX = chunkX - (this.location.blockX() >> 4);
+      int dChunkZ = chunkZ - (this.location.blockZ() >> 4);
+
+      if (dChunkX >= -1 && dChunkX <= 1 && dChunkZ >= -1 && dChunkZ <= 1) {
+        this.pendingUpdate.set(true);
+      }
     }
   }
 

orebfuscator-core/src/main/java/dev/imprex/orebfuscator/proximity/ProximitySystem.java

@@ -44,32 +44,51 @@ public void start() {
   @Override
   public void run() {
     long processStart = System.nanoTime();
-    process().whenComplete((v, throwable) -> {
-      if (throwable != null) {
-        OfcLogger.error("An error occurred while running proximity worker", throwable);
-      }
-
-      if (this.executor.isShutdown()) {
-        return;
-      }
-
-      long processTime = System.nanoTime() - processStart;
-      this.statistics.proximityProcess.add(processTime);
-
-      // check if we have enough time to sleep
-      long waitTime = Math.max(0, this.checkInterval - processTime);
-      long waitMillis = TimeUnit.NANOSECONDS.toMillis(waitTime);
-
-      if (waitMillis > 0) {
-        // measure wait time
-        this.statistics.proximityWait.add(TimeUnit.MILLISECONDS.toNanos(waitMillis));
-        this.executor.schedule(this, waitMillis, TimeUnit.MILLISECONDS);
-      } else {
-        this.executor.execute(this);
+    invokeProcess().whenComplete((v, throwable) -> {
+      try {
+        if (throwable != null) {
+          OfcLogger.error("An error occurred while running proximity worker", throwable);
+        }
+
+        if (this.executor.isShutdown()) {
+          return;
+        }
+
+        long processTime = System.nanoTime() - processStart;
+        this.statistics.proximityProcess.add(processTime);
+
+        // check if we have enough time to sleep
+        long waitTime = Math.max(0, this.checkInterval - processTime);
+        long waitMillis = TimeUnit.NANOSECONDS.toMillis(waitTime);
+
+        if (waitMillis > 0) {
+          // measure wait time
+          this.statistics.proximityWait.add(TimeUnit.MILLISECONDS.toNanos(waitMillis));
+          this.executor.schedule(this, waitMillis, TimeUnit.MILLISECONDS);
+        } else {
+          // Schedule instead of executing directly to break reentrant execution chains of
+          // OrebfuscatorExecutor and avoid potential StackOverflowError.
+          this.statistics.proximityWait.add(0);
+          this.executor.schedule(this, 0L, TimeUnit.NANOSECONDS);
+        }
+      } catch (Exception e) {
+        OfcLogger.error("An error occurred while scheduling proximity worker", e);
+
+        // backoff for 1sec on reschedule failure
+        this.statistics.proximityWait.add(TimeUnit.SECONDS.toNanos(1));
+        this.executor.schedule(this, 1L, TimeUnit.SECONDS);
       }
     });
   }
 
+  private CompletableFuture<Void> invokeProcess() {
+    try {
+      return process();
+    } catch (Exception e) {
+      return CompletableFuture.failedFuture(e);
+    }
+  }
+
   private CompletableFuture<Void> process() {
     var players = this.orebfuscator.players();
     if (players.isEmpty()) {

orebfuscator-core/src/main/java/dev/imprex/orebfuscator/statistics/ObfuscationStatistics.java

@@ -1,15 +1,13 @@
 package dev.imprex.orebfuscator.statistics;
 
-import java.util.Map;
 import java.util.StringJoiner;
 import java.util.function.BiConsumer;
-import java.util.function.Consumer;
 import dev.imprex.orebfuscator.util.RollingAverage;
 import dev.imprex.orebfuscator.util.RollingTimer;
 
 public class ObfuscationStatistics implements StatisticsSource {
 
-  public final RollingTimer debofuscation = new RollingTimer(4096);
+  public final RollingTimer deobfuscation = new RollingTimer(4096);
 
   public final RollingTimer executorWaitTime = new RollingTimer(4096);
   public final RollingTimer executorUtilization = new RollingTimer(4096);
@@ -24,10 +22,10 @@ public class ObfuscationStatistics implements StatisticsSource {
 
   @Override
   public void add(StringJoiner joiner) {
-    long debofuscation = (long) this.debofuscation.average();
+    long deobfuscation = (long) this.deobfuscation.average();
 
-    joiner.add(String.format(" - debofuscation: %s",
-        time(debofuscation)));
+    joiner.add(String.format(" - deobfuscation: %s",
+        time(deobfuscation)));
 
     long executorWaitTime = (long) this.executorWaitTime.average();
     double executorUtilization = this.executorUtilization.average();
@@ -66,13 +64,13 @@ public void add(StringJoiner joiner) {
 
   @Override
   public void debug(BiConsumer<String, String> consumer) {
-    consumer.accept("debofuscation", this.debofuscation.debugLong(this::time));
+    consumer.accept("deobfuscation", this.deobfuscation.debugLong(this::time));
 
     consumer.accept("executorWaitTime", this.executorWaitTime.debugLong(this::time));
     consumer.accept("executorUtilization", this.executorUtilization.debugDouble(this::percent));
 
     consumer.accept("proximityWait", this.proximityWait.debugLong(this::time));
-    consumer.accept("proximityProcess", this.proximityProcess.debugDouble(this::percent));
+    consumer.accept("proximityProcess", this.proximityProcess.debugLong(this::time));
 
     consumer.accept("missingNeighboringChunks", this.missingNeighboringChunks.debugDouble(this::faction));
 

orebfuscator-core/src/main/java/dev/imprex/orebfuscator/util/concurrent/OrebfuscatorExecutor.java

@@ -1,25 +1,33 @@
 package dev.imprex.orebfuscator.util.concurrent;
 
+import dev.imprex.orebfuscator.config.yaml.ConfigurationSection;
+import java.util.Arrays;
+import java.util.List;
 import java.util.Objects;
+import java.util.concurrent.BlockingQueue;
 import java.util.concurrent.Executor;
 import java.util.concurrent.ExecutorService;
 import java.util.concurrent.Executors;
+import java.util.concurrent.LinkedBlockingQueue;
 import java.util.concurrent.ScheduledExecutorService;
 import java.util.concurrent.ScheduledFuture;
+import java.util.concurrent.ScheduledThreadPoolExecutor;
+import java.util.concurrent.ThreadPoolExecutor;
 import java.util.concurrent.TimeUnit;
 import java.util.concurrent.atomic.LongAdder;
+import java.util.stream.Collectors;
 import org.jspecify.annotations.NullMarked;
 import dev.imprex.orebfuscator.interop.OrebfuscatorCore;
 import dev.imprex.orebfuscator.statistics.OrebfuscatorStatistics;
 
 @NullMarked
 public class OrebfuscatorExecutor implements Executor {
 
-  private final ScheduledExecutorService scheduledExecutorService = Executors
-      .newSingleThreadScheduledExecutor(r -> new Thread(OrebfuscatorCore.THREAD_GROUP, r, "orebfuscator-scheduler"));
+  private final ScheduledThreadPoolExecutor scheduledExecutorService = new ScheduledThreadPoolExecutor(
+      1, r -> new Thread(OrebfuscatorCore.THREAD_GROUP, r, "orebfuscator-scheduler"));
 
   private final int poolSize;
-  private final ExecutorService executorService;
+  private final ThreadPoolExecutor executorService;
 
   private final LongAdder run = new LongAdder();
   private long updateTime = System.nanoTime();
@@ -30,7 +38,11 @@ public OrebfuscatorExecutor(OrebfuscatorCore orebfuscator) {
     this.statistics = orebfuscator.statistics();
 
     this.poolSize = orebfuscator.config().advanced().threads();
-    this.executorService = Executors.newFixedThreadPool(this.poolSize, OrebfuscatorThread::new);
+    this.executorService = new ThreadPoolExecutor(
+        this.poolSize, this.poolSize,
+        0L, TimeUnit.MILLISECONDS,
+        new LinkedBlockingQueue<>(),
+        OrebfuscatorThread::new);
 
     this.scheduledExecutorService.scheduleAtFixedRate(this::updateStatistics, 1L, 1L, TimeUnit.SECONDS);
   }
@@ -65,6 +77,79 @@ public void shutdown() {
     this.executorService.shutdownNow();
   }
 
+  public void dump(ConfigurationSection section) {
+    dumpExecutor(section.createSection("scheduler"), this.scheduledExecutorService);
+    dumpExecutor(section.createSection("core"), this.executorService);
+
+    ConfigurationSection threads = section.createSection("threads");
+
+    for (var entry : Thread.getAllStackTraces().entrySet()) {
+      Thread thread = entry.getKey();
+
+      if (!(thread instanceof OrebfuscatorThread)) {
+        continue;
+      }
+
+      StackTraceElement[] stackTrace = entry.getValue();
+
+      ConfigurationSection threadSection =
+          threads.createSection("thread-" + thread.getId());
+
+      threadSection.set("name", thread.getName());
+      threadSection.set("id", thread.getId());
+      threadSection.set("state", thread.getState().name());
+      threadSection.set("alive", thread.isAlive());
+      threadSection.set("daemon", thread.isDaemon());
+      threadSection.set("priority", thread.getPriority());
+      threadSection.set("stack", formatStackTrace(stackTrace));
+    }
+  }
+
+  private static void dumpExecutor(ConfigurationSection section, ThreadPoolExecutor executor) {
+    section.set("shutdown", executor.isShutdown());
+    section.set("terminating", executor.isTerminating());
+    section.set("terminated", executor.isTerminated());
+
+    section.set("pool.size", executor.getPoolSize());
+    section.set("pool.coreSize", executor.getCorePoolSize());
+    section.set("pool.maxSize", executor.getMaximumPoolSize());
+    section.set("pool.largestSize", executor.getLargestPoolSize());
+    section.set("pool.active", executor.getActiveCount());
+
+    long taskCount = executor.getTaskCount();
+    long completedTaskCount = executor.getCompletedTaskCount();
+
+    section.set("tasks.total", taskCount);
+    section.set("tasks.completed", completedTaskCount);
+    section.set("tasks.pending", Math.max(0, taskCount - completedTaskCount));
+
+    BlockingQueue<Runnable> queue = executor.getQueue();
+    section.set("queue.type", queue.getClass().getName());
+    section.set("queue.size", queue.size());
+    section.set("queue.remainingCapacity", queue.remainingCapacity());
+
+    section.set("keepAliveMillis", executor.getKeepAliveTime(TimeUnit.MILLISECONDS));
+    section.set("allowsCoreThreadTimeOut", executor.allowsCoreThreadTimeOut());
+
+    section.set("threadFactory", executor.getThreadFactory().getClass().getName());
+    section.set("rejectedExecutionHandler", executor.getRejectedExecutionHandler().getClass().getName());
+
+    if (executor instanceof ScheduledThreadPoolExecutor scheduled) {
+      section.set("scheduled.continueExistingPeriodicTasksAfterShutdown",
+          scheduled.getContinueExistingPeriodicTasksAfterShutdownPolicy());
+      section.set("scheduled.executeExistingDelayedTasksAfterShutdown",
+          scheduled.getExecuteExistingDelayedTasksAfterShutdownPolicy());
+      section.set("scheduled.removeOnCancel",
+          scheduled.getRemoveOnCancelPolicy());
+    }
+  }
+
+  private static List<String> formatStackTrace(StackTraceElement[] stackTrace) {
+    return Arrays.stream(stackTrace)
+        .map(element -> "at " + element)
+        .collect(Collectors.toList());
+  }
+
   private void updateStatistics() {
     long time = System.nanoTime();
     try {

orebfuscator-plugin/src/main/java/net/imprex/orebfuscator/iterop/BukkitChunkPacketAccessor.java

@@ -14,6 +14,7 @@
 import net.imprex.orebfuscator.util.MinecraftVersion;
 import net.imprex.orebfuscator.util.WrappedClientboundLevelChunkPacketData;
 import org.jspecify.annotations.NullMarked;
+import org.jspecify.annotations.Nullable;
 
 @NullMarked
 public class BukkitChunkPacketAccessor implements ChunkPacketAccessor {
@@ -29,7 +30,7 @@ public class BukkitChunkPacketAccessor implements ChunkPacketAccessor {
   private final byte[] data;
 
   private final PacketContainer packet;
-  private final WrappedClientboundLevelChunkPacketData packetData;
+  private final @Nullable WrappedClientboundLevelChunkPacketData packetData;
 
   public BukkitChunkPacketAccessor(PacketContainer packet, BukkitWorldAccessor worldAccessor) {
     this.packet = packet;

orebfuscator-plugin/src/main/java/net/imprex/orebfuscator/iterop/BukkitPlayerAccessor.java

@@ -12,7 +12,6 @@
 import java.util.Objects;
 import java.util.WeakHashMap;
 import java.util.concurrent.CompletableFuture;
-import java.util.concurrent.atomic.AtomicBoolean;
 import java.util.concurrent.atomic.AtomicReference;
 import net.imprex.orebfuscator.Orebfuscator;
 import net.imprex.orebfuscator.OrebfuscatorCompatibility;