Quark Oddities

Quark Oddities

22M Downloads

Calling Chunk#getTileEntity(BlockPos) in ChunkEvent.Load hangs world gen

SuperMartijn642 opened this issue · 10 comments

commented

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

commented

:HOW:

commented

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

commented

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)
commented

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

commented

@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.

commented

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.

commented

Perhaps the event could be raised after the future is completed, but not in the next tick?

commented

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.

commented

Should be sorted now that the PR was merged

commented

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.