Sophisticated Backpacks

Sophisticated Backpacks

89M Downloads

Crash when placing the backapck

castilio20 opened this issue ยท 6 comments

commented

Describe the bug
The game crash when i try to place the backpack

crash-2023-05-19_13.04.37-client.txt

Mod version sophisticatedbackpacks-1.18.2-3.18.48.835
forge 40.2.4

commented

@embeddedt yes that change will fix it here, however my issue with that is the fact that I don't believe this part of the code is multithreaded in vanilla (at least i haven't found that is the case doing a quick look at uses of the code. Which means that by doing this I am in fact making that part of the code slower for most of the cases unless your mod is included only to allow for compatibility here. (but I may be missing something and this part of the code is infact multithreaded in vanilla/forge just find it surprising it has existed there for such a long time without a single crash).
And if that gets applied in general for anything that could become an issue of performance and the other part is that still means that other mods that do something that relies on single threaded environment need to adjust to allow for multithreading and prevent crashes. That just doesn't seem right to me.

commented

The issue was not triggered by ModernFix from what I can tell, as the vanilla method in question already bypasses the blockstate cache and calls the underlying block's getShape function directly, so my optimizations to the blockstate cache shouldn't be changing behavior here. All it would take is a mod deciding to call getShape from a worker thread, or the lighting engine/some other vanilla subsystem to do the same.

It's not surprising that this hasn't been reported before. These types of crashes can be very rare, as the map is unlikely to be concurrently accessed at exactly the same time by two threads. I have had to fix many CME-related issues in other buggy mods and they are incredibly hard to reproduce even if the code clearly has the problem.

commented

I don't think ConcurrentHashMap will add any meaningful overhead; it is pretty fast and backpacks being placed in the world is not a common occurence. I would suggest profiling before worrying about that.

However, if you insist on not using it the other safe option is to precompute every shape in this HashMap at launch time so that you don't need to use computeIfAbsent; concurrent retrieval from HashMap is safe as long as no one writes. This is fine if there are a relatively small number of shapes, otherwise the memory cost of storing all of them at once might be problematic.

Generally I have found it's best to assume the Block methods may be called from multiple threads, even if some of them are normally not used off the main thread. I know for sure that the lighting engine and worldgen both make use of worker threads and call some of these methods.

commented

Tested this and I was wrong, even in my dev environment which pretty much has just vanilla + forge I can see multiple threads running this part. So it not crashing is likely caused just by the fact that the whole cache gets intialized during startup and then multiple threads only read from it. So I am definitely going to switch this over to CHM

commented

Hi there, this was reported to me as the player is also using my performance mod and I believe the culprit is that your shape cache is not using a ConcurrentHashMap, meaning if two threads attempt to retrieve a backpack shape for the first time at any point, there is a potential for a crash. Swapping to a CHM should be a one-line change and will completely eliminate this issue as far as I know.

commented

This is fixed in the latest release