Fixed synchronization with unloaded chunks

src/main/java/net/minestom/server/UpdateManager.java

@@ -98,7 +98,7 @@ public final class UpdateManager {
      * @param threadProvider the new thread provider
      * @throws NullPointerException if {@param threadProvider} is null
      */
-    public void setThreadProvider(ThreadProvider threadProvider) {
+    public synchronized void setThreadProvider(ThreadProvider threadProvider) {
         Check.notNull(threadProvider, "The thread provider cannot be null");
         this.threadProvider = threadProvider;
     }

src/main/java/net/minestom/server/collision/CollisionUtils.java

@@ -1,11 +1,13 @@
 package net.minestom.server.collision;
 
 import net.minestom.server.entity.Entity;
+import net.minestom.server.instance.Chunk;
 import net.minestom.server.instance.Instance;
 import net.minestom.server.instance.block.Block;
 import net.minestom.server.utils.BlockPosition;
 import net.minestom.server.utils.Position;
 import net.minestom.server.utils.Vector;
+import net.minestom.server.utils.chunk.ChunkUtils;
 
 public class CollisionUtils {
 
@@ -138,7 +140,14 @@ public class CollisionUtils {
             blockPos.setX((int) Math.floor(corner.getX()));
             blockPos.setY((int) Math.floor(corner.getY()));
             blockPos.setZ((int) Math.floor(corner.getZ()));
-            final short blockStateId = instance.getBlockStateId(blockPos);
+
+            final Chunk chunk = instance.getChunkAt(blockPos);
+            if (ChunkUtils.isChunkUnloaded(chunk)) {
+                // Collision at chunk border
+                return false;
+            }
+
+            final short blockStateId = chunk.getBlockStateId(blockPos.getX(), blockPos.getY(), blockPos.getZ());
             final Block block = Block.fromStateId(blockStateId);
 
             // TODO: block collision boxes

src/main/java/net/minestom/server/entity/EntityCreature.java

@@ -346,7 +346,13 @@ public abstract class EntityCreature extends LivingEntity {
             return false;
         }
 
+        if (pathFinder == null) {
+            // Unexpected error
+            return false;
+        }
+
         this.pathLock.lock();
+
         this.pathFinder.reset();
         if (position == null) {
             this.pathLock.unlock();

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

@@ -70,7 +70,7 @@ public final class Chunk implements Viewable {
     private PFColumnarSpace columnarSpace;
 
     // Cache
-    private boolean loaded = true;
+    private volatile boolean loaded = true;
     private Set<Player> viewers = new CopyOnWriteArraySet<>();
     private ByteBuf fullDataPacket;
 

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

@@ -134,9 +134,11 @@ public abstract class Instance implements BlockModifier, EventHandler, DataConta
     public abstract void loadOptionalChunk(int chunkX, int chunkZ, Consumer<Chunk> callback);
 
     /**
-     * Unload a chunk
+     * Schedule the removal of a chunk, this method does not promise when it will be done
      * <p>
-     * WARNING: all entities other than {@link Player} will be removed
+     * WARNING: during unloading, all entities other than {@link Player} will be removed
+     * <p>
+     * For {@link InstanceContainer} it is done during {@link InstanceContainer#tick(long)}
      *
      * @param chunk the chunk to unload
      */

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

@@ -53,6 +53,7 @@ public class InstanceContainer extends Instance {
 
     private ChunkGenerator chunkGenerator;
     private Map<Long, Chunk> chunks = new ConcurrentHashMap<>();
+    private Set<Chunk> scheduledChunksToRemove = new HashSet<>();
 
     private ReadWriteLock changingBlockLock = new ReentrantReadWriteLock();
     private Map<BlockPosition, Block> currentlyChangingBlocks = new HashMap<>();
@@ -315,35 +316,13 @@ public class InstanceContainer extends Instance {
 
     @Override
     public void unloadChunk(Chunk chunk) {
-        final int chunkX = chunk.getChunkX();
-        final int chunkZ = chunk.getChunkZ();
-
-        final long index = ChunkUtils.getChunkIndex(chunkX, chunkZ);
-
-        UnloadChunkPacket unloadChunkPacket = new UnloadChunkPacket();
-        unloadChunkPacket.chunkX = chunkX;
-        unloadChunkPacket.chunkZ = chunkZ;
-        chunk.sendPacketToViewers(unloadChunkPacket);
-
-        for (Player viewer : chunk.getViewers()) {
-            chunk.removeViewer(viewer);
+        if (ChunkUtils.isChunkUnloaded(chunk)) {
+            return;
+        }
+        // Schedule the chunk removal
+        synchronized (this.scheduledChunksToRemove) {
+            this.scheduledChunksToRemove.add(chunk);
         }
-
-        callChunkUnloadEvent(chunkX, chunkZ);
-
-        // Remove all entities in chunk
-        getChunkEntities(chunk).forEach(entity -> {
-            if (!(entity instanceof Player))
-                entity.remove();
-        });
-
-        // Clear cache
-        this.chunks.remove(index);
-        this.chunkEntities.remove(index);
-
-        chunk.unload();
-
-        UPDATE_MANAGER.signalChunkUnload(this, chunkX, chunkZ);
     }
 
     @Override
@@ -607,6 +586,9 @@ public class InstanceContainer extends Instance {
 
     @Override
     public void tick(long time) {
+        // Unload all waiting chunks
+        UNSAFE_unloadChunks();
+
         super.tick(time);
         Lock wrlock = changingBlockLock.writeLock();
         wrlock.lock();
@@ -614,6 +596,48 @@ public class InstanceContainer extends Instance {
         wrlock.unlock();
     }
 
+    /**
+     * Unload all waiting chunks
+     * <p>
+     * Unsafe because it has to be done on the same thread as the instance/chunks tick update
+     */
+    private void UNSAFE_unloadChunks() {
+        synchronized (this.scheduledChunksToRemove) {
+            for (Chunk chunk : scheduledChunksToRemove) {
+                final int chunkX = chunk.getChunkX();
+                final int chunkZ = chunk.getChunkZ();
+
+                final long index = ChunkUtils.getChunkIndex(chunkX, chunkZ);
+
+                UnloadChunkPacket unloadChunkPacket = new UnloadChunkPacket();
+                unloadChunkPacket.chunkX = chunkX;
+                unloadChunkPacket.chunkZ = chunkZ;
+                chunk.sendPacketToViewers(unloadChunkPacket);
+
+                for (Player viewer : chunk.getViewers()) {
+                    chunk.removeViewer(viewer);
+                }
+
+                callChunkUnloadEvent(chunkX, chunkZ);
+
+                // Remove all entities in chunk
+                getChunkEntities(chunk).forEach(entity -> {
+                    if (!(entity instanceof Player))
+                        entity.remove();
+                });
+
+                // Clear cache
+                this.chunks.remove(index);
+                this.chunkEntities.remove(index);
+
+                chunk.unload();
+
+                UPDATE_MANAGER.signalChunkUnload(this, chunkX, chunkZ);
+            }
+            this.scheduledChunksToRemove.clear();
+        }
+    }
+
     private void callChunkLoadEvent(int chunkX, int chunkZ) {
         InstanceChunkLoadEvent chunkLoadEvent = new InstanceChunkLoadEvent(this, chunkX, chunkZ);
         callEvent(InstanceChunkLoadEvent.class, chunkLoadEvent);

src/main/java/net/minestom/server/thread/PerGroupChunkProvider.java

@@ -7,6 +7,7 @@ import net.minestom.server.utils.chunk.ChunkUtils;
 import java.util.*;
 import java.util.concurrent.ConcurrentHashMap;
 import java.util.concurrent.CopyOnWriteArraySet;
+import java.util.concurrent.atomic.AtomicBoolean;
 
 /**
  * Separate chunks into group of linked chunks
@@ -106,15 +107,24 @@ public class PerGroupChunkProvider extends ThreadProvider {
             final Instance instance = entry.getKey();
             final Map<Set<ChunkCoordinate>, Instance> instanceMap = entry.getValue();
 
+            // True if the instance ended its tick call
+            AtomicBoolean instanceUpdated = new AtomicBoolean(false);
+
             // Update all the chunks + instances
             for (Map.Entry<Set<ChunkCoordinate>, Instance> ent : instanceMap.entrySet()) {
                 final Set<ChunkCoordinate> chunks = ent.getKey();
 
-                final boolean updateInstance = updatedInstance.add(instance);
+                final boolean shouldUpdateInstance = updatedInstance.add(instance);
                 pool.execute(() -> {
                     // Used to check if the instance has already been updated this tick
-                    if (updateInstance) {
+                    if (shouldUpdateInstance) {
                         updateInstance(instance, time);
+                        instanceUpdated.set(true);
+                    }
+
+                    // Wait for the instance to be updated
+                    // Needed because the instance tick is used to unload waiting chunks
+                    while (!instanceUpdated.get()) {
                     }
 
                     for (ChunkCoordinate chunkCoordinate : chunks) {

src/main/java/net/minestom/server/utils/chunk/ChunkUtils.java

@@ -13,6 +13,8 @@ public final class ChunkUtils {
     }
 
     /**
+     * Get if a chunk is unloaded
+     *
      * @param chunk the chunk to check
      * @return true if the chunk is unloaded, false otherwise
      */
@@ -21,6 +23,8 @@ public final class ChunkUtils {
     }
 
     /**
+     * Get if a chunk is unloaded
+     *
      * @param instance the instance to check
      * @param x        instance X coordinate
      * @param z        instance Z coordinate