Wires cause infinite loop when closing world

[supersimple33]

Mod Loader

Fabric

Version Information

ac33404a

Reconfirmed in 13285bb

Log or Crash Report

[TIME][Server thread/WARN] (Galacticraft) Tried to remove wire from removed network!

Reproduction steps

When using certain wire configurations (typically just a bunch of wires like sometimes a 5x5 square of wires will cause the loop but not every time) the game can end up in an infinite loop where it repeats the error message Tried to remove wire from removed network! in the console forever.

[supersimple33]

I'm having some trouble tracking this one down but it seems that in the setRemoved override of WireBlockEntitys sometime between calling the super where it set the entity isRemoved()=true and getting new entities via this.world.getBlockEntity(pos) in the WireNetworkImpl the entities are forgetting that they are removed.

[benreid24]

I spent some time digging into this and believe that I found the root cause.

I was able to reproduce the issue with as few as 2 wires. The same issue is also present in the pipe network code. The issue does not occur if no wires or pipes are placed before the world is closed, even if wires/pipes are present in the loaded world. I was unable to reproduce the issue during chunk unloading by moving far away, it only seems to occur when chunks are unloaded to be saved on world close.

Root Cause

On world shutdown the following occurs:

1. Chunk is marked as unloaded
2. Chunk calls setRemoved on all contained block entities
3. WireBlockEntity calls into its network to remove itself
4. WireNetworkImpl removes the wire. Networks of size 1 terminate here
5. Networks with multiple wires query adjacent positions from the level to find neighbor wires
6. Level loads the chunk at the requested position because the current chunk is marked as unloaded
7. New WireBlockEntities are created at the same positions in the newly loaded chunk object
8. WireNetworkImpl determines new network groups from adjacent wires but is now getting the new entities and not the original
9. If the network is not split we terminate here. Errors will be logged, but there is no infinite loop
10. New WireNetworkImpl objects are created for each new wire grouping. The newly created wires get networks assigned to them
11. Log error source: The original WireBlockEntity objects do not get the new parent, so they call into the original (now removed) WireNetworkImpl and trigger the error
12. Infinite loop source: The newly create chunk(s) eventually get unloaded, repeating the process with the new networks and wires

Adding a check along the lines of if (adjacentWire.getNetwork() == null) continue to the network removal code prevents the new wire entities from getting networks created/assigned and prevents the infinite loop from occuring.

Why it doesn't happen every time

World saved without placing wires

From what I can tell, when the world is loaded and there are wires it does not create wire networks. This seems like a bug in and of itself, although it's possible that machines/power sources lazily create the networks as needed (I didn't look at this). When the wires in the world do not have networks they do not call into networks to be removed, which preempts the above process.

Chunk unloading not at exit

My best guess for this not causing the issue is that the cleanup order of the chunk and its contained objects differs in this scenario as opposed to world exit. I tried my best to dig through the decompiled stacktraces as chunks were unloaded, but have been unable to confirm this theory. That said, if it isn't broken don't fix it.

Solutions

Query world blocks without loading chunks

I am not sure if this is possible, and it honestly seems like it would cause more problems than solutions, but if chunks are not loaded when queried by the network, then we would not get the infinite loop. We would only want to utilize this method when recomputing the network after block removal, in all other use cases we need to load chunks in order to be fully correct.

Keep Wire references in WireNetworkImpl and query from local state

Essentially the first solution, but instead of finding some way to query Level without creating chunks we change WireNetworkImpl.wires from List<BlockPos to Map<BlockPos, Wire> and query that when remapping networks due to removed wires. WireNetworkImpl.addWire would also need to be updated to only use the local state when removing wires, and still use the Level.getBlockEntity otherwise.

If we go this route it would be nice to implement the above functionality in a generic base class that can be used by all types of networks, as this issue effects both power and fluids, and eventually any other network types as well.

Open to suggestions

This is my first contribution and first time actually working on a mod, so I may have overlooked a more correct solution. Any suggestions are welcome.