[Forge 1.16.1] Crash while locking a slot type in an onUnequip() method override

[Foozey]

Versions:

• Curios: 1.16.1-3.0.0.2
• Forge/Fabric: 1.16.1-32.0.108

Observed Behavior: Item slot locking/disappearing, then game crashing immediately after

Expected Behavior: Item slot locking/disappearing, without game crashing

Steps to Reproduce:

1. Lock a slot type using CuriosApi.getSlotHelper().lockSlotType("name", livingEntity); within an onUnequip() method override of a different slot type
2. Open the game, unequip the item that locks the specific slot type
3. Observe crash

Crash Log: Pastebin

[Aizistral]

I'm not sure how you did expect this to play out totally fine. Take a look at Curios event handler, if you are curious:

Curios/src/main/java/top/theillusivec4/curios/common/event/CuriosEventHandler.java

Line 328 in 2b76762

public void tick(LivingEvent.LivingUpdateEvent evt) {

It crashes because you basically attempt to modify the list of available slot handlers while Curios is in the process of cycling through that list to call things like onEquip()/onUnequip()/onTick()/etc., henceforth is your ConcurrentModificationException.

[Foozey]

I'm not sure how you did expect this to play out totally fine. Take a look at Curios event handler, if you are curious:

Curios/src/main/java/top/theillusivec4/curios/common/event/CuriosEventHandler.java

Line 328 in 2b76762

public void tick(LivingEvent.LivingUpdateEvent evt) {

It crashes because you basically attempt to modify the list of available slot handlers while Curios is in the process of cycling through that list to call things like onEquip()/onUnequip()/onTick()/etc., henceforth is your ConcurrentModificationException.

I don't think this is entirely correct, considering this crash doesn't happen when you're using any of the other methods, such as onEquip(), which by your response, would also cause the crash since it modifies the list of available slot handlers. Here's an example of something that works perfectly fine (from a previously locked slot):
@Override public void onEquip(String identifier, int index, LivingEntity livingEntity) { CuriosApi.getSlotHelper().unlockSlotType("arrows", livingEntity); }

[Aizistral]

It does also crash, granted that the slot you are unlocking is not unlocked already and that it actually exists. onEquip() also isn't any different from onUnequip() in this context, since they are both called within same event handler when cycling through same list.

I specifically did testing to confirm that my mind does not deceive me. My gotcha code looked as following:

	@Override
	public void onEquip(String identifier, int index, LivingEntity entityLivingBase) {
		super.onEquip(identifier, index, entityLivingBase);

		System.out.println("Unlocking type!");
		CuriosApi.getSlotHelper().unlockSlotType("spellstone", entityLivingBase);
	}

What I got in log output:

...
[25Aug2020 15:45:17.868] [Server thread/INFO] [net.minecraft.server.MinecraftServer/]: Saving chunks for level 'ServerLevel[World #7161-JS73]'/minecraft:the_nether
[25Aug2020 15:45:17.870] [Server thread/INFO] [net.minecraft.server.MinecraftServer/]: Saving chunks for level 'ServerLevel[World #7161-JS73]'/minecraft:the_end
[25Aug2020 15:45:50.420] [Server thread/INFO] [STDOUT/]: [com.integral.enigmaticlegacy.items.EnderRing:onEquip:42]: Unlocking type!
[25Aug2020 15:45:51.518] [Server thread/INFO] [STDOUT/]: [com.integral.enigmaticlegacy.items.EnderRing:onEquip:42]: Unlocking type!
[25Aug2020 15:46:06.274] [Render thread/INFO] [net.minecraft.client.gui.NewChatGui/]: [CHAT] Unknown or incomplete command, see below for error
[25Aug2020 15:46:06.275] [Render thread/INFO] [net.minecraft.client.gui.NewChatGui/]: [CHAT] ...spellstone<--[HERE]
[25Aug2020 15:46:08.904] [Server thread/INFO] [net.minecraft.server.MinecraftServer/]: [Dev: Slot spellstone has been locked for Dev]
[25Aug2020 15:46:08.906] [Render thread/INFO] [net.minecraft.client.gui.NewChatGui/]: [CHAT] Slot spellstone has been locked for Dev
[25Aug2020 15:46:12.518] [Server thread/INFO] [STDOUT/]: [com.integral.enigmaticlegacy.items.EnderRing:onEquip:42]: Unlocking type!
[25Aug2020 15:46:12.520] [Server thread/ERROR] [net.minecraftforge.eventbus.EventBus/EVENTBUS]: Exception caught during firing event: null
	Index: 2
	Listeners:
		0: NORMAL
		1: ASM: com.integral.enigmaticlegacy.handlers.EnigmaticEventHandler@73f41231 onPlayerTick(Lnet/minecraftforge/event/entity/living/LivingEvent$LivingUpdateEvent;)V
		2: ASM: top.theillusivec4.curios.common.event.CuriosEventHandler@1272b52 tick(Lnet/minecraftforge/event/entity/living/LivingEvent$LivingUpdateEvent;)V
java.util.ConcurrentModificationException
	at java.util.LinkedHashMap$LinkedHashIterator.nextNode(LinkedHashMap.java:719)
	at java.util.LinkedHashMap$LinkedEntryIterator.next(LinkedHashMap.java:752)
	at java.util.LinkedHashMap$LinkedEntryIterator.next(LinkedHashMap.java:750)
	at java.util.Collections$UnmodifiableMap$UnmodifiableEntrySet$1.next(Collections.java:1661)
	at java.util.Collections$UnmodifiableMap$UnmodifiableEntrySet$1.next(Collections.java:1654)
	at top.theillusivec4.curios.common.event.CuriosEventHandler.lambda$tick$19(CuriosEventHandler.java:334)
	at net.minecraftforge.common.util.LazyOptional.ifPresent(LazyOptional.java:161)
	at top.theillusivec4.curios.common.event.CuriosEventHandler.tick(CuriosEventHandler.java:330)
...

It worked fine at first two equips since I already had slot unlocked, but as soon as I used Curios command to lock it back, I got exactly what I expected to behold.

The moral stays simple - don't chop a tree when you sit on top of it.

[TheIllusiveC4]

Extegral is correct in that this is an expected crash considering the context. However, I'm looking into the best viable solution for locking/unlocking slots based on equipment changes, which is the use-case that this issue stems from and the one that I'd like to resolve. If there's currently a workaround for that, I'd be interested to hear about it.

[Aizistral]

Well, under current circumstances this crash can be avoided by any sort of system that would schedule slot locking/unlocking to be executed later, outside of ICurio calls. The simplest implementation of it I can think of would look something like this:

1. Create a public static Map<PlayerEntity, String> somewhere;
2. When slot needs to be locked from your onEquip()/onUnequip()/whatever, access that map instead and put player object along with identifier of that slot in the map;
3. Register event handler for LivingUpdateEvent with lower priority than Curios' one, to ensure it will be called the same tick, but after Curios is done handling it's capability calls;
4. Use ticked entity object to check whether it is present in map, and if it is, then get identifier of slot to be locked and lock it. Don't forget to remove it from map once you're done, of course.

This is a specific example that takes into account only locking and assumes it is only required for players, and only one slot type will need to be locked per tick. But I suppose it is fairly understandable how it can be expanded to handle more complicated cases.