MC-258939Non-atomic cached state can cause multithreaded crashes
ItemStacks have a an emptyCacheFlag that is used to return AIR instead of null from getItem(). This flag is updated every time count is set, however, and it is set to false momentarily before being updated to the true state in ItemStack.updateEmptyCacheFlag(). It is possible that calling ItemStack.setCount() on ItemStack.EMPTY on one thread during a very unlucky window can cause another thread to get null back from ItemStack.getItem(), which will almost universally cause a crash, as the method is not nullable, and returns AIR instead of null when the emptyCacheFlag is set correctly. Crashes like these are somewhat rare, but common enough that I've seen it happen close to a dozen times.
I don't have any unmodded crash reports because I always play with mods, however, this crash is easily possible in vanilla, and provides a very rare and hard to diagnose crash when calling ItemStack.setCount(), which is normally completely safe even on ItemStack.EMPTY. Ideally, this method should be changed to be atomic, and not have ItemStack.EMPTY flicker to be cached as non-empty frequently.
Crashlog from another user: https://mclo.gs/sGocG0Z
Linked Issues
Attachments1
Created Issue:
Non-atomic cached state can cause multithreaded crashes
ItemStacks have a an emptyCacheFlag that is used to return AIR instead of null from getItem(). This flag is updated every time count is set, however, and it is set to false momentarily before being updated to the true state in ItemStack.updateEmptyCacheFlag(). It is possible that calling ItemStack.setCount() on ItemStack.EMPTY on one thread during a very unlucky window can cause another thread to get null back from ItemStack.getItem(), which will almost universally cause a crash, as the method is not nullable, and returns AIR instead of null when the emptyCacheFlag is set correctly. Crashes like these are somewhat rare, but common enough that I've seen it happen close to a dozen times.
I don't have any unmodded crash reports because I always play with mods, however, this crash is easily possible in vanilla, and provides a very rare and hard to diagnose crash when calling ItemStack.setCount(), which is normally completely safe even on ItemStack.EMPTY. Ideally, this method should be changed to be atomic, and not have ItemStack.EMPTY flicker to be cached as non-empty frequently.
Crashlog from another user: https://mclo.gs/sGocG0Z
![[Mod] ampolive](../../../data/avatars/users/18949.svg){kind=small}
![[Mojang] slicedlime](../../../data/assets/gravatar/slicedlime.jpeg){kind=small}
![[Bot] Arisa](../../../data/avatars/users/43007.png){kind=small}
Very Important![[Mojang] Gegy](../../../data/avatars/users/10122.svg){kind=small}
[Mojang] Gegy![[Mod] Michael Wobst](../../../data/avatars/users/37868.png){kind=small}
![[Bot] Piston](../../../data/avatars/users/31202.png){kind=small}
Please attach a crash report from vanilla.
I've uploaded a new crash report in vanilla, it took several hours to get, but eventually did crash. It seems to have crashed with an NPE at Container.countItem() when ItemStack.getItem().equals() was invoked. ItemStack.getItem() is of course, supposed to not be nullable. The only way this is possible is the object being modified on another thread. My assumption is is this is caused by setCount() being invoked on ItemStack.EMPTY but there's no way for me to be sure, it could potentially be some other ad hoc empty stack being synced through the integrated server.
On that note, I've realized there's a potential hard to quantify bug relating to this. ItemStack.getItem() is supposed to return AIR when count is zero, however, for stacks synced across the integrated network protocol it is hypothetically possible this same non-atomic bug could cause empty stacks return their original item. This seems unlikely to crash in most situations, but could definitely cause logic to malfunction. I don't think I can prove this with a log so I'll just leave it as a hypothetical issue related here.