TileEntity Invalidation?
gabizou opened this issue ยท 4 comments
A bit of backstory, trust me, it's needed
In investigating an issue that we've exposed within Sponge, we might've unveiled an unintended side effect of creating BlockChange events for all block changes intrinsically (since Forge proper doesn't do that by default, this issue is somewhat masked). To give a proper explanation of what Sponge does for block changes:
- Capture all block changes as BlockSnapshots to allow "Rolling back" if the BlockChange event is cancelled
- Proxy on demand the surrounding
IBlockStates andTileEntityinstances as necessary (if multiple blocks are changing during a tick, like a piston, this proxies the appropriate blocks to continue processing at the correct "situation") - Throw the event
- IF the event isn't cancelled, and no mod/plugin has changed the output, go back and play back the changes (calling block.onPlace, tileEntity.validate() etc. while pushing the proxy to expose the surrounding blocks that were expected at the locations that already are changing)
The issue with this is that when the BlockSnapshot is told to be created, Forge proper calls to the TileEntity to write to NBT, and here in lies our first issue.
Because the BlockSnapshot is created before TileEntity.validate(), SynchedTileEntity will throw an exception
A user submitted this issue where Sponge's block proxy is out of sync due to an exception raised earlier in the game, coming from a packet being processed, specifically a place block packet. The original exception that started this is somewhat verbose because Sponge does a bit of exposure as to "what's going on" when these exceptions are raised:
/****************************************************************************************************************************************************************************************/
> /****************************************************************************************************************************************************************************************/
/* Exception Exiting Phase */
/****************************************************************************************************************************************************************************************/
/* Something happened when trying to unwind Packet{PlaceBlock} */
/* PhaseContext : */
/* - Owner: EntityPlayerMP['Lain__'/354, l='world', x=224.75, y=72.00, z=157.35] */
/* - Source: EntityPlayerMP['Lain__'/354, l='world', x=224.75, y=72.00, z=157.35] */
/* - CapturedBlocks: MultiBlockCaptureSupplier{Captured=0, Head=ChangeBlock{}} */
/* - PacketPlayer: EntityPlayerMP['Lain__'/354, l='world', x=224.75, y=72.00, z=157.35] */
/* - Packet: CPacketPlayerTryUseItemOnBlock{position=BlockPos{x=224, y=71, z=155}, placedBlockDirection=up, hand=MAIN_HAND, facingX=0.5188592, facingY=1.0, facingZ=0.43686384} */
/* - IgnoreCreative: false */
/* - ItemStackUsed: 1xtile.openblocks.trophy@5 */
/* - OpenContainer: null */
/* Phases remaining : */
/* - Phase: General{Unwinding} */
/* Context: */
/* - Source: EntityPlayerMP['Lain__'/354, l='world', x=224.75, y=72.00, z=157.35] */
/* - UnwindingState: Packet{PlaceBlock} */
/* - UnwindingContext: BasicPacketContext{isCompleted=true} */
/* - IsPostingSpecial: false */
/* - Owner: EntityPlayerMP['Lain__'/354, l='world', x=224.75, y=72.00, z=157.35] */
/* - Source: EntityPlayerMP['Lain__'/354, l='world', x=224.75, y=72.00, z=157.35] */
/* - CapturedBlocks: MultiBlockCaptureSupplier{Captured=0, Head=ChangeBlock{}} */
/* - PacketPlayer: EntityPlayerMP['Lain__'/354, l='world', x=224.75, y=72.00, z=157.35] */
/* - Packet: CPacketPlayerTryUseItemOnBlock{position=BlockPos{x=224, y=71, z=155}, placedBlockDirection=up, hand=MAIN_HAND, facingX=0.5188592, facingY=1.0, facingZ=0.43686384} */
/* - IgnoreCreative: false */
/* - ItemStackUsed: 1xtile.openblocks.trophy@5 */
/* - OpenContainer: null */
/* - Phase: Packet{PlaceBlock} */
/* Context: */
/* - Owner: EntityPlayerMP['Lain__'/354, l='world', x=224.75, y=72.00, z=157.35] */
/* - Source: EntityPlayerMP['Lain__'/354, l='world', x=224.75, y=72.00, z=157.35] */
/* - CapturedBlocks: MultiBlockCaptureSupplier{Captured=0, Head=ChangeBlock{}} */
/* - PacketPlayer: EntityPlayerMP['Lain__'/354, l='world', x=224.75, y=72.00, z=157.35] */
/* - Packet: CPacketPlayerTryUseItemOnBlock{position=BlockPos{x=224, y=71, z=155}, placedBlockDirection=up, hand=MAIN_HAND, facingX=0.5188592, facingY=1.0, facingZ=0.43686384} */
/* - IgnoreCreative: false */
/* - ItemStackUsed: 1xtile.openblocks.trophy@5 */
/* - OpenContainer: null */
/* - Phase: Plugin{ScheduledTask} */
/* Context: */
/* - Source: com.google.common.util.concurrent.ListenableFutureTask@76b9b077 */
/* Stacktrace: */
/* java.lang.IllegalStateException: Tile entity not initialized properly */
/* com.google.common.base.Preconditions.checkState(Preconditions.java:444) */
/* openmods.tileentity.SyncedTileEntity.getSyncMap(SyncedTileEntity.java:104) */
/* openmods.tileentity.SyncedTileEntity.writeToNBT(SyncedTileEntity.java:113) */
/* openblocks.common.tileentity.TileEntityTrophy.writeToNBT(TileEntityTrophy.java:81) */
/* net.minecraftforge.common.util.BlockSnapshot.getTileNBT(BlockSnapshot.java:132) */
/* net.minecraftforge.common.util.BlockSnapshot.<init>(BlockSnapshot.java:61) */
/* org.spongepowered.mod.event.SpongeToForgeEventFactory.createAndPostBlockPlaceEvent(SpongeToForgeEventFactory.java:755) */
/* org.spongepowered.mod.event.SpongeToForgeEventFactory.createAndPostForgeEvent(SpongeToForgeEventFactory.java:247) */
/* org.spongepowered.mod.event.SpongeModEventManager.post(SpongeModEventManager.java:337) */
/* org.spongepowered.mod.event.SpongeModEventManager.extendedPost(SpongeModEventManager.java:446) */
/* org.spongepowered.mod.event.SpongeModEventManager.post(SpongeModEventManager.java:415) */
/* org.spongepowered.common.event.SpongeEventManager.post(SpongeEventManager.java:465) */
/* org.spongepowered.common.SpongeImpl.postEvent(SpongeImpl.java:253) */
/* org.spongepowered.common.event.tracking.TrackingUtil.iterateChangeBlockEvents(TrackingUtil.java:564) */
/* org.spongepowered.common.event.tracking.TrackingUtil.processBlockCaptures(TrackingUtil.java:435) */
/* org.spongepowered.common.event.tracking.TrackingUtil.processBlockCaptures(TrackingUtil.java:389) */
/* org.spongepowered.common.event.tracking.phase.packet.player.PlaceBlockPacketState.unwind(PlaceBlockPacketState.java:134) */
/* org.spongepowered.common.event.tracking.phase.packet.player.PlaceBlockPacketState.unwind(PlaceBlockPacketState.java:67) */
/* org.spongepowered.common.event.tracking.PhaseTracker.completePhase(PhaseTracker.java:316) */
/* org.spongepowered.common.event.tracking.PhaseContext.close(PhaseContext.java:618) */
/* org.spongepowered.common.event.tracking.phase.packet.PacketPhaseUtil.onProcessPacket(PacketPhaseUtil.java:211) */
/* net.minecraft.network.PacketThreadUtil$1.redirect$impl$redirectToPhaseTracker$zla000(PacketThreadUtil.java:540) */
/* net.minecraft.network.PacketThreadUtil$1.run(PacketThreadUtil.java:21) */
/* java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:511) */
/* java.util.concurrent.FutureTask.run$$$capture(FutureTask.java:266) */
/* java.util.concurrent.FutureTask.run(FutureTask.java) */
/* net.minecraft.util.Util.runTask(Util.java:53) */
/* org.spongepowered.common.SpongeImplHooks.onUtilRunTask(SpongeImplHooks.java:308) */
/* net.minecraft.server.MinecraftServer.redirect$onRun$zje000(MinecraftServer.java:4930) */
/* net.minecraft.server.MinecraftServer.updateTimeLightAndEntities(MinecraftServer.java:798) */
/* net.minecraft.server.dedicated.DedicatedServer.updateTimeLightAndEntities(DedicatedServer.java:415) */
/* net.minecraft.server.MinecraftServer.tick(MinecraftServer.java:743) */
/* net.minecraft.server.MinecraftServer.run(MinecraftServer.java:592) */
/* java.lang.Thread.run(Thread.java:748) */
/* */
/* Minecraft : 1.12.2 */
/* Minecraft Forge : 14.23.5.2838 */
/****************************************************************************************************************************************************************************************/
And as you can see, the stack leads up to the point where we're calling to create a BlockSnapshot, which will ask for the Trophy TileEntity to writeToNbt. But here in lies the first problem: Because Sponge doesn't call validate() until after the block event is thrown (basically when the block event is passed, we process the actual changes and call the necessary callbacks), your [SyncedTileEntity will check if it's map has been created yet])(
Problems with a workaround
Since I know it's easier for active devs to write a workaround for something already complex, I have accepted that Sponge may just call TileEntity.validate() when proxying before throwing these events, but an issue does arise in that your classes don't override invalidate() to un-listen to changes and empty the map, so if the event was cancelled, Sponge wouldn't be able to tell your TileEntity to invalidate any connections to abstract systems you've developed.
Thank you for detailed report, I saw that crash couple times and wasn't sure where was that coming from.
I guess I can make sync map initialization lazy, though I still need non-null TileEntity.world to perform it. Seems to be the case in pure Forge. Do you know if that also holds in Sponge?
Decided to go with FMLCommonHandler.instance().getEffectiveSide(), let's see how this one breaks.
(also, call to .invalidate in this case would not be needed, all state changes from validate are limited to TileEntity).
I guess I can make sync map initialization lazy, though I still need non-null
TileEntity.worldto perform it. Seems to be the case in pure Forge. Do you know if that also holds in Sponge?
It'll still be true in Sponge, we don't interrupt the TileEntity creation process, just the application (adding) to the world.
(also, call to
.invalidatein this case would not be needed, all state changes fromvalidateare limited toTileEntity).
I was thinking it'd be needed if we basically had to "undo" the tile entity being added, in the case of our workaround basically calling validate() before we throw events, and then invalidate() if the event is cancelled. But then again, I don't know how much the synched maps are being tied into networking or listeners otherwise.