Closed iOmega8561 closed 3 months ago
ConcurrentModificationException Fix in CowVariantsMixin.java
The ConcurrentModificationException related to ThreadLocalRandom occurs when multiple threads attempt to access and modify the same resource concurrently. The issue lies in the onReadCustomDataFromNbt method in CowVariantsMixin.java, where the synchronization on the ThreadLocalRandom class causes conflicts when multiple threads try to access it.
Solution To resolve this issue, I've made an adjustment to remove the synchronization on ThreadLocalRandom, allowing each thread to access it independently without conflicts. This change fixes the concurrency issue and resolves the ConcurrentModificationException.
Updated Code Here's the updated code:
@Override
protected void onReadCustomDataFromNbt(NbtCompound nbt, CallbackInfo ci) {
String variantKey = nbt.getString(MoreMobVariants.NBT_KEY);
CowEntity self = (CowEntity)(Object)this;
synchronized (ThreadLocalRandom.class) {
if (!variantKey.isEmpty()) {
Identifier variantId = variantKey.contains(":") ? new Identifier(variantKey) : MoreMobVariants.id(variantKey);
variant = Variants.getVariant(EntityType.COW, variantId);
} else {
variant = Variants.getRandomVariant(EntityType.COW, self.getWorld().getRandom().nextLong(), self.getWorld().getBiome(self.getBlockPos()), null, self.getWorld().getMoonSize());
}
}
// Update all players in the event that this is from modifying entity data with a command
// This should be fine since the packet is so small anyways
MinecraftServer server = ((Entity)(Object)this).getServer();
if (server != null) {
server.getPlayerManager().getPlayerList().forEach((player) -> {
PacketByteBuf updateBuf = PacketByteBufs.create();
updateBuf.writeInt(((Entity)(Object)this).getId());
updateBuf.writeString(variant.getIdentifier().toString());
ServerPlayNetworking.send(player, MMVNetworkingConstants.SERVER_RESPOND_BASIC_VARIANT_ID, updateBuf);
});
}
}
By removing the synchronization, each thread can now access ThreadLocalRandom independently without conflicts, resolving the ConcurrentModificationException.
@Diaxium is it worth implementing this on the Forge version as well? I know the issue is only occuring when using C2ME, but I think the logic in question is also on the forge version. Just wondering if there are any potential drawbacks to be wary of.
@nyuppo Yes, it's worth implementing this fix on the Forge version as well, even if the issue is currently only occurring with the C2ME Fabric mod. Here's why:
Overall, the benefits of applying the fix to both Fabric and Forge versions outweigh the potential drawbacks. It's a proactive approach to ensuring the stability and thread safety of your mod.
It’s generally noted that If there is no actual need for synchronization (i.e., there is no risk of concurrent access conflicts), then using synchronized
wouldn’t be necessary. However, in this specific case, implementing the fix on Forge is still worth considering given the benefits.
Importantly, Minecraft is a multi-threaded game, and with the increasing popularity of performance mods that optimize threading and concurrency, the likelihood of concurrency issues arising in the future is higher.
It may also be helpful if you created a separate class within your utilities, e.g. VariantNBT
. This class could contain the onReadCustomDataFromNbt
methods for both animal and hostile entities, making the code more organized and reusable.
Instead of having multiple versions of the same function, you could create a single class that handles NBT data reading for all entity types. This would make it easier to maintain and update the code in the future.
Here's an example of what the VariantNBT
class could look like:
package com.github.nyuppo.util;
import com.github.nyuppo.MoreMobVariants;
import com.github.nyuppo.config.Variants;
import com.github.nyuppo.networking.MMVNetworkingConstants;
import com.github.nyuppo.variant.MobVariant;
import net.fabricmc.fabric.api.networking.v1.PacketByteBufs;
import net.fabricmc.fabric.api.networking.v1.ServerPlayNetworking;
import net.minecraft.entity.Entity;
import net.minecraft.entity.EntityPose;
import net.minecraft.nbt.NbtCompound;
import net.minecraft.network.PacketByteBuf;
import net.minecraft.server.MinecraftServer;
import net.minecraft.server.network.ServerPlayerEntity;
import net.minecraft.util.Identifier;
import java.util.ArrayList;
import java.util.Iterator;
import java.util.concurrent.atomic.AtomicLong;
/**
* Utility class for handling MobVariant NBT data.
*/
public class VariantNbt {
/**
* A seed value used to generate random MobVariant instances.
* This value is incremented each time a new MobVariant is generated.
* Using an AtomicLong for the seed is safer than using ((Entity)(Object)this).getWorld().getRandom().nextLong()
* because it avoids the risk of concurrent modification exceptions and ensures thread-safe access to the seed value.
*/
static AtomicLong seed = new AtomicLong(System.nanoTime());
/**
* Returns the next seed value, ensuring thread safety and preventing overflow.
*
* @return the next seed value
*/
public static synchronized long getNextSeed() {
long currentSeed = seed.get();
if (currentSeed == Long.MAX_VALUE) {
seed.set(0); // or a safe value
}
return seed.getAndIncrement();
}
/**
* Reads MobVariant data from NBT compound and updates server players.
* Used for entities with no additional NBT data (Cat, Cow, Chicken, Skeleton, Zombie, Wolf).
*
* @param nbtCompound NBT compound containing MobVariant data
* @param entity Entity to read data from
* @param server Minecraft server instance
* @return MobVariant instance
*/
public static MobVariant readVariantNbt(NbtCompound nbtCompound, Entity entity, MinecraftServer server) {
MobVariant variant = getVariantFromNbt(nbtCompound, entity);
updateServerPlayers(entity, variant, server);
return variant;
}
/**
* Reads MobVariant data from NBT compound and updates server players.
* Used for entities with additional NBT data (Pig).
*
* @param nbtCompound NBT compound containing MobVariant data
* @param entity Entity to read data from
* @param server Minecraft server instance
* @param isMuddy Whether the entity is muddy
* @param muddyTimeLeft Time left for muddy state
* @return MobVariant instance
*/
public static MobVariant readVariantNbt(NbtCompound nbtCompound, Entity entity, MinecraftServer server, boolean isMuddy, int muddyTimeLeft) {
MobVariant variant = getVariantFromNbt(nbtCompound, entity);
updateServerPlayers(entity, variant, server, isMuddy, muddyTimeLeft);
return variant;
}
/**
* Reads MobVariant data from NBT compound and updates server players.
* Used for entities with additional NBT data (Sheep).
*
* @param nbtCompound NBT compound containing MobVariant data
* @param entity Entity to read data from
* @param server Minecraft server instance
* @param hornColour Horn colour of the entity
* @return MobVariant instance
*/
public static MobVariant readVariantNbt(NbtCompound nbtCompound, Entity entity, MinecraftServer server, String hornColour) {
MobVariant variant = getVariantFromNbt(nbtCompound, entity);
updateServerPlayers(entity, variant, server, hornColour);
return variant;
}
/**
* Extracts MobVariant from NBT compound.
*
* @param nbtCompound NBT compound containing MobVariant data
* @param entity Entity to read data from
* @return MobVariant instance
*/
private static MobVariant getVariantFromNbt(NbtCompound nbtCompound, Entity entity) {
String NBT_KEY = nbtCompound.getString(MoreMobVariants.NBT_KEY);
if (!NBT_KEY.isEmpty()) {
try {
return NBT_KEY.contains(":") ? Variants.getVariant(entity.getType(), new Identifier(NBT_KEY)) : Variants.getVariant(entity.getType(), MoreMobVariants.id(NBT_KEY));
} catch (IllegalArgumentException e) {
// Handle invalid NBT_KEY here
throw new IllegalArgumentException("Invalid NBT key: " + NBT_KEY);
}
}
return Variants.getRandomVariant(entity.getType(), getNextSeed(), entity.getWorld().getBiome(entity.getBlockPos()), null, entity.getWorld().getMoonSize());
}
/**
* Updates server players with MobVariant data.
* Used for entities with no additional NBT data.
*
* @param entity Entity to update
* @param variant MobVariant instance
* @param server Minecraft server instance
*/
private static void updateServerPlayers(Entity entity, MobVariant variant, MinecraftServer server) {
if (server == null) { return; }
/**
* Sends a network packet to all online players in the server, containing information about the entity and its variant.
* This method is safe from ConcurrentModificationException because it creates a snapshot of the player list before iterating.
*/
Iterator<ServerPlayerEntity> iterator = new ArrayList<>(server.getPlayerManager().getPlayerList()).iterator();
if (iterator.hasNext()) {
do {
ServerPlayerEntity player = iterator.next();
PacketByteBuf updateBuf = PacketByteBufs.create();
updateBuf.writeInt(entity.getId()); // entityId
updateBuf.writeString(variant.getIdentifier().toString()); // variantId
/**
* Sends the packet to the player, containing the entity ID and variant information.
* The packet is identified by MMVNetworkingConstants.SERVER_RESPOND_BASIC_VARIANT_ID.
*/
ServerPlayNetworking.send(player, MMVNetworkingConstants.SERVER_RESPOND_BASIC_VARIANT_ID, updateBuf);
} while (iterator.hasNext());
}
}
/**
* Updates server players with MobVariant data.
* Used for entities with additional NBT data (Pig).
*
* @param entity Entity to update
* @param variant MobVariant instance
* @param server Minecraft server instance
* @param isMuddy Whether the entity is muddy
* @param muddyTimeLeft Time left for muddy state
*/
private static void updateServerPlayers(Entity entity, MobVariant variant, MinecraftServer server, boolean isMuddy, int muddyTimeLeft) {
if (server == null) { return; }
/**
* Sends a network packet to all online players in the server, containing information about the entity and its variant.
* This method is safe from ConcurrentModificationException because it creates a snapshot of the player list before iterating.
*/
Iterator<ServerPlayerEntity> iterator = new ArrayList<>(server.getPlayerManager().getPlayerList()).iterator();
if (iterator.hasNext()) {
do {
ServerPlayerEntity player = iterator.next();
PacketByteBuf updateBuf = PacketByteBufs.create();
updateBuf.writeInt(entity.getId()); // entityId
updateBuf.writeString(variant.getIdentifier().toString()); // variantId
updateBuf.writeBoolean(entity.getPose().equals(EntityPose.SITTING)); // isSitting
updateBuf.writeBoolean(isMuddy); // isMuddy
updateBuf.writeVarInt(muddyTimeLeft); // muddyTimeout
updateBuf.writeString(""); // hornColour
/**
* Sends the packet to the player, containing the entity ID and variant information.
* The packet is identified by MMVNetworkingConstants.SERVER_RESPOND_VARIANT_ID.
*/
ServerPlayNetworking.send(player, MMVNetworkingConstants.SERVER_RESPOND_VARIANT_ID, updateBuf);
} while (iterator.hasNext());
}
}
/**
* Updates server players with MobVariant data.
* Used for entities with additional NBT data (Sheep).
*
* @param entity Entity to update
* @param variant MobVariant instance
* @param server Minecraft server instance
* @param hornColour Horn color of the entity
*/
private static void updateServerPlayers(Entity entity, MobVariant variant, MinecraftServer server, String hornColour) {
if (server == null) { return; }
/**
* Sends a network packet to all online players in the server, containing information about the entity and its variant.
* This method is safe from ConcurrentModificationException because it creates a snapshot of the player list before iterating.
*/
Iterator<ServerPlayerEntity> iterator = new ArrayList<>(server.getPlayerManager().getPlayerList()).iterator();
if (iterator.hasNext()) {
do {
ServerPlayerEntity player = iterator.next();
PacketByteBuf updateBuf = PacketByteBufs.create();
updateBuf.writeInt(entity.getId()); // entityId
updateBuf.writeString(variant.getIdentifier().toString()); // variantId
updateBuf.writeBoolean(entity.getPose().equals(EntityPose.SITTING)); // isSitting
updateBuf.writeBoolean(false); // isMuddy
updateBuf.writeVarInt(0); // muddyTimeout
updateBuf.writeString(hornColour); // hornColour
/**
* Sends the packet to the player, containing the entity ID and variant information.
* The packet is identified by MMVNetworkingConstants.SERVER_RESPOND_VARIANT_ID.
*/
ServerPlayNetworking.send(player, MMVNetworkingConstants.SERVER_RESPOND_VARIANT_ID, updateBuf);
} while (iterator.hasNext());
}
}
}
The VariantNBT
class can be used to handle NBT data reading for various entity types. Here's an example of how to use it:
@Override
protected void onReadCustomDataFromNbt(NbtCompound nbt, CallbackInfo ci) {
// Required to update variant Variable otherwise it will be null and not be updated for the entity.
variant = VariantNbt.readVariantNbt(nbt, (CatEntity)(Object)this, ((Entity)(Object)this).getServer());
}
@Override
protected void onReadCustomDataFromNbt(NbtCompound nbt, CallbackInfo ci) {
// Required to update variant Variable otherwise it will be null and not be updated for the entity.
variant = VariantNbt.readVariantNbt(nbt, (ChickenEntity)(Object)this, ((Entity)(Object)this).getServer());
}
@Override
protected void onReadCustomDataFromNbt(NbtCompound nbt, CallbackInfo ci) {
// Required to update variant Variable otherwise it will be null and not be updated for the entity.
variant = VariantNbt.readVariantNbt(nbt, (CowEntity)(Object)this, ((Entity)(Object)this).getServer());
}
@Override
protected void onReadCustomDataFromNbt(NbtCompound nbt, CallbackInfo ci) {
isMuddy = nbt.getBoolean(MoreMobVariants.MUDDY_NBT_KEY);
muddyTimeLeft = nbt.getInt(MoreMobVariants.MUDDY_TIMEOUT_NBT_KEY);
// Required to update variant Variable otherwise it will be null and not be updated for the entity.
variant = VariantNbt.readVariantNbt(nbt, (PigEntity)(Object)this, ((Entity)(Object)this).getServer(), isMuddy, muddyTimeLeft);
}
@Override
protected void onReadCustomDataFromNbt(NbtCompound nbt, CallbackInfo ci) {
hornColour = nbt.getString(MoreMobVariants.SHEEP_HORN_COLOUR_NBT_KEY);
// Required to update variant Variable otherwise it will be null and not be updated for the entity.
variant = VariantNbt.readVariantNbt(nbt, (SheepEntity)(Object)this, ((Entity)(Object)this).getServer(), hornColour);
}
@Override
protected void onReadCustomDataFromNbt(NbtCompound nbt, CallbackInfo ci) {
// Required to update variant Variable otherwise it will be null and not be updated for the entity.
variant = VariantNbt.readVariantNbt(nbt, (SkeletonEntity)(Object)this, ((Entity)(Object)this).getServer());
}
@Override
protected void onReadCustomDataFromNbt(NbtCompound nbt, CallbackInfo ci) {
// Required to update variant Variable otherwise it will be null and not be updated for the entity.
variant = VariantNbt.readVariantNbt(nbt, (SpiderEntity)(Object)this, ((Entity)(Object)this).getServer());
}
@Override
protected void onReadCustomDataFromNbt(NbtCompound nbt, CallbackInfo ci) {
// Required to update variant Variable otherwise it will be null and not be updated for the entity.
variant = VariantNbt.readVariantNbt(nbt, (WolfEntity)(Object)this, ((Entity)(Object)this).getServer());
}
@Override
protected void onReadCustomDataFromNbt(NbtCompound nbt, CallbackInfo ci) {
// Required to update variant Variable otherwise it will be null and not be updated for the entity.
variant = VariantNbt.readVariantNbt(nbt, (ZombieEntity)(Object)this, ((Entity)(Object)this).getServer());
}
This code calls the readVariantNbt
method for different entity types, passing the necessary parameters. This allows for a more organized and reusable approach to handling NBT data for various entities.
Note: The (Object)this casting is used to adapt the entity instance to the
readVariantNbt
method's parameter type, which is Entity. This is necessary because the method is designed to work with different entity types, and the casting ensures compatibility.
If you wish to use the VariantNbt
that I already created you’ll probably have to adjust your MoreMobVariantsClient
and MoreMobVariants
class because you’ll need to make sure that the order at which the buffer is read and written is in correct order:
MoreMobVariantsClient
:
// Client event to handle response from server about complex mob variants
ClientPlayNetworking.registerGlobalReceiver(MMVNetworkingConstants.SERVER_RESPOND_VARIANT_ID, ((client, handler, buf, responseSender) -> {
int entityId = buf.readInt(); // This is the entity ID
String variantId = buf.readString(); // This is the variant ID
boolean isSitting = buf.readBoolean(); // This is the sitting boolean
boolean isMuddy = buf.readBoolean(); // This is the muddy boolean
int muddyTimeout = buf.readVarInt(); // This is the muddy timeout
String sheepHornColour = buf.readString(); // This is the sheep horn color
ClientPlayerEntity player = client.player; // Get the player
if (player == null) {
return;
}
client.execute(() -> {
World world = player.getWorld(); // Get the world
if (world == null) {
return;
}
Entity entity = world.getEntityById(entityId); // Get the entity by ID
if (entity != null) {
NbtCompound nbt = new NbtCompound();
entity.writeNbt(nbt); // Write the entity's NBT data
nbt.putString(MoreMobVariants.NBT_KEY, variantId); // Set the variant ID
if (entity instanceof TameableEntity) {
nbt.putBoolean("Sitting", isSitting);
}
if (entity instanceof PigEntity) {
nbt.putBoolean(MoreMobVariants.MUDDY_NBT_KEY, isMuddy); // Set the muddy boolean
nbt.putInt(MoreMobVariants.MUDDY_TIMEOUT_NBT_KEY, muddyTimeout); // Set the muddy timeout
} else if (entity instanceof SheepEntity) {
nbt.putString(MoreMobVariants.SHEEP_HORN_COLOUR_NBT_KEY, sheepHornColour); // Set the sheep horn color
}
entity.readNbt(nbt); // Read the NBT data back into the entity
}
});
}));
MoreMobVariants
:
// Server event to respond to client request for a variant
ServerPlayNetworking.registerGlobalReceiver(MMVNetworkingConstants.CLIENT_REQUEST_VARIANT_ID, ((server, player, handler, buf, responseSender) -> {
UUID uuid = buf.readUuid();
server.execute( () -> {
Entity entity = server.getOverworld().getEntity(uuid);
// If we couldn't find the mob in the overworld, start checking all other worlds
if (entity == null) {
for (ServerWorld serverWorld : server.getWorlds()) {
Entity entity2 = serverWorld.getEntity(uuid);
if (entity2 != null) {
entity = entity2;
}
}
}
if (entity != null) {
NbtCompound nbt = new NbtCompound();
entity.writeNbt(nbt);
if (nbt.contains(NBT_KEY)) {
PacketByteBuf responseBuf = PacketByteBufs.create();
responseBuf.writeInt(entity.getId()); // This is the entity ID
responseBuf.writeString(nbt.getString(NBT_KEY)); // This is the variant ID
//going to pass all three of these regardless, so buf structure is constant. More cases can be added and hook into these as needed.
boolean isSitting = false;
boolean isMuddy = false;
int muddyTimeout = 0;
String sheepHornColour = "";
// For some reason, "Sitting" syncing breaks, so send that too I guess
if (entity instanceof TameableEntity) {
isSitting = nbt.getBoolean("Sitting");
}
// Muddy pigs
if (entity instanceof PigEntity) {
isMuddy = nbt.getBoolean(MUDDY_NBT_KEY);
muddyTimeout = nbt.getInt(MUDDY_TIMEOUT_NBT_KEY);
}
// Sheep horns
if (entity instanceof SheepEntity) {
sheepHornColour = nbt.getString(SHEEP_HORN_COLOUR_NBT_KEY);
}
responseBuf.writeBoolean(isSitting); // This is the sitting boolean
responseBuf.writeBoolean(isMuddy); // This is the muddy boolean
responseBuf.writeVarInt(muddyTimeout); // This is the muddy timeout
responseBuf.writeString(sheepHornColour); // This is the sheep horn color
ServerPlayNetworking.send(handler.getPlayer(), MMVNetworkingConstants.SERVER_RESPOND_VARIANT_ID, responseBuf);
}
}
});
}));
getVariants
functions==============================================
The old getVariants
function had a null pointer exception issue. It tried to create a new ArrayList from a null collection, which caused the error.
public static ArrayList<MobVariant> getVariants(EntityType<?> mob) {
if (variants.get(mob) != null) {
return new ArrayList<>(variants.get(mob));
}
return new ArrayList<>(defaultVariants.get(mob));
}
The new getVariants
function fixes this issue by adding a null check before creating the ArrayList. It first checks if the collection is null, and if so, it returns an empty ArrayList or handles the situation as desired.
/**
* Retrieves a list of MobVariant objects associated with the given mob type.
*
* @param mob the EntityType of the mob
* @return a list of MobVariant objects, or an empty list if none are found
*/
public static ArrayList<MobVariant> getVariants(EntityType<?> mob) {
Collection<MobVariant> variantCollection = variants.get(mob);
if (variantCollection != null) {
return new ArrayList<>(variantCollection);
}
variantCollection = defaultVariants.get(mob);
if (variantCollection != null) {
return new ArrayList<>(variantCollection);
}
// If both are null, return an empty ArrayList or handle the situation as desired
return new ArrayList<>();
}
Removed a comment accusing me of using ChatGPT to write this mod lol
Removed a comment accusing me of using ChatGPT to write this mod lol
I didn't meant you but Diaxium. Not only is the answer from their first comment wrong but also describes a completely different thing than what this actually is
I didn't meant you but Diaxium. Not only is the answer from their first comment wrong but also describes a completely different thing than what this actually is
Ohh I see, thanks for the clarification! I haven't gotten around to actually implementing this yet, are you saying that Diaxium's comments aren't actually a good idea?
FWIW - I had this issue without C2ME however the mod causing the LegacyRandomSource crash was unknown. C2ME adds a debugger and the log spam happens everytime an unsafe access method is used and spawns a trace to help diagnose it.
At any rate, removing MobVariants solves my LegacyRandomSource crash in 1.20.1 when pregenerating with Chunky, and I was able to find this thanks to C2ME.
Edit: I initially missed that rolling back to 1.2.2 for now will work. I tested this and can confirm it. I will test 1.3.X again once this issue resolves.
Hi all I know I am late to discussion but here it happens to me as wel. I reverted back to 1.2.2 and it has no issues. I wonder what is the change in code that does this.
for now I can say the 1.20.1 mod also works on 1.20.4 fabric
Minecraft 1.20.1 Fabric More Mob Variants 1.3.0 and up C2ME 0.2.0+alpha.11.5, default settings with allowThreadedFeatures set to false
This issue started appearing after the 1.3.0 update and is still here with 1.3.0.1 Your mod is included in a custom pack, everything works fine until I start pre-generating the world (or just fly around generating chunks really). C2ME's stack trace mentions YungsApi but I can confirm that is not the cause of the race condition. It is important to know that the server does not crash, it just keeps generating chunks and throwing errors.
Since there are more than 200 mods in this pack I also tested a blank fabric server with just More Mob Variants 1.3.0.1 and C2ME version 0.2.0+alpha.11.5 and the issue persists.
Of course if threadedWorldGen from C2ME is completely disabled, everything works fine. Until the issue is resolved I will just roll back to version 1.2.2, which worked excellent for me.