Concurrent Chunk Management Engine (Fabric)

Concurrent Chunk Management Engine (Fabric)

231k Downloads

Replace CheckedThreadLocalRandom with a custom Random or ThreadSafeRandom

fzzyhmstrs opened this issue ยท 4 comments

commented

Is your feature request related to a problem? Please describe.
The introduction of a LocalRandom by c2me introduces hard crashes that force modpack devs, mod devs, etc to fix issues under rules they didn't know they were subject to, and weren't subject to when writing their mod. This is frustrating to say the least and leads to the following things happening which would probably be frustrating to you:

  1. They open issue report with c2me instead of with the source of off-thread randomness because the last line of the stack trace shows this:
    [15:21:51] [Thread-30/INFO]: [STDERR]: at com.ishland.c2me.fixes.worldgen.threading_issues.common.CheckedThreadLocalRandom.method_43156(CheckedThreadLocalRandom.java:32)
  2. They simply remove or downgrade c2me. Not ideal from your point of view for several reasons I'm sure. You want c2me's latest features included.

or otherwise if the modpack author is commited to determining a cause, can be frustrating for them under the following (relatively common) example circumstance and I'm sure plenty of others:

  1. Mod A adds an entity. Nothing they've done is problematic. Adding entities is normal.
  2. Mob B introduces some worker thread that happens to instantiate an entity from Mod A, maybe to display it in some GUI layout. They have done nothing problematic, since Mod A hasn't introduced a potentially problematic random usage, so Mod B isn't adding thread unsafety to Mod A's mob.
  3. Mod C adds variants to Mod A's mobs, using the world random. Uh Oh!! Mod B is now introducing thread-unsafety they did not know they were introducing, and aren't introducing under normal circumstances. Mod C's random usage would be the source of issues, and should be the target of fixes.

The issue:

  1. With the thread access exception that is now being caused (they didn't know they had a chance to cause, because they don't know Mod C exists), the crash report will look something like this, with representative Mods A, B, C pointed out:
    image

  2. With this example, note that Mod A is actually vanilla minecraft, so mostly avoids suspicion completely.

  3. Mod C is the tail of the stack trace causing the issue, but doesn't prepend their mod-id to the mixin handler, so per this stacktrace we have no idea what mod it is

  4. Mod B, which isn't actually the source of the issue, simply the unknowing cause of the issue (in a way they normally would not be the cause of an issue) is the only easy-to-track mod here. They get the issue report. It confuses them because they are just client-side instantiating mob instances to render in GUIs later, something that is normally ok to do off-thread. In fact, Mod B is a plugin for another mod that is actually adding the worker threads, which normally don't even instantiate entities in the worker threads.

  5. Even with a more clear stack trace, if the modpack dev isn't simply bothering c2me first (due to the above), they now have to wait for a fix, and possibly remove the offending mod or c2me completely in the meantime.

This is frustrating. It's enforcing rules upon people that didn't know they now have certain rules enforced upon them, and often only issues appear when interactions of multiple other mods they don't know about are introduced. The Readme, FAQ, and CF description mention nothing about this random change, nor give users any advise on what to do in this situation, a situation introduced explicitly by c2me.

Describe the solution you'd like
Solution 1. Use the pre-packaged ThreadSafeRandom. This fixes the issue c2me is trying to fix in a way that introduces no frustration to modpack devs who were not expecting to have to deal with rule enforcement as part of pack updating. It is noted that this random is deprecated, so
Solution 2. Create a custom Random that, instead of hard-crashing, prints an error report into the logs upon improper usage detection. This log could state things like:

  1. Print what thread it found the issue on,
  2. Print an explanation of why the error is appearing, and steps to take to make it not appear,
  3. print a stacktrace where it detected the issue, providing the same data a crash would, without crashing.
  4. The modpack dev or user may notice their log being populated with these errors and track them down the same as with a LocalRandom crash, but without the crash; letting them continue their update testing etc. In the case of a concurrent access exception, the log will again, have this error report included by c2me that points to the proper root cause and allows for proper issue creation/fixing.

Describe alternatives you've considered
Two alternatives mentioned above, but the fallback alternative would be adding a description and solution guide to the LocalRandom crashes introduced by c2me, either in the Readme, description, or FAQ. Readme probably the ideal place for this fallback.

Additional context
Some context provided inline with the suggestion explanation.

commented

With this line, it seems like a good change that meets the spirit of my recommended Solution 2:
(You may make this a fatal warning instead of a hard crash in c2me.toml)

And the addition of printed help information is good too.

The only thing I might recommend is stating exactly where in the .toml file to look for the option to change (what section and key to look for). Besides that, seems good!

commented

With 3eb1512, the crash now looks like this (intentionally triggered crash)

---- Minecraft Crash Report ----
// Don't be sad. I'll do better next time, I promise!

Time: 2023-02-18 23:54:41
Description: Exception in server tick loop

com.ishland.c2me.fixes.worldgen.threading_issues.common.CheckedThreadLocalRandom$1: ThreadLocalRandom accessed from a different thread (owner: Thread-4, current: Server thread) 
This is NOT a bug in C2ME, but a bug in another mod or in vanilla code. 
Possible solutions: 
  - Find possible causes in the stack trace below and 
    - if caused by another mod, report this to the corresponding mod authors 
    - if no other mods are involved, report this to C2ME 
 
 (You may make this a fatal warning instead of a hard crash in c2me.toml)
Caused by: java.util.ConcurrentModificationException: ThreadLocalRandom accessed from a different thread (owner: Thread-4, current: Server thread) 

	at com.ishland.c2me.fixes.worldgen.threading_issues.common.CheckedThreadLocalRandom.handleNotOwner(CheckedThreadLocalRandom.java:55)
	at com.ishland.c2me.fixes.worldgen.threading_issues.common.CheckedThreadLocalRandom.isSafe(CheckedThreadLocalRandom.java:38)
	at com.ishland.c2me.fixes.worldgen.threading_issues.common.CheckedThreadLocalRandom.next(CheckedThreadLocalRandom.java:81)
	at net.minecraft.util.math.random.BaseRandom.nextInt(BaseRandom.java:11)
	at net.minecraft.world.World.redirect$zbm000$redirectWorldRandomInit(com/ishland/c2me/fixes/worldgen/threading_issues/mixin/threading_detections/random_instances/MixinWorld.java [c2me-fixes-worldgen-threading-issues.mixins.json]:20)
	at net.minecraft.world.World.<init>(net/minecraft/world/World.java:120)
	at net.minecraft.server.world.ServerWorld.<init>(net/minecraft/server/world/ServerWorld.java:211)
	at net.minecraft.server.MinecraftServer.createWorlds(net/minecraft/server/MinecraftServer.java:370)
	at net.minecraft.server.MinecraftServer.loadWorld(net/minecraft/server/MinecraftServer.java:339)
	at net.minecraft.server.dedicated.MinecraftDedicatedServer.setupServer(net/minecraft/server/dedicated/MinecraftDedicatedServer.java:169)
	at net.minecraft.server.MinecraftServer.runServer(net/minecraft/server/MinecraftServer.java:695)
	at net.minecraft.server.MinecraftServer.method_29739(net/minecraft/server/MinecraftServer.java:266)
	at java.base/java.lang.Thread.run(Thread.java:1589)

I would like to know if this is good enough and whether this can be further improved.

commented

The only thing I might recommend is stating exactly where in the .toml file to look for the option to change (what section and key to look for). Besides that, seems good!

This should be added now.

commented

Closing as resolved.