Applied Energistics 2

Applied Energistics 2

156M Downloads

API crafting callback on wrong thread?

uecasm opened this issue ยท 3 comments

commented

Describe the bug
Using the API to call ICraftingGrid.beginCraftingJob, and then inside the ICraftingCallback.calculationComplete I'm trying to call ICraftingGrid.submitJob, but it deadlocks the server while trying to update chunks.

I'm not sure if I'm doing something wrong or you are, or if this is entirely expected, but it seems wrong at least...

I can successfully avoid the deadlock if I post a task to call submitJob to the server tick thread instead of calling it directly from calculationComplete.

(FWIW, I had a similar deadlock when trying to call something that updated BlockState from various IGridNode connection-related events; as well as from TileEntity.validate during startup. Offloading to server tick solved that too.)

Expected behaviour
No deadlock. In particular the documentation for calculationComplete states that it's "synchronised with the world", which I assume means that it shouldn't deadlock on chunk loading.

Essentially, it probably shouldn't be getting called on the AE Crafting Calculator thread at all.

Environment
ForgeGradle dev environment of an unreleased addon mod.
Minecraft version 1.16.4
Mappings version 20201028-1.16.3
Forge version 35.1.4
AE2 version 8.2.0-beta.1

Call stack of the "AE Crafting Calculator" thread (which is where the callback gets fired) at the time of the deadlock:

park:-1, Unsafe (sun.misc)
park:175, LockSupport (java.util.concurrent.locks)
block:1707, CompletableFuture$Signaller (java.util.concurrent)
managedBlock:3334, ForkJoinPool (java.util.concurrent)
waitingGet:1742, CompletableFuture (java.util.concurrent)
join:1947, CompletableFuture (java.util.concurrent)
getChunk:113, ServerChunkProvider (net.minecraft.world.server)
getChunk:185, World (net.minecraft.world)
getChunk:116, IWorldReader (net.minecraft.world)
getChunk:181, World (net.minecraft.world)
getChunkAt:177, World (net.minecraft.world)
markChunkDirty:867, World (net.minecraft.world)
saveChanges:437, DriveTileEntity (appeng.tile.storage)
saveChanges:153, AbstractCellInventory (appeng.me.storage)
extractItems:197, BasicCellInventory (appeng.me.storage)
extractItems:55, MEPassThrough (appeng.me.storage)
extractItems:100, MEInventoryHandler (appeng.me.storage)
extractItems:100, MEInventoryHandler (appeng.me.storage)
extractItems:70, DriveWatcher (appeng.me.storage)
extractItems:196, NetworkInventoryHandler (appeng.me.storage)
extractItems:89, NetworkMonitor (appeng.me.cache)
commit:254, MECraftingInventory (appeng.crafting)
submitJob:788, CraftingCPUCluster (appeng.me.cluster.implementations)
submitJob:490, CraftingGridCache (appeng.me.cache)
doCalculationComplete:112, CraftingCallback (testmod.integration.ae2)
calculationComplete:79, CraftingCallback (testmod.integration.ae2)
finish:226, CraftingJob (appeng.crafting)
run:192, CraftingJob (appeng.crafting)
call:511, Executors$RunnableAdapter (java.util.concurrent)
run:266, FutureTask (java.util.concurrent)
runWorker:1149, ThreadPoolExecutor (java.util.concurrent)
run:624, ThreadPoolExecutor$Worker (java.util.concurrent)
run:748, Thread (java.lang)

Call stack of the SERVER main thread at the same time (probably why it deadlocked...):

wait:-1, Object (java.lang)
wait:502, Object (java.lang)
simulateFor:293, CraftingJob (appeng.crafting)
onWorldTick:208, TickHandler (appeng.hooks)
accept:-1, 1339494867 (appeng.hooks.TickHandler$$Lambda$3871)
doCastFilter:247, EventBus (net.minecraftforge.eventbus)
lambda$addListener$11:239, EventBus (net.minecraftforge.eventbus)
invoke:-1, 1196855655 (net.minecraftforge.eventbus.EventBus$$Lambda$2946)
post:297, EventBus (net.minecraftforge.eventbus)
onPostWorldTick:100, BasicEventHooks (net.minecraftforge.fml.hooks)
updateTimeLightAndEntities:890, MinecraftServer (net.minecraft.server)
tick:820, MinecraftServer (net.minecraft.server)
tick:84, IntegratedServer (net.minecraft.server.integrated)
func_240802_v_:663, MinecraftServer (net.minecraft.server)
lambda$startServer$0:233, MinecraftServer (net.minecraft.server)
run:-1, 1922600460 (net.minecraft.server.MinecraftServer$$Lambda$6821)
run:748, Thread (java.lang)

(On a peripherally related topic: any chance that an API could be added to get a Crafting Status view? i.e. all jobs currently in progress with options to cancel them even if you weren't the one starting it. Unless I'm missing something, that doesn't seem to be there right now.)

commented

synchronised here means the java synchronized, so it will deadlock. I guess we should extend the documentation about it being called from a different thread and that any consumer has to take care themselves in case their handling relies on it being called from a specific thread.

commented

So "synchronized with the world you passed" means "it is not safe to do anything in this world" rather than that it is? That seems like an odd way to phrase it.

It also seems like an odd choice when the only point of the callback is to call submitJob, which isn't safe to call...

commented

I would recommend reading about java synchronization. All it prevents here is that the crafting calculation is called from multiple threads. E.g. would mojang or forge decide to run every dimension in their own thread every dimension thread would have to wait until the current one is finished. But otherwise there are no further safe guard. E.g. in case another thread did acquire a lock the crafting cpu, the crafting thread can no longer acquire it, but will wait until it can. While the other thread won't release it until it could acquire the one for the crafting first. So a classic dead lock.

What you did expect would be declared as explicitly thread safe. Which it is not and should never be within minecraft implicitly.