Seemingly random crashes

[Maltshakes]

I've been getting recurring crashes on 1.16.5, mod version 0.3.8. I installed Blame to try and narrow down the cause, and it seems to point towards feature placement from this mod.

https://pastebin.com/JQeNwZCS

[kreezxil]

Confirmed, I just got this too.

My crash report from the server: https://gist.github.com/kreezxil/17af2216b8562c7f66430a7c00ea6583
My latest log from the server: https://gist.github.com/b37a112f3a46fa3229736be3c1926da4

I don't know what my players were doing to trigger it.

Mod version: 1.16.4-0.3.8
Forge version: 1.16.5-36.2.9

[yungnickyoung]

Both of you double check that your configs contain all the necessary information. You may be missing an item or two. You can try deleting the betterportals directory entirely and restarting the game to see if the issue is fixed. In particular, @Maltshakes make sure you have the insideSelector setting included in each variant entry in your monoliths json, and @kreezxil make sure you have the spawn chance setting included in each variant entry in your monoliths json.

[kreezxil]

@yungnickyoung my monoliths.json, it has the chance set to 0.1 https://gist.github.com/16b70a4129f609652c06e4961759e8ac

seems that line 58 is the issue for me

https://github.com/yungnickyoung/YUNGs-Better-Portals/blob/447a94d995066ce4aa54b658602b64616196374c/src/main/java/com/yungnickyoung/minecraft/betterportals/world/feature/MonolithFeature.java#L58

i'm guess the config is capable of sending a null, a simple fix might be to change the line to

if (setting.getSpawnChance() != null && sharedSeedRandom.nextFloat() > settings.getSpawnChance()) {

[kreezxil]

I'll delete my configs for this just in case, they were stock anyway.

[Maltshakes]

Thanks, I'll try generating a fresh set of configs. I didn't make any changes to them anyway.

[kreezxil]

From your Discord, we have a solution and we know why you couldn't duplicate it.

[6:37 AM] TelepathicGrunt:
Caused by: java.lang.NullPointerException
    at com.yungnickyoung.minecraft.betterportals.world.feature.MonolithFeature.generate(MonolithFeature.java:95) ~[?:1.16.4-0.3.8]
 hmm
[6:38 AM] TelepathicGrunt: https://github.com/yungnickyoung/YUNGs-Better-Portals/blob/447a94d995066ce4aa54b658602b64616196374c/src/main/java/com/yungnickyoung/minecraft/betterportals/world/feature/MonolithFeature.java#L95
[6:38 AM] TelepathicGrunt: Only thing I can see being null to give that crash is settings
[6:39 AM] TelepathicGrunt: But there’s a check for null above
[6:39 AM] Kreezxil: maybe it's a null on the nextint
[6:39 AM] Kreezxil: is the randomizer initialized corrrectly
[6:41 AM] TelepathicGrunt: @YUNGNICKYOUNG you’re storing settings as a field for the feature. Remember, there’s only one instance of that feature that you registered. So when chunks load at the exact same time with your feature in both, generate method is called twice for that instance of the feature.
[6:41 AM] TelepathicGrunt: This is a race condition
[6:42 AM] Kreezxil: so the npe is false but it's the closest forge has to saying "i have a race condition"?
[6:47 AM] TelepathicGrunt: A race condition is an issue that depends on certain code executing out of order base on who finished or starts first. 

In this case, let’s say a chunk in overworld is being genned by a player in overworld and another player is in a modded dimension generating chunks there without the portal.

Both chunks begins generate very close to the same time. Overworld goes through to line 56 and has a non-null settings. The modded dimension feature now just executed line 47 and got null because the dimension doesn’t have the feature. It now turns that settings field to null to store it. 

Remember, the feature in overworld and modded dimension is the same exact instance. So that field is shared between them. Now the overworld feature tries to continue past line 56 but it has a null settings field that was set by the modded dimension’s call to generate at nearly same time. That’s the race condition and why yung’s has issue reproducing it 
[6:50 AM] Kreezxil: nice
[6:50 AM] Kreezxil: now you guys know what's going on. What's the preferred way to fix that?
[6:51 AM] Kreezxil: oh nvm
[6:52 AM] Kreezxil: he has to register another instance for the other dimensions
[6:52 AM] Kreezxil: and use the right instance based on dimension 
[7:07 AM] TelepathicGrunt: I mean, null is a valid result. The issue is the settings field is shared across all generate calls which modified the settings field
[7:07 AM] TelepathicGrunt: Not threadsafe
[7:10 AM] TelepathicGrunt: I would make getVariantForDimension be a map of world registrykeys to settings. Then move the settings field to being a local variable in the generate method and always call getVariantForDimension every time. The map would be a quick lookup to get the settings for the dimension and since the settings is now a local variable, it won’t be modified by other generate calls
[7:20 AM] Kreezxil: you're a java god, i'm really liking this information.
[7:20 AM] Kreezxil: so deleting the config for it wasn't a solution at all then
[7:21 AM] Kreezxil: the problem was, which shouuld not have been, but was, players in both dims at the same time.
[7:21 AM] Kreezxil: something not possible to replicate indev because it's just you, going from one dim and back and again. 

TL;DR

According to your server moderator, the problem happens when the FeaurePlacement is run in a multiplayer scenario where there are active players in the overworld and in the nether at the same time. So there are times when the data is, in fact, null and thus the npe gets thrown because the same instance is being used for both dimensions.

Solution appears to be

make getVariantForDimension be a map of world registrykeys to settings. Then move the settings field to being a local variable in the generate method and always call getVariantForDimension every time. The map would be a quick lookup to get the settings for the dimension and since the settings is now a local variable, it won’t be modified by other generate calls

[yungnickyoung]

@kreezxil @Maltshakes Please try version 0.3.9 and see if that fixes the problem.

[kreezxil]

Alright, plugging it in now, and we'll shall see how it goes for the weekend. Which is when I got so many players on that the race condition can happen.