Folia/patches/server/0028-Mark-POI-Entity-load-tasks-as-completed-before-relea.patch

From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
From: Spottedleaf <Spottedleaf@users.noreply.github.com>
Date: Mon, 15 May 2023 11:34:28 -0700
Subject: [PATCH] Mark POI/Entity load tasks as completed before releasing
 scheduling lock

It must be marked as completed during that lock hold since the
waiters field is set to null. Thus, any other thread attempting
a cancellation will fail to remove from waiters. Also, any
other thread attempting to cancel may set the completed field
to true which would cause accept() to fail as well.

Completion was always designed to happen while holding the
scheduling lock to prevent these race conditions. The code
was originally set up to complete while not holding the
scheduling lock to avoid invoking callbacks while holding the
lock, however the access to the completion field was not
considered.

Resolve this by marking the callback as completed during the
lock, but invoking the accept() function after releasing
the lock. This will prevent any cancellation attempts to be
blocked, and allow the current thread to complete the callback
without any issues.

diff --git a/src/main/java/io/papermc/paper/chunk/system/scheduling/NewChunkHolder.java b/src/main/java/io/papermc/paper/chunk/system/scheduling/NewChunkHolder.java
index 1ff6b138ccf4a1cefa719cd0b2b3af02d18a26fb..dc60f4af918c2bd73873cadfb69e5572173b8e46 100644
--- a/src/main/java/io/papermc/paper/chunk/system/scheduling/NewChunkHolder.java
+++ b/src/main/java/io/papermc/paper/chunk/system/scheduling/NewChunkHolder.java
@@ -156,6 +156,12 @@ public final class NewChunkHolder {
                     LOGGER.error("Unhandled entity data load exception, data data will be lost: ", result.right());
                 }
 
+                // Folia start - mark these tasks as completed before releasing the scheduling lock
+                for (final GenericDataLoadTaskCallback callback : waiters) {
+                    callback.markCompleted();
+                }
+                // Folia end - mark these tasks as completed before releasing the scheduling lock
+
                 completeWaiters = waiters;
             } else {
                 // cancelled
@@ -187,7 +193,7 @@ public final class NewChunkHolder {
         // avoid holding the scheduling lock while completing
         if (completeWaiters != null) {
             for (final GenericDataLoadTaskCallback callback : completeWaiters) {
-                callback.accept(result);
+                callback.acceptCompleted(result); // Folia - mark these tasks as completed before releasing the scheduling lock
             }
         }
 
@@ -273,6 +279,12 @@ public final class NewChunkHolder {
                     LOGGER.error("Unhandled poi load exception, poi data will be lost: ", result.right());
                 }
 
+                // Folia start - mark these tasks as completed before releasing the scheduling lock
+                for (final GenericDataLoadTaskCallback callback : waiters) {
+                    callback.markCompleted();
+                }
+                // Folia end - mark these tasks as completed before releasing the scheduling lock
+
                 completeWaiters = waiters;
             } else {
                 // cancelled
@@ -304,7 +316,7 @@ public final class NewChunkHolder {
         // avoid holding the scheduling lock while completing
         if (completeWaiters != null) {
             for (final GenericDataLoadTaskCallback callback : completeWaiters) {
-                callback.accept(result);
+                callback.acceptCompleted(result); // Folia - mark these tasks as completed before releasing the scheduling lock
             }
         }
         schedulingLock = this.scheduler.schedulingLockArea.lock(this.chunkX, this.chunkZ); // Folia - use area based lock to reduce contention
@@ -357,7 +369,7 @@ public final class NewChunkHolder {
         }
     }
 
-    public static abstract class GenericDataLoadTaskCallback implements Cancellable, Consumer<GenericDataLoadTask.TaskResult<?, Throwable>> {
+    public static abstract class GenericDataLoadTaskCallback implements Cancellable { // Folia - mark callbacks as completed before unlocking scheduling lock
 
         protected final Consumer<GenericDataLoadTask.TaskResult<?, Throwable>> consumer;
         protected final NewChunkHolder chunkHolder;
@@ -393,13 +405,23 @@ public final class NewChunkHolder {
             return this.completed = true;
         }
 
-        @Override
-        public void accept(final GenericDataLoadTask.TaskResult<?, Throwable> result) {
+        // Folia start - mark callbacks as completed before unlocking scheduling lock
+        // must hold scheduling lock
+        void markCompleted() {
+            if (this.completed) {
+                throw new IllegalStateException("May not be completed here");
+            }
+            this.completed = true;
+        }
+        // Folia end - mark callbacks as completed before unlocking scheduling lock
+
+        // Folia - mark callbacks as completed before unlocking scheduling lock
+        void acceptCompleted(final GenericDataLoadTask.TaskResult<?, Throwable> result) {
             if (result != null) {
-                if (this.setCompleted()) {
+                if (this.completed) { // Folia - mark callbacks as completed before unlocking scheduling lock
                     this.consumer.accept(result);
                 } else {
-                    throw new IllegalStateException("Cannot be cancelled at this point");
+                    throw new IllegalStateException("Cannot be uncompleted at this point"); // Folia - mark callbacks as completed before unlocking scheduling lock
                 }
             } else {
                 throw new NullPointerException("Result cannot be null (cancelled)");