Fix CME from Accessing World in onLoad

[serenibyss]

What:
This PR simply moves the check for redstone power out of MetaTileEntity#onLoad into MetaTileEntity#update with a check ensuring it only runs on first tick. As it was before, it caused a ConcurrentModificationException in some cases, detailed in #1256, caused by a known bug in Forge: MinecraftForge/MinecraftForge#2365.

How solved:
I moved the redstone check from onLoad to update with a check for first tick.

Outcome:

• Fix rare CME crash on world load caused by a call to getRedstonePower
• Closes [BUG] ConcurrentModificationException causing server crash #1256

Possible compatibility issue:
None expected

[hjae78]

I think there is a similar reference to getRedstonePower somewhere in the pipe or cable code that I saw when I was looking at uses, probably best to move that too

[hjae78]

Here

[hjae78]

This is a bug that is extremely difficult (maybe even close to impossible) to replicate but I experienced it in my world. Doing this change resolved it and I witnessed no functional change.

[warjort]

It seems to me that there is a backwards compatibility issue where a MetaTileEntity could do

@Override
public void onLoad() {
    super.onLoad();
    // Do processing based on the redstone cache being already initialised
}

This possibility could be avoided if the patch was instead implemented in MetaTileEntityHolder
by adding first tick handling that just calls MetaTileEntity.onLoad()
and then removing the current onLoad() invocation that causes problems:

diff --git a/src/main/java/gregtech/api/metatileentity/MetaTileEntityHolder.java b/src/main/java/gregtech/api/metatileentity/MetaTileEntityHolder.java
index 25966e25..4a24b1e4 100644
--- a/src/main/java/gregtech/api/metatileentity/MetaTileEntityHolder.java
+++ b/src/main/java/gregtech/api/metatileentity/MetaTileEntityHolder.java
@@ -153,6 +153,9 @@ public class MetaTileEntityHolder extends TickableTileEntityBase implements IUIH
     @Override
     public void update() {
         if (metaTileEntity != null) {
+            if (getTimer() == 0) {
+                metaTileEntity.onLoad();
+            }
             metaTileEntity.update();
         }
         if (this.needToUpdateLightning) {
@@ -212,14 +215,6 @@ public class MetaTileEntityHolder extends TickableTileEntityBase implements IUIH
         markDirty();
     }

-    @Override
-    public void onLoad() {
-        super.onLoad();
-        if (metaTileEntity != null) {
-            metaTileEntity.onLoad();
-        }
-    }
-
     @Override
     public void onChunkUnload() {
         super.onChunkUnload();

[warjort]

Doing this for the pipes looks to be more complicated.

The pipes have 2 different states, ticking and not ticking.
A pipe is ticking if it has (or used to have) a cover, or for fluid pipes if it has an active connection to a tank.

The redstone power cache is however maintained for both types.

Without a first tick for non ticking pipes its not so simple to move the onLoad()

One possible implementation is to make it lazily initialised, something like:

--- a/src/main/java/gregtech/api/pipenet/tile/PipeCoverableImplementation.java
+++ b/src/main/java/gregtech/api/pipenet/tile/PipeCoverableImplementation.java
@@ -30,6 +30,7 @@ public class PipeCoverableImplementation implements ICoverable {
     private final IPipeTile<?, ?> holder;
     private final CoverBehavior[] coverBehaviors = new CoverBehavior[6];
     private int[] sidedRedstoneInput = new int[6];
+    private boolean redstoneInitialised = false;

     public PipeCoverableImplementation(IPipeTile<?, ?> holder) {
         this.holder = holder;
@@ -121,6 +122,7 @@ public class PipeCoverableImplementation implements ICoverable {
         for (EnumFacing side : EnumFacing.VALUES) {
             this.sidedRedstoneInput[side.getIndex()] = GTUtility.getRedstonePower(getWorld(), getPos(), side);
         }
+        this.redstoneInitialised = true;
     }

     @Override
@@ -128,10 +130,14 @@ public class PipeCoverableImplementation implements ICoverable {
         if(!ignoreCover && getCoverAtSide(side) != null) {
             return 0; //covers block input redstone signal for machine
         }
+        if (!redstoneInitialised) {
+            onLoad();
+        }
         return sidedRedstoneInput[side.getIndex()];
     }

     public void updateInputRedstoneSignals() {
+        // What to do here?
         for (EnumFacing side : EnumFacing.VALUES) {
             int redstoneValue = GTUtility.getRedstonePower(getWorld(), getPos(), side);
             int currentValue = sidedRedstoneInput[side.getIndex()];
@@ -143,6 +149,7 @@ public class PipeCoverableImplementation implements ICoverable {
                 }
             }
         }
+        this.redstoneInitialised = true;
     }

     @Override
diff --git a/src/main/java/gregtech/api/pipenet/tile/TileEntityPipeBase.java b/src/main/java/gregtech/api/pipenet/tile/TileEntityPipeBase.java
index bf125b60..8264d069 100644
--- a/src/main/java/gregtech/api/pipenet/tile/TileEntityPipeBase.java
+++ b/src/main/java/gregtech/api/pipenet/tile/TileEntityPipeBase.java
@@ -280,12 +280,6 @@ public abstract class TileEntityPipeBase<PipeType extends Enum<PipeType> & IPipe
         this.coverableImplementation.readFromNBT(compound);
     }

-    @Override
-    public void onLoad() {
-        super.onLoad();
-        this.coverableImplementation.onLoad();
-    }
-
     protected void writePipeProperties(PacketBuffer buf) {
         buf.writeVarInt(pipeType.ordinal());
     }

But notice the "What to do here?" question in the code. This code is invoked on a block update next to the pipe.
If we lazy init here it would miss a redstone signal change for the first block update.

But it could also miss an update if we don't.
e.g. on chunk load the signal is 15, but a block update changes it to 0
since we didn't init the data it would see it as no change.

Maybe it should always fire the change if redstoneInitialised == false?

[warjort]

AFAICT, the only usecase for the redstone signal in base GTCE is the machine controller
which does not depend on the redstone signal change as an event, but instead just needs to know the value vs a threshold.
So firing for redstoneInitialised == false would not change its behaviour, it would just be extra work on the first block update.

[warjort]

My example code above for the pipe lazy initialsation has a "recursion" problem.
For the machine controller it will callback to get the signal strength which would lead to the lazy initialisation of all the cached values.
So a second cover on a pipe might not see a changed value.