fix: always execute chunk load callback/event on the instance tick thread

src/main/java/net/minestom/server/instance/InstanceContainer.java

@@ -20,6 +20,7 @@ import net.minestom.server.network.packet.server.play.BlockChangePacket;
 import net.minestom.server.network.packet.server.play.BlockEntityDataPacket;
 import net.minestom.server.network.packet.server.play.EffectPacket;
 import net.minestom.server.network.packet.server.play.UnloadChunkPacket;
+import net.minestom.server.thread.TickSchedulerThread;
 import net.minestom.server.utils.NamespaceID;
 import net.minestom.server.utils.PacketUtils;
 import net.minestom.server.utils.async.AsyncUtils;
@@ -44,6 +45,7 @@ import java.util.concurrent.CopyOnWriteArrayList;
 import java.util.concurrent.ForkJoinPool;
 import java.util.concurrent.locks.Lock;
 import java.util.concurrent.locks.ReentrantLock;
+import java.util.function.Consumer;
 import java.util.function.Supplier;
 
 import static net.minestom.server.utils.chunk.ChunkUtils.*;
@@ -291,6 +293,15 @@ public class InstanceContainer extends Instance {
         final CompletableFuture<Chunk> prev = loadingChunks.putIfAbsent(index, completableFuture);
         if (prev != null) return prev;
         final IChunkLoader loader = chunkLoader;
+        final Consumer<Chunk> postLoadCallback = chunk -> {
+            cacheChunk(chunk);
+            chunk.onLoad();
+
+            EventDispatcher.call(new InstanceChunkLoadEvent(this, chunk));
+            final CompletableFuture<Chunk> future = this.loadingChunks.remove(index);
+            assert future == completableFuture : "Invalid future: " + future;
+            completableFuture.complete(chunk);
+        };
         final Runnable retriever = () -> loader.loadChunk(this, chunkX, chunkZ)
                 .thenCompose(chunk -> {
                     if (chunk != null) {
@@ -303,14 +314,11 @@ public class InstanceContainer extends Instance {
                 })
                 // cache the retrieved chunk
                 .thenAccept(chunk -> {
-                    // TODO run in the instance thread?
-                    cacheChunk(chunk);
-                    chunk.onLoad();
-
-                    EventDispatcher.call(new InstanceChunkLoadEvent(this, chunk));
-                    final CompletableFuture<Chunk> future = this.loadingChunks.remove(index);
-                    assert future == completableFuture : "Invalid future: " + future;
-                    completableFuture.complete(chunk);
+                    if (Thread.currentThread() instanceof TickSchedulerThread) {
+                        postLoadCallback.accept(chunk); // Already running in instance tick thread
+                    } else {
+                        scheduleNextTick(ignored -> postLoadCallback.accept(chunk));
+                    }
                 })
                 .exceptionally(throwable -> {
                     MinecraftServer.getExceptionManager().handleException(throwable);

src/test/java/net/minestom/server/instance/InstanceChunkLoadIntegrationTest.java

@@ -0,0 +1,71 @@
+package net.minestom.server.instance;
+
+
+import net.minestom.testing.Env;
+import net.minestom.testing.EnvTest;
+import org.jetbrains.annotations.NotNull;
+import org.jetbrains.annotations.Nullable;
+import org.junit.jupiter.api.Test;
+
+import java.time.Duration;
+import java.util.concurrent.CompletableFuture;
+import java.util.concurrent.atomic.AtomicReference;
+
+import static org.junit.jupiter.api.Assertions.assertSame;
+
+@EnvTest
+public class InstanceChunkLoadIntegrationTest {
+
+    @Test
+    public void testSyncChunkLoad(Env env) {
+        // Kind of a weird test because the test method is orchestrating ticks, meaning that the test thread is the tick thread.
+
+        var expectedThread = new AtomicReference<Thread>(null);
+        var instance = env.createFlatInstance(new NoopLoader(false));
+        instance.loadChunk(0, 0).thenRun(() -> {
+            // Will run in the thread which completed the future, which should always be the instance tick thread
+            expectedThread.set(Thread.currentThread());
+        });
+        env.tick(); // Tick once to run the scheduled task
+
+        assertSame(Thread.currentThread(), expectedThread.get(), "expected the callback to be executed in the same thread as the tick");
+    }
+
+    @Test
+    public void testAsyncChunkLoad(Env env) {
+        // Kind of a weird test because the test method is orchestrating ticks, meaning that the test thread is the tick thread.
+
+        var expectedThread = new AtomicReference<Thread>(null);
+        var instance = env.createFlatInstance(new NoopLoader(true));
+        instance.loadChunk(0, 0).thenRun(() -> {
+            // Will run in the thread which completed the future, which should always be the instance tick thread
+            expectedThread.set(Thread.currentThread());
+        });
+        env.tickWhile(() -> expectedThread.get() == null, Duration.ofSeconds(5)); // Tick once to run the scheduled task
+
+        assertSame(Thread.currentThread(), expectedThread.get(), "expected the callback to be executed in the same thread as the tick");
+    }
+
+    private static class NoopLoader implements IChunkLoader {
+        private final boolean isParallel;
+
+        private NoopLoader(boolean isParallel) {
+            this.isParallel = isParallel;
+        }
+
+        @Override
+        public boolean supportsParallelLoading() {
+            return isParallel;
+        }
+
+        @Override
+        public @NotNull CompletableFuture<@Nullable Chunk> loadChunk(@NotNull Instance instance, int chunkX, int chunkZ) {
+            return CompletableFuture.completedFuture(instance.getChunkSupplier().createChunk(instance, chunkX, chunkZ));
+        }
+
+        @Override
+        public @NotNull CompletableFuture<Void> saveChunk(@NotNull Chunk chunk) {
+            throw new UnsupportedOperationException("Not implemented");
+        }
+    }
+}