From f3a6e82f96c2d39b6f0b1066bb2c431addfcbb7d Mon Sep 17 00:00:00 2001 From: Jason Mitchell Date: Mon, 18 Jan 2021 13:33:49 -0800 Subject: Do some locking around getTileEntity to avoid CMEs on the main thread --- .../threads/GT_Runnable_MachineBlockUpdate.java | 23 +++++++++++++++++++++- 1 file changed, 22 insertions(+), 1 deletion(-) (limited to 'src/main/java/gregtech/api') diff --git a/src/main/java/gregtech/api/threads/GT_Runnable_MachineBlockUpdate.java b/src/main/java/gregtech/api/threads/GT_Runnable_MachineBlockUpdate.java index 8f7a84e2cb..494b21de1f 100644 --- a/src/main/java/gregtech/api/threads/GT_Runnable_MachineBlockUpdate.java +++ b/src/main/java/gregtech/api/threads/GT_Runnable_MachineBlockUpdate.java @@ -1,5 +1,8 @@ package gregtech.api.threads; + +import cpw.mods.fml.common.eventhandler.SubscribeEvent; +import cpw.mods.fml.common.gameevent.TickEvent; import gregtech.GT_Mod; import gregtech.api.GregTech_API; import gregtech.api.interfaces.tileentity.IMachineBlockUpdateable; @@ -15,6 +18,7 @@ import java.util.concurrent.ExecutorService; import java.util.concurrent.Executors; import java.util.concurrent.ThreadFactory; import java.util.concurrent.TimeUnit; +import java.util.concurrent.locks.ReentrantLock; public class GT_Runnable_MachineBlockUpdate implements Runnable { // used by runner thread @@ -23,6 +27,9 @@ public class GT_Runnable_MachineBlockUpdate implements Runnable { private final Set visited = new HashSet<>(80); private final Queue tQueue = new LinkedList<>(); + // Locking + private static ReentrantLock lock = new ReentrantLock(); + // Threading private static final ThreadFactory THREAD_FACTORY = r -> { Thread thread = new Thread(r); @@ -40,6 +47,15 @@ public class GT_Runnable_MachineBlockUpdate implements Runnable { } + @SubscribeEvent + public void onServerTick(TickEvent.ServerTickEvent aEvent) { + if (aEvent.phase == TickEvent.Phase.START) { + lock.lock(); + } else { + lock.unlock(); + } + } + public static boolean isEnabled() { return isEnabled; } @@ -101,8 +117,13 @@ public class GT_Runnable_MachineBlockUpdate implements Runnable { try { while (!tQueue.isEmpty()) { final ChunkCoordinates aCoords = tQueue.poll(); + + // This might load a chunk... which might load a TileEntity... which might get added to `loadedTileEntityList`... which might be in the process + // of being iterated over during `UpdateEntities()`... which might cause a ConcurrentModificationException. So, lock that shit. + lock.lock(); TileEntity tTileEntity = world.getTileEntity(aCoords.posX, aCoords.posY, aCoords.posZ); - + lock.unlock(); + // See if the block itself needs an update if (tTileEntity instanceof IMachineBlockUpdateable) ((IMachineBlockUpdateable) tTileEntity).onMachineBlockUpdate(); -- cgit From 94c98540f8a6044aa3e27722218f8eeb395e9cb1 Mon Sep 17 00:00:00 2001 From: Jason Mitchell Date: Mon, 18 Jan 2021 17:20:42 -0800 Subject: Include getBlock and getBlockMetadata in the lock --- .../java/gregtech/api/threads/GT_Runnable_MachineBlockUpdate.java | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) (limited to 'src/main/java/gregtech/api') diff --git a/src/main/java/gregtech/api/threads/GT_Runnable_MachineBlockUpdate.java b/src/main/java/gregtech/api/threads/GT_Runnable_MachineBlockUpdate.java index 494b21de1f..09b2801c79 100644 --- a/src/main/java/gregtech/api/threads/GT_Runnable_MachineBlockUpdate.java +++ b/src/main/java/gregtech/api/threads/GT_Runnable_MachineBlockUpdate.java @@ -121,7 +121,8 @@ public class GT_Runnable_MachineBlockUpdate implements Runnable { // This might load a chunk... which might load a TileEntity... which might get added to `loadedTileEntityList`... which might be in the process // of being iterated over during `UpdateEntities()`... which might cause a ConcurrentModificationException. So, lock that shit. lock.lock(); - TileEntity tTileEntity = world.getTileEntity(aCoords.posX, aCoords.posY, aCoords.posZ); + final TileEntity tTileEntity = world.getTileEntity(aCoords.posX, aCoords.posY, aCoords.posZ); + final boolean isMachineBlock = GregTech_API.isMachineBlock(world.getBlock(aCoords.posX, aCoords.posY, aCoords.posZ), world.getBlockMetadata(aCoords.posX, aCoords.posY, aCoords.posZ)); lock.unlock(); // See if the block itself needs an update @@ -134,7 +135,7 @@ public class GT_Runnable_MachineBlockUpdate implements Runnable { // 3) If the block at the coordinates is marked as a machine block if (visited.size() < 5 || (tTileEntity instanceof IMachineBlockUpdateable && ((IMachineBlockUpdateable) tTileEntity).isMachineBlockUpdateRecursive()) - || GregTech_API.isMachineBlock(world.getBlock(aCoords.posX, aCoords.posY, aCoords.posZ), world.getBlockMetadata(aCoords.posX, aCoords.posY, aCoords.posZ))) + || isMachineBlock) { ChunkCoordinates tCoords; -- cgit From 6d58f65939261a915761c11ecf01848c2cef451c Mon Sep 17 00:00:00 2001 From: Jason Mitchell Date: Mon, 18 Jan 2021 18:19:29 -0800 Subject: try/finally --- .../gregtech/api/threads/GT_Runnable_MachineBlockUpdate.java | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) (limited to 'src/main/java/gregtech/api') diff --git a/src/main/java/gregtech/api/threads/GT_Runnable_MachineBlockUpdate.java b/src/main/java/gregtech/api/threads/GT_Runnable_MachineBlockUpdate.java index 09b2801c79..e46d63cdff 100644 --- a/src/main/java/gregtech/api/threads/GT_Runnable_MachineBlockUpdate.java +++ b/src/main/java/gregtech/api/threads/GT_Runnable_MachineBlockUpdate.java @@ -117,13 +117,18 @@ public class GT_Runnable_MachineBlockUpdate implements Runnable { try { while (!tQueue.isEmpty()) { final ChunkCoordinates aCoords = tQueue.poll(); + final TileEntity tTileEntity; + final boolean isMachineBlock; // This might load a chunk... which might load a TileEntity... which might get added to `loadedTileEntityList`... which might be in the process // of being iterated over during `UpdateEntities()`... which might cause a ConcurrentModificationException. So, lock that shit. lock.lock(); - final TileEntity tTileEntity = world.getTileEntity(aCoords.posX, aCoords.posY, aCoords.posZ); - final boolean isMachineBlock = GregTech_API.isMachineBlock(world.getBlock(aCoords.posX, aCoords.posY, aCoords.posZ), world.getBlockMetadata(aCoords.posX, aCoords.posY, aCoords.posZ)); - lock.unlock(); + try { + tTileEntity = world.getTileEntity(aCoords.posX, aCoords.posY, aCoords.posZ); + isMachineBlock = GregTech_API.isMachineBlock(world.getBlock(aCoords.posX, aCoords.posY, aCoords.posZ), world.getBlockMetadata(aCoords.posX, aCoords.posY, aCoords.posZ)); + } finally { + lock.unlock(); + } // See if the block itself needs an update if (tTileEntity instanceof IMachineBlockUpdateable) -- cgit From 17225fb5d3727f677121ec6017d5f09b632af9a9 Mon Sep 17 00:00:00 2001 From: Jason Mitchell Date: Mon, 18 Jan 2021 18:26:37 -0800 Subject: add comment as to onServerTick vs onWorldTick --- src/main/java/gregtech/api/threads/GT_Runnable_MachineBlockUpdate.java | 1 + 1 file changed, 1 insertion(+) (limited to 'src/main/java/gregtech/api') diff --git a/src/main/java/gregtech/api/threads/GT_Runnable_MachineBlockUpdate.java b/src/main/java/gregtech/api/threads/GT_Runnable_MachineBlockUpdate.java index e46d63cdff..6564cf316a 100644 --- a/src/main/java/gregtech/api/threads/GT_Runnable_MachineBlockUpdate.java +++ b/src/main/java/gregtech/api/threads/GT_Runnable_MachineBlockUpdate.java @@ -49,6 +49,7 @@ public class GT_Runnable_MachineBlockUpdate implements Runnable { @SubscribeEvent public void onServerTick(TickEvent.ServerTickEvent aEvent) { + // Using onServerTick because there's race conditions in updateTrackedEntities() which is called AFTER the END phase of onWorldTick if (aEvent.phase == TickEvent.Phase.START) { lock.lock(); } else { -- cgit