From 7a0dc39eb7eccd0d64c03f0bf633a95ef5b12753 Mon Sep 17 00:00:00 2001 From: dordsor21 Date: Wed, 3 Mar 2021 14:26:00 +0000 Subject: [PATCH] Apply a lot of synchronization to ChunkHolder (#941) This is basically the main "chunk" class for internal FAWE. Chunk operations should (and are) almost always single-threaded operations, however, under certain circumstances it is possible for the chunk to be "called" (flushed: written to the world and sent to the player) from a separate thread. This would specifically occur from SingleThreadQueueExtent when there are a lot of chunks being loaded in memory by FAWE (where the chunk would then be submitted to a multi-threaded queue). It would therefore be possible for a thread accessing the chunk to attempt to access it in the middle of the call, which can lead to a number of issues, and it is my opinion that the most frequent of these is the NPE seen during lighting operations, where new chunks can be accessed/loaded very quickly, increasing the likelihood for the aforementioned synchronisation issue to occur. Co-authored-by: Matt <4009945+MattBDev@users.noreply.github.com> --- .../implementation/chunk/ChunkHolder.java | 91 +++++++++++-------- 1 file changed, 51 insertions(+), 40 deletions(-) diff --git a/worldedit-core/src/main/java/com/boydti/fawe/beta/implementation/chunk/ChunkHolder.java b/worldedit-core/src/main/java/com/boydti/fawe/beta/implementation/chunk/ChunkHolder.java index c6380b549..932f900a1 100644 --- a/worldedit-core/src/main/java/com/boydti/fawe/beta/implementation/chunk/ChunkHolder.java +++ b/worldedit-core/src/main/java/com/boydti/fawe/beta/implementation/chunk/ChunkHolder.java @@ -59,11 +59,11 @@ public class ChunkHolder> implements IQueueChunk { } @Override - public void recycle() { + public synchronized void recycle() { delegate = NULL; } - public IBlockDelegate getDelegate() { + public synchronized IBlockDelegate getDelegate() { return delegate; } @@ -98,11 +98,13 @@ public class ChunkHolder> implements IQueueChunk { return delegate.set(this).getBiomes(); } - @Override public char[][] getLight() { + @Override + public char[][] getLight() { return delegate.set(this).getLight(); } - @Override public char[][] getSkyLight() { + @Override + public char[][] getSkyLight() { return delegate.set(this).getSkyLight(); } @@ -139,34 +141,36 @@ public class ChunkHolder> implements IQueueChunk { } @Override - public CompoundTag getEntity(UUID uuid) { + public synchronized CompoundTag getEntity(UUID uuid) { return delegate.get(this).getEntity(uuid); } - @Override public void setCreateCopy(boolean createCopy) { + @Override + public void setCreateCopy(boolean createCopy) { this.createCopy = createCopy; } - @Override public boolean isCreateCopy() { + @Override + public boolean isCreateCopy() { return createCopy; } @Override - public void setLightingToGet(char[][] lighting) { + public synchronized void setLightingToGet(char[][] lighting) { delegate.setLightingToGet(this, lighting); } @Override - public void setSkyLightingToGet(char[][] lighting) { + public synchronized void setSkyLightingToGet(char[][] lighting) { delegate.setSkyLightingToGet(this, lighting); } @Override - public void setHeightmapToGet(HeightMapType type, int[] data) { + public synchronized void setHeightmapToGet(HeightMapType type, int[] data) { delegate.setHeightmapToGet(this, type, data); } - public void flushLightToGet(boolean heightmaps) { + public synchronized void flushLightToGet(boolean heightmaps) { delegate.flushLightToGet(this, heightmaps); } @@ -796,22 +800,22 @@ public class ChunkHolder> implements IQueueChunk { }; @Override - public Map getTiles() { + public synchronized Map getTiles() { return delegate.get(this).getTiles(); } @Override - public Set getEntities() { + public synchronized Set getEntities() { return delegate.get(this).getEntities(); } @Override - public boolean hasSection(int layer) { + public synchronized boolean hasSection(int layer) { return chunkExisting != null && chunkExisting.hasSection(layer); } @Override - public void filterBlocks(Filter filter, ChunkFilterBlock block, @Nullable Region region, boolean full) { + public synchronized void filterBlocks(Filter filter, ChunkFilterBlock block, @Nullable Region region, boolean full) { final IChunkGet get = getOrCreateGet(); final IChunkSet set = getOrCreateSet(); set.setFastMode(fastmode); @@ -823,7 +827,7 @@ public class ChunkHolder> implements IQueueChunk { } @Override - public boolean trim(boolean aggressive) { + public synchronized boolean trim(boolean aggressive) { if (chunkSet != null) { final boolean result = chunkSet.trim(aggressive); if (result) { @@ -847,7 +851,7 @@ public class ChunkHolder> implements IQueueChunk { } @Override - public boolean trim(boolean aggressive, int layer) { + public synchronized boolean trim(boolean aggressive, int layer) { return this.trim(aggressive); } @@ -859,7 +863,7 @@ public class ChunkHolder> implements IQueueChunk { /** * Get or create the existing part of this chunk. */ - public final IChunkGet getOrCreateGet() { + public synchronized final IChunkGet getOrCreateGet() { if (chunkExisting == null) { chunkExisting = newWrappedGet(); } @@ -869,7 +873,7 @@ public class ChunkHolder> implements IQueueChunk { /** * Get or create the settable part of this chunk. */ - public final IChunkSet getOrCreateSet() { + public synchronized final IChunkSet getOrCreateSet() { if (chunkSet == null) { chunkSet = newWrappedSet(); } @@ -881,7 +885,7 @@ public class ChunkHolder> implements IQueueChunk { * - The purpose of wrapping is to allow different extents to intercept / alter behavior * - e.g., caching, optimizations, filtering */ - private IChunkSet newWrappedSet() { + private synchronized IChunkSet newWrappedSet() { return extent.getCachedSet(chunkX, chunkZ); } @@ -890,12 +894,12 @@ public class ChunkHolder> implements IQueueChunk { * - The purpose of wrapping is to allow different extents to intercept / alter behavior * - e.g., caching, optimizations, filtering */ - private IChunkGet newWrappedGet() { + private synchronized IChunkGet newWrappedGet() { return extent.getCachedGet(chunkX, chunkZ); } @Override - public void init(IQueueExtent extent, int chunkX, int chunkZ) { + public synchronized void init(IQueueExtent extent, int chunkX, int chunkZ) { this.extent = extent; this.chunkX = chunkX; this.chunkZ = chunkZ; @@ -913,13 +917,17 @@ public class ChunkHolder> implements IQueueChunk { public synchronized T call() { if (chunkSet != null) { chunkSet.setBitMask(bitMask); - return this.call(chunkSet, this::recycle); + return this.call(chunkSet, () -> { + synchronized (this) { + this.recycle(); + } + }); } return null; } @Override - public T call(IChunkSet set, Runnable finalize) { + public synchronized T call(IChunkSet set, Runnable finalize) { if (set != null) { IChunkGet get = getOrCreateGet(); get.trim(false); @@ -955,79 +963,82 @@ public class ChunkHolder> implements IQueueChunk { } @Override - public boolean setBiome(int x, int y, int z, BiomeType biome) { + public synchronized boolean setBiome(int x, int y, int z, BiomeType biome) { return delegate.setBiome(this, x, y, z, biome); } @Override - public > boolean setBlock(int x, int y, int z, B block) { + public synchronized > boolean setBlock(int x, int y, int z, B block) { return delegate.setBlock(this, x, y, z, block); } @Override - public BiomeType getBiomeType(int x, int y, int z) { + public synchronized BiomeType getBiomeType(int x, int y, int z) { return delegate.getBiome(this, x, y, z); } @Override - public BlockState getBlock(int x, int y, int z) { + public synchronized BlockState getBlock(int x, int y, int z) { return delegate.getBlock(this, x, y, z); } @Override - public BaseBlock getFullBlock(int x, int y, int z) { + public synchronized BaseBlock getFullBlock(int x, int y, int z) { return delegate.getFullBlock(this, x, y, z); } @Override - public void setSkyLight(int x, int y, int z, int value) { + public synchronized void setSkyLight(int x, int y, int z, int value) { delegate.setSkyLight(this, x, y, z, value); } - @Override public void setHeightMap(HeightMapType type, int[] heightMap) { + @Override + public synchronized void setHeightMap(HeightMapType type, int[] heightMap) { delegate.setHeightMap(this, type, heightMap); } - @Override public void removeSectionLighting(int layer, boolean sky) { + @Override + public synchronized void removeSectionLighting(int layer, boolean sky) { delegate.removeSectionLighting(this, layer, sky); } - @Override public void setFullBright(int layer) { + @Override + public synchronized void setFullBright(int layer) { delegate.setFullBright(this, layer); } @Override - public void setBlockLight(int x, int y, int z, int value) { + public synchronized void setBlockLight(int x, int y, int z, int value) { delegate.setBlockLight(this, x, y, z, value); } @Override - public void setLightLayer(int layer, char[] toSet) { + public synchronized void setLightLayer(int layer, char[] toSet) { delegate.setLightLayer(this, layer, toSet); } @Override - public void setSkyLightLayer(int layer, char[] toSet) { + public synchronized void setSkyLightLayer(int layer, char[] toSet) { delegate.setSkyLightLayer(this, layer, toSet); } @Override - public int getSkyLight(int x, int y, int z) { + public synchronized int getSkyLight(int x, int y, int z) { return delegate.getSkyLight(this, x, y, z); } @Override - public int getEmmittedLight(int x, int y, int z) { + public synchronized int getEmmittedLight(int x, int y, int z) { return delegate.getEmmittedLight(this, x, y, z); } @Override - public int getBrightness(int x, int y, int z) { + public synchronized int getBrightness(int x, int y, int z) { return delegate.getBrightness(this, x, y, z); } @Override - public int getOpacity(int x, int y, int z) { + public synchronized int getOpacity(int x, int y, int z) { return delegate.getOpacity(this, x, y, z); }