From 6f01aa51ffa1dbfad84f8715abbf643e0c187eda Mon Sep 17 00:00:00 2001 From: mworzala Date: Thu, 21 Mar 2024 15:29:03 -0400 Subject: [PATCH] fix: always execute chunk load callback/event on the instance tick thread --- .../server/instance/InstanceContainer.java | 24 ++++--- .../InstanceChunkLoadIntegrationTest.java | 71 +++++++++++++++++++ 2 files changed, 87 insertions(+), 8 deletions(-) create mode 100644 src/test/java/net/minestom/server/instance/InstanceChunkLoadIntegrationTest.java diff --git a/src/main/java/net/minestom/server/instance/InstanceContainer.java b/src/main/java/net/minestom/server/instance/InstanceContainer.java index bb3569025..c0934143c 100644 --- a/src/main/java/net/minestom/server/instance/InstanceContainer.java +++ b/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 prev = loadingChunks.putIfAbsent(index, completableFuture); if (prev != null) return prev; final IChunkLoader loader = chunkLoader; + final Consumer postLoadCallback = chunk -> { + cacheChunk(chunk); + chunk.onLoad(); + + EventDispatcher.call(new InstanceChunkLoadEvent(this, chunk)); + final CompletableFuture 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 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); diff --git a/src/test/java/net/minestom/server/instance/InstanceChunkLoadIntegrationTest.java b/src/test/java/net/minestom/server/instance/InstanceChunkLoadIntegrationTest.java new file mode 100644 index 000000000..7998ca1ce --- /dev/null +++ b/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(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(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 saveChunk(@NotNull Chunk chunk) { + throw new UnsupportedOperationException("Not implemented"); + } + } +}