[1.16.5] VanillaFoodPantry/Carrots Library classloads the TagCollectionManager far to early
Closed this issue · 7 comments
Hello! It appears that org.jwaresoftware.mcmods.lib.RID.getTag is called a ton in your mod and far too early. This is an issue as I received a report on my mod that my structures were crashing: https://www.curseforge.com/minecraft/mc-mods/repurposed-structures?comment=741
So I ran VanillaFoodPantry/Carrots Library alone with my other mod called Blame and the log stated all the places that TagCollectionManager would be classloaded from:
latest.log
And here's a slice of the massive log the user gave me (see the bottom where all the worldgen stuff was broken due to TagCollectionManager being classloaded too early):
2021-06-14-1.log
The reason why classloading TagCollectionManager at mod startup is dangerous is because if it is classloaded before another mod can register their tags in code, the tag will blow up where-ever it is used as it now will not exist in TagCollectionManager. In this case, TagCollectionManager was classloaded too early, broke The Undergarden's tag that they use in their worldgen features, and the parse error cascaded out of that json file into all worldgen json files including my own which caused a massive ton of our mods to no longer spawn and crash in some cases.
The best solution would be to never try to get a tag before a world exist as tags really aren't a thing at mod startup and reading tags before the world is entered defeats the purpose of letting users configure tags with datapacks that aren't around at mod startup. But if you need to use TagCollectionManager early, could you try delaying it to after FMLCommonSetupEvent is finished for all mods? At that point, all mod's tags should be finished being registered in code. Though if you are just needing a reference to a tag to store somewhere to grab after the world is made, try using Lazy.of(() -> <the tag>) to create a lazy that will only run its code when you call .get() on it later and thus, prevents TagCollectionManager from being classloaded too early.
I hope this helps!
I'm looking at the latest.log you provided: most calls seem to be part of recipe validation or in response to search asking for item groups contents; exactly how would this code be rewritten "to run at a later time"? For instance -- the system calls ::fillItemGroup...and I read a tag to determine what to fill in...again, how do I not do that when asked to some times and do it other times?
Except for one case (recipe deserialization), all the stack traces are coming from Item::fillItemGroup being called by the system itself, not as part of my mod's startup. Maybe I'm missing something in the logs, but I have checked each stack trace and only see recipe validation and responses to fillItemGroup requests where World references are not supplied so I cannot tell who/when/why it's being called. The way Mojang's search initializes has always been a royal pain since 1.13 due to how tags work; Item groups are inherently UI-related, hence the world 'should' be available, but it is not because..caching or something. I imagine a magical global flag that is set after world load might work, but I'm not convinced enough to change all my code to have such a flag at this time AND the search results would be wrong for my items as search is cached once (I recall JEI recently tweaked something itself to work around this very issue with out-of-date search results with tags being queried by fillItemGroup for mods).
In the immediate timeframe I recommend you tell the user to remove VFP; it's just a food mod.
From the looks of the log, the first few were caused by VfpBuilder eventually directly calling getTagManager from within your mod at mod startup so it doesn't go through fillItemGroup. The later ones appear to be kicked started by initializeSearchableContainers in vanilla but then VfpPantryMultiItem hooks in and eventually calls getTagManager and this occurs for every food in your mod. It does seem like the initializeSearchableContainers does call fillItemGroup but vanilla does not access tags at all in it.
I wish I could help you out but since the mod is closed source, I can't really look at the code to get a better sense of what you are doing and what your goal is. But if I am understanding right by fillItemGroup, you're having trouble getting your items into a creative tab? If so, it's actually pretty easy and doesn't need tags at all. The item property has a method called tab that you can pass in the tab that the item should appear in (I am using mojmap so my methods may be different)
https://github.com/TelepathicGrunt/Bumblezone/blob/2ce4b400e910ed33d0fdb4807494742f4970de62/src/main/java/com/telepathicgrunt/the_bumblezone/modinit/BzItems.java#L35
Does that help or am I misunderstanding what you are trying to do?
I did take the time to debug through these: there is the first path (VfpBuilder)...that is at recipe deserialization where tags ideally should be present but are not yet apparently (the method is a used 3 times from a single IRecipeSerializer event use). As for the second path (the vast majority) the tags lookups are really for tags not item tabs; some VFP items are optional and only appear if certain tags are non-empty which has nothing to do with item tab determination (VFP uses its own tab exclusively so nothing needs to be checked for this). As noted the search caching issue is a well known one. There is a no "proper method" path here...it's simply an awkward situation that is caused by the manager class being tied to the loading of its data (I guess like tying a database manager to a specific database rather than having those two things be independent).
As I mentioned earlier, only a global flag that is set by the custom tags loaded event would suffice (again I tested this before posting the response about using a flag as a possible solution). I have tested using such a flag and I noticed the breakage w/ JEI due to the broken search results. The latest versions of JEI fixes this by sending itself a reload message based on the tags loaded event as well I believe. So technically the issue is resolved, it's just an ugly solution that I'm not willing to declare "mission accomplished" until I have tested it further w/ all my dependent mods.
Ah sounds good. Good luck!
@TelepathicGrunt Ping. Is there a particular configuration for repurposed structures + the undergarden that I should test against to ensure the issues are resolved? I'm prepping another VFP release and have time to tackle this issue. Currently using: repurposed_structures-1.16.5-2.7.10-forge.jar and The_Undergarden-1.16.5-0.5.4.jar off-the-shelf (no config changes from whatever defaults).
Just the mods with default configs was enough to trigger Undergarden’s tag to blow up and cascade the error through my mod as well. Having the mod called Blame on will show if the worldgen json files blew up or not in the latest.log. If no blame report about worldgen files shows up, then the problem is resolved
Workaround for this added to 6.0 version. Not a complete solution (due to issue the JEI reload being too early but should stop the calls to early tag loading). Reopen if necessary.