PersonTheCat / OreStoneVariants

A powerful utility for generating new blocks when given a foreground and background.
GNU General Public License v3.0
7 stars 8 forks source link

Concurrent Modification Error (1.14-1.16.2) #87

Closed Omnimetatron closed 3 years ago

Omnimetatron commented 4 years ago

I'm currently working on building a modpack and this mod seems to be causing a crash. I have removed the mod for now to continue testing, but would really like to be able to address the issue.

https://pastebin.com/YMW5h6ac

Hope this helps ^^

PersonTheCat commented 4 years ago

Hey, thanks so much for reporting this! It seems like there are more concurrent operations than I was aware of since 1.13. As a result, this mod doesn't play very nicely with some other mods. This kind of thing is usually pretty easy to fix in Java, but unfortunately, the crash report doesn't give me much to work with, so I really have no idea where the problem is coming from. What I need is to find just one single mod that doesn't work alongside OSV to hopefully help me figure out what they have in common. I'll be working on that this weekend, but I'm not sure how long it'll take.

DatrixTHLK commented 4 years ago

Sorry to beat a dead horse, but I'm also receiving this same issue as well. Not sure it's the same thing as the person who posted first.

2019-11-18 (2) Latest FML Log: https://gist.github.com/DatrixTHLK/948fc71ac2a207031bd24e8235cf4155 Crash Log: https://gist.github.com/DatrixTHLK/6237138a07ad55aa12866af5dff78bc0

Minecraft 1.14.4 Forge: 28.1.90 Ore Stone Variants: 5.0-b1

EDIT: A mod I've used that seemed to cause an issue before removing it was Water Strainer might want to give that a try and see if that helps

mjac0by commented 4 years ago

I experienced the same problem with OSV 5.0b1 and Geolosys 4.0.10.

PersonTheCat commented 4 years ago

Hello. Sorry it's been so long, but I believe I have fixed this issue. Feel free to try the latest 1.14 build and let me know if it's working for you as well. Thanks again for all your help!

ore_stone_variants-5.2-1.14.zip

PersonTheCat commented 4 years ago

Closing this for now. Please reopen the issue if it still occurs for you in either 1.14 or 1.16.

DatrixTHLK commented 4 years ago

Same Error for 1.16.2 the only difference is that null isn't mentioned in the error message OSV: 5.5 Forge: 33.0.37

Crash Log: https://gist.github.com/DatrixTHLK/c02207d0dbbbed1a25c4249bff970f0e Crash Report: https://gist.github.com/DatrixTHLK/d917ea5e011c38cff6235cffeeb76c7d Mods: https://filebin.net/5o3i9xqg9s9tlvif

PersonTheCat commented 4 years ago

@DatrixTHLK which other mods are you using? Also, please share the crash log.

PersonTheCat commented 4 years ago

@DatrixTHLK I have some updates, but still need some help. When I first tried these mods, I got a reflection error related to kotori316.scala_lib, which after some research I determined was the result of LargeFluidTank. I removed this mod and was not able to reproduce the error you were getting. Furthermore, I took a look inside of alltheores(...).jar and noticed that it also modifies the ConfiguredFeature registry inside of BiomeGenerationSettings, but it does not use synchronized collections. I suspect that this does contribute to the error you're seeing and cannot confirm whether OSV is responsible.

Is there any way you could try to remove either of these mods and confirm if that changes things for you as well? If the problem is related to AllTheOres, you'll need to file a ticket with the devs.

DatrixTHLK commented 4 years ago

For me I had to take out all the mods in currently minus jei, level hearts and OSV and it works just fine.

  1. Added back in falling trees and the biomes you'll go (got an error message) 3 Took out the biomes you'll go and the error went away. Me thinks it is because it also has differing stone variants and might utilize the same hooks or overrides yours.
  2. Added in clumps and ATO Mod (same error)
  3. Took out ATO mod because of the reasoning you mentioned and no error happens
  4. Added in Xaero's Minimap and World map and Zmodskills (no error)
  5. Added in endercompass, exnihilosequentia, fallingtree and forgiving void. (no error)
  6. Added in Dimstorage, Mixinbootstrap, oreseeds (no errors)
  7. Added in Shetiphaincore, terraqueous, and spatial harvesters (no errors)
  8. Added in Scalablecatsforce, reap, repair chests (no errors)
  9. Added in large fluid tanks (no errors)

Short version Biomes you'll go and ATO are the ones incompatible with OSV. Your mod has no issues without those two. I'll put in a ticket with both of them. Thanks for looking into the issue and taking it a step further.

CorgiTaco commented 4 years ago

Not exactly sure how this is BYG's issue. Only thing I can assume, is that OSV is crashing bc it's still under the assumption that the given field is immutable when in fact BYG Converts this field to mutable. I'm not really good at reading reflection code but that crash makes me believe it's the reflector causing the crash when converting the field to mutable as seen here: https://github.com/PersonTheCat/ore_stone_variants/blob/53331d6163bf05c18bed267fa51ebd3afa3aa6a3/src/main/java/com/personthecat/orestonevariants/world/OreGen.java#L76

One does not need to reflect and can just modify the variable directly as i do here: https://github.com/CorgiTaco/BYG/blob/c3d12ee9401b2482726ec5e755fa50d2fdc0a4ad/src/main/java/voronoiaoc/byg/common/world/feature/biomefeatures/BYGFeaturesInVanilla.java#L18

But then again reading this, I'm as lost as y'all.

CorgiTaco commented 4 years ago

And im assuming this because of this seen in the crashlog:at java.lang.reflect.Method.invoke(Unknown Source) ~[?:1.8.0_261] {}

DatrixTHLK commented 4 years ago

Hmmmm....It's also possible I stated incorrectly how the mods were incompatible with one another, since I'm not fluent in programming. My apologies on that one.

PersonTheCat commented 3 years ago

@CorgiTaco This is complicated, so I made a list (pun intended).

  1. The Method.invoke(Unknown Source) in the stacktrace is coming from FMLClientLaunchProvider. It probably happens when it first starts loading the new MC client and thus before any mods are loaded.
  2. I don't think this has to do with reflection in OSV. In fact, I rechecked BiomeGenerationSettings.java and the field definitely is private and final. I bet the difference in what we're seeing has to do with Forge vs. Fabric.
  3. I do think you're right about OSV replacing the list after BYG in the log. If that's true, there's a concurrency issue with the new type of list placed there by OSV. I made some changes on my end, but I think the order in which this field is modified depends on the number of mods, available threads, and clock speeds. If that's true then we both need to make a similar change until Forge ultimately does it for us.

In the meantime, @DatrixTHLK Is there any way you could try this build out for me with the original setup and see if the crash still occurs? I have yet to actually see it in person. Also, please share the log. Thanks!

Ore Stone Variants-5.6-1.16.2.zip

DatrixTHLK commented 3 years ago

It works just fine now. No errors were received. Thanks for the update and response to this.