Race Condition when used in combination with C2ME #33
Comments
|
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 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: Benefits
Potential drawbacks
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. Note on Forge's caseIt’s generally noted that If there is no actual need for synchronization (i.e., there is no risk of concurrent access conflicts), then using 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. VariantNbt UtilityIt may also be helpful if you created a separate class within your utilities, e.g. 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 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());
}
}
}Example Usage:The @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 Note: The (Object)this casting is used to adapt the entity instance to the Important!If you wish to use the
// 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
}
});
}));
// 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);
}
}
});
}));Difference between
|
Repository owner
deleted a comment from
fionera
May 20, 2024
|
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 |
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? |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
The text was updated successfully, but these errors were encountered: