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>
This commit is contained in:
dordsor21 2021-03-03 14:26:00 +00:00 committed by GitHub
parent 6bd866bdf7
commit 7a0dc39eb7
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23

View File

@ -59,11 +59,11 @@ public class ChunkHolder<T extends Future<T>> implements IQueueChunk<T> {
} }
@Override @Override
public void recycle() { public synchronized void recycle() {
delegate = NULL; delegate = NULL;
} }
public IBlockDelegate getDelegate() { public synchronized IBlockDelegate getDelegate() {
return delegate; return delegate;
} }
@ -98,11 +98,13 @@ public class ChunkHolder<T extends Future<T>> implements IQueueChunk<T> {
return delegate.set(this).getBiomes(); return delegate.set(this).getBiomes();
} }
@Override public char[][] getLight() { @Override
public char[][] getLight() {
return delegate.set(this).getLight(); return delegate.set(this).getLight();
} }
@Override public char[][] getSkyLight() { @Override
public char[][] getSkyLight() {
return delegate.set(this).getSkyLight(); return delegate.set(this).getSkyLight();
} }
@ -139,34 +141,36 @@ public class ChunkHolder<T extends Future<T>> implements IQueueChunk<T> {
} }
@Override @Override
public CompoundTag getEntity(UUID uuid) { public synchronized CompoundTag getEntity(UUID uuid) {
return delegate.get(this).getEntity(uuid); return delegate.get(this).getEntity(uuid);
} }
@Override public void setCreateCopy(boolean createCopy) { @Override
public void setCreateCopy(boolean createCopy) {
this.createCopy = createCopy; this.createCopy = createCopy;
} }
@Override public boolean isCreateCopy() { @Override
public boolean isCreateCopy() {
return createCopy; return createCopy;
} }
@Override @Override
public void setLightingToGet(char[][] lighting) { public synchronized void setLightingToGet(char[][] lighting) {
delegate.setLightingToGet(this, lighting); delegate.setLightingToGet(this, lighting);
} }
@Override @Override
public void setSkyLightingToGet(char[][] lighting) { public synchronized void setSkyLightingToGet(char[][] lighting) {
delegate.setSkyLightingToGet(this, lighting); delegate.setSkyLightingToGet(this, lighting);
} }
@Override @Override
public void setHeightmapToGet(HeightMapType type, int[] data) { public synchronized void setHeightmapToGet(HeightMapType type, int[] data) {
delegate.setHeightmapToGet(this, type, data); delegate.setHeightmapToGet(this, type, data);
} }
public void flushLightToGet(boolean heightmaps) { public synchronized void flushLightToGet(boolean heightmaps) {
delegate.flushLightToGet(this, heightmaps); delegate.flushLightToGet(this, heightmaps);
} }
@ -796,22 +800,22 @@ public class ChunkHolder<T extends Future<T>> implements IQueueChunk<T> {
}; };
@Override @Override
public Map<BlockVector3, CompoundTag> getTiles() { public synchronized Map<BlockVector3, CompoundTag> getTiles() {
return delegate.get(this).getTiles(); return delegate.get(this).getTiles();
} }
@Override @Override
public Set<CompoundTag> getEntities() { public synchronized Set<CompoundTag> getEntities() {
return delegate.get(this).getEntities(); return delegate.get(this).getEntities();
} }
@Override @Override
public boolean hasSection(int layer) { public synchronized boolean hasSection(int layer) {
return chunkExisting != null && chunkExisting.hasSection(layer); return chunkExisting != null && chunkExisting.hasSection(layer);
} }
@Override @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 IChunkGet get = getOrCreateGet();
final IChunkSet set = getOrCreateSet(); final IChunkSet set = getOrCreateSet();
set.setFastMode(fastmode); set.setFastMode(fastmode);
@ -823,7 +827,7 @@ public class ChunkHolder<T extends Future<T>> implements IQueueChunk<T> {
} }
@Override @Override
public boolean trim(boolean aggressive) { public synchronized boolean trim(boolean aggressive) {
if (chunkSet != null) { if (chunkSet != null) {
final boolean result = chunkSet.trim(aggressive); final boolean result = chunkSet.trim(aggressive);
if (result) { if (result) {
@ -847,7 +851,7 @@ public class ChunkHolder<T extends Future<T>> implements IQueueChunk<T> {
} }
@Override @Override
public boolean trim(boolean aggressive, int layer) { public synchronized boolean trim(boolean aggressive, int layer) {
return this.trim(aggressive); return this.trim(aggressive);
} }
@ -859,7 +863,7 @@ public class ChunkHolder<T extends Future<T>> implements IQueueChunk<T> {
/** /**
* Get or create the existing part of this chunk. * Get or create the existing part of this chunk.
*/ */
public final IChunkGet getOrCreateGet() { public synchronized final IChunkGet getOrCreateGet() {
if (chunkExisting == null) { if (chunkExisting == null) {
chunkExisting = newWrappedGet(); chunkExisting = newWrappedGet();
} }
@ -869,7 +873,7 @@ public class ChunkHolder<T extends Future<T>> implements IQueueChunk<T> {
/** /**
* Get or create the settable part of this chunk. * Get or create the settable part of this chunk.
*/ */
public final IChunkSet getOrCreateSet() { public synchronized final IChunkSet getOrCreateSet() {
if (chunkSet == null) { if (chunkSet == null) {
chunkSet = newWrappedSet(); chunkSet = newWrappedSet();
} }
@ -881,7 +885,7 @@ public class ChunkHolder<T extends Future<T>> implements IQueueChunk<T> {
* - The purpose of wrapping is to allow different extents to intercept / alter behavior * - The purpose of wrapping is to allow different extents to intercept / alter behavior
* - e.g., caching, optimizations, filtering * - e.g., caching, optimizations, filtering
*/ */
private IChunkSet newWrappedSet() { private synchronized IChunkSet newWrappedSet() {
return extent.getCachedSet(chunkX, chunkZ); return extent.getCachedSet(chunkX, chunkZ);
} }
@ -890,12 +894,12 @@ public class ChunkHolder<T extends Future<T>> implements IQueueChunk<T> {
* - The purpose of wrapping is to allow different extents to intercept / alter behavior * - The purpose of wrapping is to allow different extents to intercept / alter behavior
* - e.g., caching, optimizations, filtering * - e.g., caching, optimizations, filtering
*/ */
private IChunkGet newWrappedGet() { private synchronized IChunkGet newWrappedGet() {
return extent.getCachedGet(chunkX, chunkZ); return extent.getCachedGet(chunkX, chunkZ);
} }
@Override @Override
public <V extends IChunk> void init(IQueueExtent<V> extent, int chunkX, int chunkZ) { public synchronized <V extends IChunk> void init(IQueueExtent<V> extent, int chunkX, int chunkZ) {
this.extent = extent; this.extent = extent;
this.chunkX = chunkX; this.chunkX = chunkX;
this.chunkZ = chunkZ; this.chunkZ = chunkZ;
@ -913,13 +917,17 @@ public class ChunkHolder<T extends Future<T>> implements IQueueChunk<T> {
public synchronized T call() { public synchronized T call() {
if (chunkSet != null) { if (chunkSet != null) {
chunkSet.setBitMask(bitMask); chunkSet.setBitMask(bitMask);
return this.call(chunkSet, this::recycle); return this.call(chunkSet, () -> {
synchronized (this) {
this.recycle();
}
});
} }
return null; return null;
} }
@Override @Override
public T call(IChunkSet set, Runnable finalize) { public synchronized T call(IChunkSet set, Runnable finalize) {
if (set != null) { if (set != null) {
IChunkGet get = getOrCreateGet(); IChunkGet get = getOrCreateGet();
get.trim(false); get.trim(false);
@ -955,79 +963,82 @@ public class ChunkHolder<T extends Future<T>> implements IQueueChunk<T> {
} }
@Override @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); return delegate.setBiome(this, x, y, z, biome);
} }
@Override @Override
public <B extends BlockStateHolder<B>> boolean setBlock(int x, int y, int z, B block) { public synchronized <B extends BlockStateHolder<B>> boolean setBlock(int x, int y, int z, B block) {
return delegate.setBlock(this, x, y, z, block); return delegate.setBlock(this, x, y, z, block);
} }
@Override @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); return delegate.getBiome(this, x, y, z);
} }
@Override @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); return delegate.getBlock(this, x, y, z);
} }
@Override @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); return delegate.getFullBlock(this, x, y, z);
} }
@Override @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); 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); 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); delegate.removeSectionLighting(this, layer, sky);
} }
@Override public void setFullBright(int layer) { @Override
public synchronized void setFullBright(int layer) {
delegate.setFullBright(this, layer); delegate.setFullBright(this, layer);
} }
@Override @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); delegate.setBlockLight(this, x, y, z, value);
} }
@Override @Override
public void setLightLayer(int layer, char[] toSet) { public synchronized void setLightLayer(int layer, char[] toSet) {
delegate.setLightLayer(this, layer, toSet); delegate.setLightLayer(this, layer, toSet);
} }
@Override @Override
public void setSkyLightLayer(int layer, char[] toSet) { public synchronized void setSkyLightLayer(int layer, char[] toSet) {
delegate.setSkyLightLayer(this, layer, toSet); delegate.setSkyLightLayer(this, layer, toSet);
} }
@Override @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); return delegate.getSkyLight(this, x, y, z);
} }
@Override @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); return delegate.getEmmittedLight(this, x, y, z);
} }
@Override @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); return delegate.getBrightness(this, x, y, z);
} }
@Override @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); return delegate.getOpacity(this, x, y, z);
} }