LootrMinecraft / Lootr

friendly loot
MIT License
45 stars 38 forks source link

1.21.1 Crash with ZGC arguments, when Lootr has to write it's config file. #463

Closed djnifos closed 2 weeks ago

djnifos commented 2 weeks ago

crash-2024-09-01_22.11.35-fml.txt This is predictably a crash with ATM 10 when launching without the config file existing.
user_jvm_args.txt Args are attached.

Anything else I can do to help please let me know, I am glad to test and provide any info.

djnifos commented 2 weeks ago

This is in a server environment, should have mentioned.

noobanidus commented 2 weeks ago

You're specifically mention the custom ZGC arguments; does this still happen without them? And you said "without the config file", does this not occur if the file already exists?

From what I can see, NightConfig is failing to write lootr-common.toml, but more specifically it's failing to replace lootr-common.toml with lootr-common.toml.new.tmp. The actual error itself is an AccessDeniedException.

This implies to me that whatever process you're using to launch the Minecraft server doesn't have permission in the relevant config folder (or it doesn't exist), or that the lootr-common.toml file has different permissions assigned to it than what would be expected for your user profile.

I would definitely recommend trying things like the following (if you haven't already): deleting the config folder if it exists and re-creating it, deleting any config files that are found in it, and ensuring that the folders you are writing to have the correct permissions.

That said, digging a bit deeper, you appear to be doing stuff inside of AppData/Roaming. From personal experience these folders have been a bit funky when it comes to permissions. Another option would be to try running your server out of the user directory itself, instead of the AppData/Roaming, unless you absolutely need the synchronization of the roaming folder.

djnifos commented 2 weeks ago

As part of our upgrade process, we recommend server operators remove the config, kubejs, and mods folders. They then replace them with what we provide. We don't have a current lootr config shipped as part of our config files image I can confirm that this does not happen with G1GC. I can also confirm that the fix to this is to just launch it again. The second launch works with no changes.

I've moved the server out of roaming and into the root of C, and it's some other mod's crash now. Sorry for the confusion, it's hard to say Lootr had anything to do with it at this point.

noobanidus commented 2 weeks ago

No worries! Thanks for the response.

djnifos commented 2 weeks ago

So had an update this morning, and crashed the same crash from the C drive. Launched the server again. So in total: 1 failed launch, 1 successful launch. Just an odd bit of artifact in the config folder. It would appear the lootr-common config was created more times than the server started? I know I closed the issue, but it's awfully strange to me. image https://mclo.gs/yrjvejo

noobanidus commented 2 weeks ago

This is a pretty common thing in my development environment. I'm not sure why nightconfig does it -- I think potentially its part of the "this file didn't match what I expected, so I'm replacing it with defaults". I haven't dug deeply into it however.

That said, one of the more commonly repeated errors that gets reported is nightconfig not being able to load the configuration file -- but that tends to cause the mod to crash when loading instead of causing backups.

I guess: I'm not 100% sure why it does this, but you shouldn't need to worry about it.

pietro-lopes commented 2 weeks ago

I'm suspicious of this:

https://github.com/LootrMinecraft/Lootr/blob/8abdc269e2052812bc424374342ca6916c7ee17c/neoforge/src/main/java/noobanidus/mods/lootr/neoforge/Lootr.java#L45-L46 https://github.com/LootrMinecraft/Lootr/blob/8abdc269e2052812bc424374342ca6916c7ee17c/neoforge/src/main/java/noobanidus/mods/lootr/neoforge/config/ConfigManager.java#L167-L170

maybe move/remove this logic to Load event? I see that current implementation of Configs do not touch nightconfig internals.

AE2 Config as reference https://github.com/AppliedEnergistics/Applied-Energistics-2/blob/92e8deec2417040a46274f3789e97abff485585e/src/main/java/appeng/core/AEConfig.java#L54-L74

noobanidus commented 2 weeks ago

I'm actually not sure if this code is even required at this point, @pietro-lopes . I'll blame EpicSquid who used this in the very first mod that used the nightconfig setup, and was just copied into subsequent projects. I'll mark it as something to check out, thanks.

noobanidus commented 2 weeks ago

I'm suspicious of this:

https://github.com/LootrMinecraft/Lootr/blob/8abdc269e2052812bc424374342ca6916c7ee17c/neoforge/src/main/java/noobanidus/mods/lootr/neoforge/Lootr.java#L45-L46

https://github.com/LootrMinecraft/Lootr/blob/8abdc269e2052812bc424374342ca6916c7ee17c/neoforge/src/main/java/noobanidus/mods/lootr/neoforge/config/ConfigManager.java#L167-L170

maybe move/remove this logic to Load event? I see that current implementation of Configs do not touch nightconfig internals.

AE2 Config as reference https://github.com/AppliedEnergistics/Applied-Energistics-2/blob/92e8deec2417040a46274f3789e97abff485585e/src/main/java/appeng/core/AEConfig.java#L54-L74

I've made this change in the most recent release, we'll see how it goes.

djnifos commented 1 week ago

Just got a first start with ZGC on a server, no crash!

noobanidus commented 1 week ago

Thanks for the report @djnifos and @pietro-lopes for pointing out the potential issue!