ConcurrentModificationException caused by Rosehip Syrup leads to server crash

[Nikolay-Sitnikov]

Your GTNH Discord Username

nicksitnikov

Your Pack Version

2.6.1

Your Server

private server & SP

Java Version

Java 21

Type of Server

Forge

Your Actions

Here's a simple way to cause the crash: Gain 2 different warp effects: /effect [username] 39 60 2 /effect [username] 41 60 2 (Blurred Vision and Thaumarrhea, respectively) Drink a Rosehip Syrup.

Crash Report

https://mclo.gs/xiGCwfO latest.log crash-2024-06-07_17.30.44-server.txt

Final Checklist

• [X] I have searched this issue tracker and there is nothing similar already. Posting on a closed issue saying the crash still exists will prompt us to investigate and reopen it once we confirm your report.
• [X] I can reproduce this crash consistently by following the exact steps I described above.
• [X] I have asked other people and they confirm they also crash by following the exact steps I described above.

[github-actions[bot]]

Found 1 linked crash report(s)

Primitive Automated Analysis of Crash Report

https://mclo.gs/xiGCwfO

Stacktrace

java.util.ConcurrentModificationException
  at java.util.HashMap$HashIterator.nextNode(HashMap.java:1605)
  at java.util.HashMap$ValueIterator.next(HashMap.java:1633)
  at tb.common.item.ItemRosehipSyrup.onEaten(ItemRosehipSyrup.java:48)
  at net.minecraft.item.ItemStack.onFoodEaten(ItemStack.java:151)
  at net.minecraft.entity.player.EntityPlayer.onItemUseFinish(EntityPlayer.java:419)
  at net.minecraft.entity.player.EntityPlayerMP.onItemUseFinish(EntityPlayerMP.java:866)
  at net.minecraft.entity.player.EntityPlayer.onUpdate(EntityPlayer.java:242)
  at net.minecraft.entity.player.EntityPlayerMP.localOnUpdateEntity(EntityPlayerMP.java:295)
  at api.player.server.ServerPlayerAPI.onUpdateEntity(ServerPlayerAPI.java:5932)
  at net.minecraft.entity.player.EntityPlayerMP.onUpdateEntity(EntityPlayerMP.java)
  at net.minecraft.network.NetHandlerPlayServer.processPlayer(NetHandlerPlayServer.java:303)
  at net.minecraft.network.play.client.C03PacketPlayer.processPacket(SourceFile:137)
  at net.minecraft.network.play.client.C03PacketPlayer.processPacket(SourceFile:8)
  at net.minecraft.network.NetworkManager.processReceivedPackets(NetworkManager.java:212)
  at net.minecraft.network.NetworkSystem.networkTick(NetworkSystem.java:165)
  at net.minecraft.server.MinecraftServer.updateTimeLightAndEntities(MinecraftServer.java:659)
  at net.minecraft.server.MinecraftServer.tick(MinecraftServer.java:547)
  at net.minecraft.server.integrated.IntegratedServer.tick(IntegratedServer.java:111)
  at net.minecraft.server.MinecraftServer.run(MinecraftServer.java:427)
  at net.minecraft.server.MinecraftServer$2.run(MinecraftServer.java:685)

Missing 3 mods

• +unimixins-all-1.7.10-0.1.17.jar
• TwilightForest-2.5.25.jar
• CoreTweaks-1.7.10-0.3.3.2+nomixin.jar

Added 3 mods

• CoreTweaks-1.7.10-0.3.3.2-nomixin.jar (CoreTweaks)
• TwilightForest-2.6.12.jar (The Twilight Forest)
• -unimixins-all-1.7.10-0.1.17.jar (UniMixins: MixinExtras)

[Nikolay-Sitnikov]

An analysis of the crash: This is caused by the Rosehip Syrup iterating over the list of Potion effects while simultaneously updating it. Specifically, line 47 of ItemRosehipSyrup.java iterates over every effect and calls processPotion for each potion effect (while still iterating.)

Collection<PotionEffect> c = player.getActivePotionEffects();
Iterator<PotionEffect> $i = c.iterator();
ArrayList<Integer> removeEffects = new ArrayList<Integer>();
while ($i.hasNext()) {
    PotionEffect effect = $i.next();
    int i = processPotion(player, effect);
    if (i != -1) removeEffects.add(i);
}

processPotion checks if a potion effect is a warp effect with a level greater than 0 and, if so, calls reflectPotionEffect

if (canDecreaseLevel(Potion.potionTypes[effect.getPotionID()])) {
    if (effect.getAmplifier() == 0) return effect.getPotionID();
    else {
        reflectPotionEffect(p, effect);
        return -1;
    }
}

(canDecreaseLevel checks if the ID of the effect matches one of the warp effect IDs - it also has a theoretically possible NullPointerException.)

reflectPotionEffect then modifies the set of potion effects.

public static void reflectPotionEffect(EntityPlayer p, PotionEffect effect) {
    int amp = effect.getAmplifier() - 1;
    int id = effect.getPotionID();
    int dur = effect.getDuration();
    boolean transparent = effect.getIsAmbient();
    p.removePotionEffect(effect.getPotionID());
    p.addPotionEffect(new PotionEffect(id, dur, amp, transparent));
}

The reason this error isn't triggered every time is because hasNext() doesn't check for concurrent modifications, so if the potion effect was the last one in the collection, it will exit the loop without ever calling next() (which is the method that check for concurrent modification)

[Nikolay-Sitnikov]

A better way would be to have the loop that is responsible for removing the effects do the check and call reflectPotionEffect instead of the first iterating loop calling reflectPotionEffect.

(Also, the Rosehip Syrup doesn't affect Sun Scorned. That may have just been forgotten, or it may be intentional (but I think it was just forgotten.)