Minecraftforge: Tile entities stop working after chunk reload

[viveleroi]

Using forge 1.12.1-14.22.0.2469 without mods, tile entities stop working (meaning I can open their UI and interact with it, but updates have stopped) after a chunk has loaded, after having been previously unloaded. The only solution is to break and re-place the malfunctioning block (which causes other issues for blocks like the smeltery) or a server reboot.

Steps to Reproduce

• Place a furnace in a chunk that can be unloaded (far from spawn, since I believe spawn chunks are kept loaded).
• Travel far away and allow the furnace chunk to unload. (Or disconnect if running a server, but do not reboot).
• Return to that chunk.
• Place coal and cobble in the furnace. It won't cook.

This affects every tile entity, here's a list of those I've personally seen affected and how:

• vanilla furnaces (I can put in fuel and an item like coal/cobble, but it never ignites or starts cooking)
• Tinker's smeltery ("pouring" actions can be started but they pour infinitely. Nothing smelts.)
• Forestry farm never acts on grown plants.
• Forestry apiaries never "start" queens after placing princess/drones
• Immersive Engineering water wheel/windmill (or the dynamo) stop producing electricity.
• IE capacitors stop outputting electricity they have stored.
• Botania end-o-flame flowers stop igniting when a fuel source is dropped.

These blocks are all affected simultaneously so it's clear to me that they're all affected by the same root cause, whatever that may be.

On the forums, darkphan posted that "it seems to have started happening after build 2450 of Forge"

[williewillus]

https://github.com/MinecraftForge/MinecraftForge/pull/4281 seems the likely culprit

[bs2609]

I'm not sure if this is responsible for the issue, but #4281 does seem to be susceptible to the situation I described in #3914.

[williewillus]

Maybe try rebasing #3940 on latest 1.12 and see if it happens?
EDIT: Oh that's #4162

[liach]

Actually, in pure forge or vanilla, the world seems to be able to load a block entity without its corresponding chunk loaded, according to the source code of the method which adds block entities to a world.

[bs2609]

4162 won't help here, as that only applies to the dormant chunk cache. This would need it's own fix.

A possible approach is to add some state to removeTileEntitiesForRemovedChunks() so that the removeIf() predicates only remove a tile entity for a given block position once (the first one).

[liach]

@bs2609 Or we can revert mezz's pull and add all tes in tileEntitiesToBeRemoved into a set when we use removeAll, for the efficiency of ArrayList#removeAll depends on the efficiency of contains(Object o) in the collection in the argument.

[liach]

Original:

            for (Object tile : tileEntitiesToBeRemoved)
            {
               ((TileEntity)tile).onChunkUnload();
            }

            this.tickableTileEntities.removeAll(this.tileEntitiesToBeRemoved);
            this.loadedTileEntityList.removeAll(this.tileEntitiesToBeRemoved);
            this.tileEntitiesToBeRemoved.clear();

We may do:

            for (Object tile : tileEntitiesToBeRemoved)
            {
               ((TileEntity)tile).onChunkUnload();
            }

            java.util.Set<TileEntity> toRemove = com.google.common.collect.Sets.newHashSet(this.tileEntitiesToBeRemoved);
            this.tickableTileEntities.removeAll(toRemove);
            this.loadedTileEntityList.removeAll(toRemove);
            this.tileEntitiesToBeRemoved.clear();

This is also backward compatible, possible for people who want to port it to prior versions that use Java 6 (Collection#removeIf is a default method added in Java 8)

[mezz]

I will get started on that.
Can someone confirm that this is not an issue on Forge 2449?

[bs2609]

That's the impression I got from here: http://www.minecraftforge.net/forum/topic/60621-tile-entities-randomly-stop-working-1121/

[liach]

for the efficiency of ArrayList#removeAll depends on the efficiency of contains(Object o) in the collection in the argument.

Here are Java sources that prove it http://hg.openjdk.java.net/jdk6/jdk6/jdk/file/3d2f9f6b0f09/src/share/classes/java/util/ArrayList.java#l604
http://hg.openjdk.java.net/jdk8/jdk8/jdk/file/tip/src/share/classes/java/util/ArrayList.java#l673

In both 6 and 8, the code is pretty much the same.

[liach]

@mezz Rebase the fix for load order before you apply this "improvement" (needs to see benchmark result too)

[mezz]

I'm not able to reproduce this issue in dev in single player or on multiplayer server.

Placed a furnace at (10000, 10000) (far away from spawn) and filled it with wood and coal to start it working.
Then went through a portal and came back, teleported away and back, flew away and came back, logged off and back on.
None of those showed the issue described.

Can anyone else reproduce this and explain the steps to take?

[liach]

You might need to wait for like a minute for the chunk to fully unload per my experience in 1.10.2. If the time is too short, the chunk might not be unloaded yet when you come back.

[mezz]

I confirmed that it unloaded each time, using a debug log.

[PhiPro95]

@liach Is it safe to use HashSet on TEs? #3872 had a discussion about this and there wasn't really a definite answer.

[bs2609]

I think this is it:

Using furnaces placed around a village, flying away and back. Based on trying to reproduce #3897, which suggests it might be the same thing.

[mezz]

So this is only reproducible with the chunk cache enabled?

[bs2609]

No - it's the chunk unload/load behaviour I described in #3914 that causes this I think.

If there are both 'old' and 'new' versions of a TE in the world lists, the code in #4281 will remove them both, as they both have positions in a chunk that had been removed.

[PhiPro95]

What about adding a field to TileEntity that indicates whether this TE should be removed in the next update cycle and then check for this flag? Chunk unloading can then just flag all its TEs. TEs that are added afterwards won't be flagged, so this should solve the issue and also be even faster than the current solution.

[mezz]

@bs2609 I looked through that and I agree it is the most likely cause.

Tested @liach's solution here and it is still very fast.
@PhiPro95 I think the TE flag solution is optimally fast, but somewhat more invasive.

I will revert the existing one and implement liach's.

[liach]

If a plain hash set is bad, guess we can use an identity hash set, for we don't need iterations.

[mezz]

Yeah I am using identity hash set just to be safe.

[bs2609]

This'll probably still be broken with the dormant chunk cache then, so can I get another look at #4162?

[mezz]

Fixed in Forge 14.22.0.2475

[viveleroi]

Excellent, thanks guys and @mezz for the quick fix. I will test this today!

[darkphan]

This does appear to fix the issue. Thanks guys!

[bs2609]

@mezz, as #4281 appears to have adopted by other projects, it might be a good idea to inform them of this isssue?

[mezz]

I have already notified the sponge devs. If you know of other projects using it let me know.

[bs2609]

https://github.com/PaperMC/Paper/commit/0181286eb9cdda705cdd45719f58af3833c1827d

[zachbr]

We just got a report about this today. Should be handled by the end of the night. Thanks.