Repository: MinecraftForge/MinecraftForge (language: java)

Issue:
[1.18.2] Classloading deadlock
**Minecraft Version:** 1.18.2

**Forge Version:** 40.2.2

**Description of issue:**

Under certain circumstances, it appears possible for Forge's networking classes to be loaded by two mods in an order that causes a deadlock. Here is a snippet of a thread dump exhibiting the problem:

```
"modloading-worker-0" #39 daemon prio=5 os_prio=0 cpu=93.75ms elapsed=462.94s tid=0x000001eb0182d230 nid=0xa068 in Object.wait()  [0x00000004a3ffd000]
   java.lang.Thread.State: RUNNABLE
	at net.minecraftforge.network.NetworkInitialization.getHandshakeChannel(forge@40.2.2/NetworkInitialization.java:29)
	- waiting on the Class initialization monitor for net.minecraftforge.network.HandshakeHandler
	at net.minecraftforge.network.NetworkConstants.<clinit>(forge@40.2.2/NetworkConstants.java:42)
	at net.minecraftforge.common.ForgeMod.<init>(forge@40.2.2/ForgeMod.java:150)
	at jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance0(java.base@17.0.1/Native Method)
	at jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance(java.base@17.0.1/NativeConstructorAccessorImpl.java:77)
	at jdk.internal.reflect.DelegatingConstructorAccessorImpl.newInstance(java.base@17.0.1/DelegatingConstructorAccessorImpl.java:45)
	at java.lang.reflect.Constructor.newInstanceWithCaller(java.base@17.0.1/Constructor.java:499)
	at java.lang.reflect.Constructor.newInstance(java.base@17.0.1/Constructor.java:480)
 
   Locked ownable synchronizers:
	- None

"modloading-worker-0" #53 daemon prio=5 os_prio=0 cpu=31.25ms elapsed=462.93s tid=0x000001eb018328d0 nid=0x81fc in Object.wait()  [0x00000004a56fd000]
   java.lang.Thread.State: RUNNABLE
	at net.minecraftforge.network.HandshakeHandler.<clinit>(forge@40.2.2/HandshakeHandler.java:84)
	- waiting on the Class initialization monitor for net.minecraftforge.network.NetworkConstants
	at com.teamabnormals.environmental.core.Environmental.setupLoginMessages(environmental@2.0.0/Environmental.java:144)
	at com.teamabnormals.environmental.core.Environmental.<init>(environmental@2.0.0/Environmental.java:68)
	at jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance0(java.base@17.0.1/Native Method)
	at jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance(java.base@17.0.1/NativeConstructorAccessorImpl.java:77)
	at jdk.internal.reflect.DelegatingConstructorAccessorImpl.newInstance(java.base@17.0.1/DelegatingConstructorAccessorImpl.java:45)
	at java.lang.reflect.Constructor.newInstanceWithCaller(java.base@17.0.1/Constructor.java:499)
	at java.lang.reflect.Constructor.newInstance(java.base@17.0.1/Constructor.java:480)
 
   Locked ownable synchronizers:
	- None
```

I believe the situation that has occured is the following:

