Calling Chunk#getTileEntity(BlockPos) in ChunkEvent.Load hangs world gen
SuperMartijn642 opened this issue · 10 comments
Versions:
- Minecraft 1.16.4
- Forge 35.1.37
- Quark r2.4-296
- AutoRegLib 1.6-47
Issue:
With Quark and AutoRegLib installed, calling Chunk#getTileEntity(BlockPos)
in ChunkEvent.Load
prevents new worlds from generating. This does not happen without Quark installed.
Reproduce:
Just subscribe the following method to the event bus.
public static void onChunkLoad(ChunkEvent.Load e){
IChunk chunk = e.getChunk();
for(BlockPos pos : chunk.getTileEntitiesPos())
chunk.getTileEntity(pos);
}
When creating a new world it loads to 100% and then just stalls without a crashing.
Example:
I made an example mod to demonstrate the issue.
https://github.com/SuperMartijn642/QuarkIssueTestMod/blob/master/src/main/java/com/supermartijn642/quarkissue/QuarkIssue.java
Seems to be related to MinecraftForge/MinecraftForge#7519 (comment).
Has a temporary fix by the OP there till fix is refined a PR is created and merged I think.
Tested the fix with quark and the issue test mod and it works.
Versions:
AutoRegLib-1.6-47
f7519-1.2
quarkissue-1.0.0-mc1.16
Quark-r2.4-297
Had a little bit of time to diagnose this.
I would say this issue isn't on Quark it's the fact that calling getTileEntity
during ChunkLoad
is not safe.
Chunk#getTileEntity
will change a deferred
TileEntity at into a real TileEntity if it matches that position.
This undeferrement has an edge case that it's unsafe and will deadlock the thread if Chunk#loaded
is true and the ChunkStatus
of the Chunk is not FULL
(Same underlying cause as MinecraftForge/MinecraftForge#7519).
This is what your running into here.
ChunkLoad
is currently being fired after the chunk is marked loaded
but before the ChunkStatus
is marked FULL
.
Why do Vanilla TileEntities not have this problem? It's because vanilla calls getTileEntity
during generation to set additional data, this causes the undefferment
to occur when the Chunk hasn't been marked as loaded
as such prevents the deadlock.
My PR MinecraftForge/MinecraftForge#7532 and Test Mod (It's there so you can test out the proposed changes without having to build a custom version of forge) includes a change that causes ChunkLoad
to be fired at a stage which it's available via FULL
which fixes this.
Quark could call getTileEntity
for MonsterBox
[1]'s during world generation just like vanilla, however whilst this will fix the issue Quark side your assumption that calling getTileEntity
is safe is wrong and will break with other mods.
Sorry if this is comes across slightly ranty but I'll just mention how much I hate the absolute mess that is the newish chunk loading/generation system in vanilla.
[1] This may occur in other places but after a few tests this was the TileEntity that showed up during deadlocks.
Here have a stacktrace of the actual deadlock:
Server thread@17147" prio=5 tid=0x41 nid=NA waiting
java.lang.Thread.State: WAITING
at sun.misc.Unsafe.park(Unsafe.java:-1)
at java.util.concurrent.locks.LockSupport.parkNanos(LockSupport.java:215)
at net.minecraft.util.concurrent.ThreadTaskExecutor.threadYieldPark(ThreadTaskExecutor.java:134)
at net.minecraft.util.concurrent.ThreadTaskExecutor.driveUntil(ThreadTaskExecutor.java:123)
at net.minecraft.world.server.ServerChunkProvider.getChunk(ServerChunkProvider.java:130)
at net.minecraft.world.World.getChunk(World.java:185)
at net.minecraft.world.IWorldReader.getChunk(IWorldReader.java:116)
at net.minecraft.world.World.getChunk(World.java:181)
at net.minecraft.world.World.getChunkAt(World.java:177)
at net.minecraft.world.World.setTileEntity(World.java:689)
at net.minecraft.world.chunk.Chunk.addTileEntity(Chunk.java:412)
at net.minecraft.world.chunk.Chunk.setDeferredTileEntity(Chunk.java:766)
at net.minecraft.world.chunk.Chunk.getTileEntity(Chunk.java:392)
at net.minecraft.world.chunk.Chunk.getTileEntity(Chunk.java:379)
at com.supermartijn642.quarkissue.QuarkIssue$Events.onChunkLoad(QuarkIssue.java:46)
at net.minecraftforge.eventbus.ASMEventHandler_10_Events_onChunkLoad_Load.invoke(.dynamic:-1)
at net.minecraftforge.eventbus.ASMEventHandler.invoke(ASMEventHandler.java:85)
at net.minecraftforge.eventbus.EventBus$$Lambda$2600.985900994.invoke(Unknown Source:-1)
at net.minecraftforge.eventbus.EventBus.post(EventBus.java:302)
at net.minecraftforge.eventbus.EventBus.post(EventBus.java:283)
at net.minecraft.world.server.ChunkManager.lambda$null$25(ChunkManager.java:622)
at net.minecraft.world.server.ChunkManager$$Lambda$5929.1925885999.apply(Unknown Source:-1)
at com.mojang.datafixers.util.Either.lambda$mapLeft$0(Either.java:162)
at com.mojang.datafixers.util.Either$$Lambda$5930.2099888761.apply(Unknown Source:-1)
at com.mojang.datafixers.util.Either$Left.map(Either.java:38)
at com.mojang.datafixers.util.Either.mapLeft(Either.java:162)
at net.minecraft.world.server.ChunkManager.lambda$func_219200_b$26(ChunkManager.java:586)
at net.minecraft.world.server.ChunkManager$$Lambda$5926.1444986033.apply(Unknown Source:-1)
at java.util.concurrent.CompletableFuture.uniApply(CompletableFuture.java:602)
at java.util.concurrent.CompletableFuture$UniApply.tryFire(CompletableFuture.java:577)
at java.util.concurrent.CompletableFuture$Completion.run(CompletableFuture.java:442)
at net.minecraft.world.chunk.ChunkTaskPriorityQueueSorter.lambda$null$1(ChunkTaskPriorityQueueSorter.java:44)
at net.minecraft.world.chunk.ChunkTaskPriorityQueueSorter$$Lambda$5852.280607060.run(Unknown Source:-1)
at net.minecraft.util.concurrent.ThreadTaskExecutor.run(ThreadTaskExecutor.java:139)
at net.minecraft.world.server.ServerChunkProvider$ChunkExecutor.run(ServerChunkProvider.java:512)
at net.minecraft.util.concurrent.ThreadTaskExecutor.driveOne(ThreadTaskExecutor.java:109)
at net.minecraft.world.server.ServerChunkProvider$ChunkExecutor.driveOne(ServerChunkProvider.java:520)
at net.minecraft.world.server.ServerChunkProvider.driveOneTask(ServerChunkProvider.java:270)
at net.minecraft.server.MinecraftServer.driveOneInternal(MinecraftServer.java:746)
at net.minecraft.server.MinecraftServer.driveOne(MinecraftServer.java:735)
at net.minecraft.util.concurrent.ThreadTaskExecutor.driveUntil(ThreadTaskExecutor.java:122)
at net.minecraft.server.MinecraftServer.runScheduledTasks(MinecraftServer.java:721)
at net.minecraft.server.MinecraftServer.loadInitialChunks(MinecraftServer.java:479)
at net.minecraft.server.MinecraftServer.func_240800_l__(MinecraftServer.java:317)
at net.minecraft.server.integrated.IntegratedServer.init(IntegratedServer.java:63)
at net.minecraft.server.MinecraftServer.func_240802_v_(MinecraftServer.java:642)
at net.minecraft.server.MinecraftServer.lambda$startServer$0(MinecraftServer.java:233)
at net.minecraft.server.MinecraftServer$$Lambda$5733.610680069.run(Unknown Source:-1)
at java.lang.Thread.run(Thread.java:748)
Found in testing a while back, as above, because the event is called during the future completion, so calling getTileEntity
results in calling getChunk
waiting on that future to complete which never ends up happening because it is currently is in the process of being completed.
Gave up trying to fix it though but great that you didn't! :3
@AterAnimAvis from your comment, I conclude that this issue doesn't require changes from Quark and that getTileEntity
just shouldn't be called during the ChunkEvent.Load
.
Did I understand that correctly?
If so, I guess this issue can be closed and hopefully the proposed changes can be merged into Forge.
Pretty much, ChunkLoad is worthless for world interaction until the PR is merged unless you schedule the execution later by a tick to allow the future to complete, this is practically what my PR does which is why it requires testing to make sure it doesn’t break mods that don’t expect this (This may not be actually pulled until the next breaking changes period).
Unfortunately from my knowledge at least there’s no other easy fix however Im only partially familiar with the mess that is chunk loading so an alternative fix may be possible.
Quark could fix this instance of the issue by ensure the TileEntity gets undeferred during worldgen, but it won’t solve the root cause.
As to whether it’s worth it anyway. 🤷♂️ It’s a one line fix for compatibility so maybe.
Perhaps the event could be raised after the future is completed, but not in the next tick?
Sorry I wasn’t really clear there.
The PR does exactly that, delays the relevant block of code until the future for Chunk.FULL is complete (This effects TileEntities and EntityJoinWorld as well).
Further discussion of this would probably be better on the PR/Issue in the MinecraftForge repo seeing as we’ve strayed from the realm of Quark, I’m also available in Forgecord (AterAnimAvis#4372) if you would prefer discord.
If your talking about MinecraftForge/MinecraftForge#7532 please note it pivoted to only fixed the issue in the forge handler, Lex preferred that option due to potential side effects.
Theres an open PR MinecraftForge/MinecraftForge#7697 that looks to fix this in a different way (Funnily enough the original fix I prototyped before switching to the option I did), however that still has issues, and currently would cause CMEs.