* `ForgeMod` triggers class initialization of `NetworkConstants` by calling `NetworkConstants.init()`. Since `NetworkConstants` references `HandshakeHandler` it also attempts to begin initializing that class.
* At the same time, Environmental references `HandshakeHandler` from its own mod initializer [here](https://github.com/team-abnormals/environmental/blob/12917a8afa07a7ae361e0dc32ad6823e5767eb39/src/main/java/com/teamabnormals/environmental/core/Environmental.java#L144C17-L147). Therefore it loads that class. Since `HandshakeHandler` references `NetworkConstants` it attempts to begin initializing that class.

However, Java synchronizes class initializers (to avoid running them twice from different threads) and so this leads to an instant deadlock. Each class needs to wait on the other to finish initializing (since they each reference each other in their static initializer) and so no progress can be made.

I see two approaches to fix this:

* Uglier, but quicker solution: force classloading of `NetworkConstants` and `HandshakeHandler` before any other mod initializers run.
* Cleaner solution: rewrite these classes to not depend on each other in this manner.

The issue of initialization of static fields causing deadlocks is well known. Here is a [Twitter thread](https://twitter.com/shipilev/status/1471216975485579267) from OpenJDK developer Aleksey Shipilëv with a simple example.


Discussion / hints:
Looks like this was introduced in the 1.17 update cycle. Pre-1.17, `ModLoader` had an explicit call early on to ensure that `NetworkConstants` was classloaded:

https://github.com/MinecraftForge/MinecraftForge/blob/09f1b8d43856643e4ab36d9d64e2176c440a3e72/src/main/java/net/minecraftforge/fml/ModLoader.java#L115

However, in 1.17 the call was [moved into the `ForgeMod` constructor](https://github.com/MinecraftForge/MinecraftForge/blob/50b5414033de82f46be23201db50484f36c37d4f/src/main/java/net/minecraftforge/common/ForgeMod.java#L148), which can run in parallel with other mods' constructors and opens up the possibility of this issue.

So this probably affects 1.18 and all 1.19.x versions.
Ok yeah, I see why this is happening. This is rough. Because the FML projects exist in their own modules, I can't directly reference the NetworkConstants class from ModLoader because that class exists inside of Forge, not FML. It becomes a race to see if ForgeMod can initialize the NetworkConstants before some other mod tries to set up network stuff.

The only thing I could think of that would fix this would be to call `NetworkConstants#init` from a static initializer inside of `ForgeMod`. Although I don't think I'd be able to reliably reproduce the issue once I do that. Thankfully this isn't an issue from 1.20.2 and onwards due to the networking rewrite, but it does effect 1.17.1 all the way until 1.20.1.
I'm not sure calling it from a static initializer in `ForgeMod` will work, unless the `@Mod` classes are all statically initialized on one thread, before their constructors are invoked in parallel. IIRC, that's not how it worked when I reviewed the FML code at the time.
It won't, that would've been a hail mary option in order to try and get it to load first. In the worst case scenario I could try using reflection in `ModLoader` or move the class itself to FML where it can be referenced.
I think the solution I use in a mod is to call `NetworkConstants.init` from vanilla's `Bootstrap.bootStrap` instead, which didn't seem to cause any problems.
Ok, will try that. Although I need to double check if `Bootstrap.bootStrap` is called before mod containers are constructed, I don't remember off the top of my head. Thank you!
Fixed by #10407 in https://github.com/MinecraftForge/MinecraftForge/commit/fa109380f7105ea2bccfe7ea899145acc24b0118

Fix patch:
diff --git a/patches/minecraft/net/minecraft/server/Bootstrap.java.patch b/patches/minecraft/net/minecraft/server/Bootstrap.java.patch
index d68e4c4f5d0..cf945325e92 100644
--- a/patches/minecraft/net/minecraft/server/Bootstrap.java.patch
+++ b/patches/minecraft/net/minecraft/server/Bootstrap.java.patch
@@ -1,10 +1,15 @@
 --- a/net/minecraft/server/Bootstrap.java
 +++ b/net/minecraft/server/Bootstrap.java
-@@ -53,6 +_,8 @@
+@@ -53,6 +_,13 @@
                 DispenseItemBehavior.m_123402_();
                 CauldronInteraction.m_175649_();
                 Registry.m_206101_();
 +               net.minecraftforge.registries.GameData.vanillaSnapshot();
++               // Forge: Hacky fix to ensure that NetworkConstants is loaded before mods are constructed.
++               // Many older mods use network internals that shouldn't be used, yet are exposed so they get used anyways.
++               // This can cause class-loading issues with ForgeMod loading NetworkConstants and HandshakeResolver.
++               // To ensure that doesn't happen, we load it here and now. This is not an issue in 1.20.2 and newer.
++               net.minecraftforge.network.NetworkHooks.init();
 +               if (false) // skip redirectOutputToLog, Forge already redirects stdout and stderr output to log so that they print with more context
                 m_135890_();
              }
diff --git a/src/main/java/net/minecraftforge/common/ForgeMod.java b/src/main/java/net/minecraftforge/common/ForgeMod.java
index fb977451344..a9be84423fb 100644
--- a/src/main/java/net/minecraftforge/common/ForgeMod.java
+++ b/src/main/java/net/minecraftforge/common/ForgeMod.java
@@ -433,7 +433,6 @@ public ForgeMod(FMLJavaModLoadingContext context)
             return uuid.toString();
         });
 
-        LOGGER.debug(FORGEMOD, "Loading Network data for FML net version: {}", NetworkConstants.init());
         CrashReportCallables.registerCrashCallable("FML", ForgeVersion::getSpec);
         CrashReportCallables.registerCrashCallable("Forge", ()->ForgeVersion.getGroup()+":"+ForgeVersion.getVersion());
 
diff --git a/src/main/java/net/minecraftforge/network/NetworkHooks.java b/src/main/java/net/minecraftforge/network/NetworkHooks.java
index 328ec4c636b..a2c5859f2ec 100644
--- a/src/main/java/net/minecraftforge/network/NetworkHooks.java
+++ b/src/main/java/net/minecraftforge/network/NetworkHooks.java
@@ -46,6 +46,11 @@ public class NetworkHooks
 {
     private static final Logger LOGGER = LogManager.getLogger();
 
+    public static void init()
+    {
+        LOGGER.debug("Loading Network data for FML net version: {}", NetworkConstants.init());
+    }
+
     public static String getFMLVersion(final String ip)
     {
         return ip.contains("\0") ? Objects.equals(ip.split("\0")[1], NetworkConstants.NETVERSION) ? NetworkConstants.NETVERSION : ip.split("\0")[1] : NetworkConstants.NOVERSION;


Test patch: