fix(D-01): atomic snapshot for registry to prevent torn reads (BUG-001)

Replace two separate volatile fields (DEFINITIONS, COMPONENT_HOLDERS)
with a single RegistrySnapshot record swapped atomically. This prevents
race conditions where a reader thread could see new definitions paired
with stale/empty component holders between the two volatile writes.
This commit is contained in:
NotEvil
2026-04-14 02:25:57 +02:00
parent dbacef66d5
commit 1327e3bfc3

View File

@@ -18,9 +18,9 @@ import org.jetbrains.annotations.Nullable;
* Thread-safe registry for data-driven bondage item definitions. * Thread-safe registry for data-driven bondage item definitions.
* *
* <p>Populated by the reload listener that scans {@code tiedup_items/} JSON files. * <p>Populated by the reload listener that scans {@code tiedup_items/} JSON files.
* Uses volatile atomic swap (same pattern as {@link * Uses a single volatile snapshot reference to ensure readers always see a
* com.tiedup.remake.client.animation.context.ContextGlbRegistry}) to ensure * consistent pair of definitions + component holders. This prevents torn reads
* the render thread always sees a consistent snapshot.</p> * where one map is updated but the other is stale.</p>
* *
* <p>Lookup methods accept either a {@link ResourceLocation} ID directly * <p>Lookup methods accept either a {@link ResourceLocation} ID directly
* or an {@link ItemStack} (reads the {@code tiedup_item_id} NBT tag).</p> * or an {@link ItemStack} (reads the {@code tiedup_item_id} NBT tag).</p>
@@ -31,20 +31,22 @@ public final class DataDrivenItemRegistry {
public static final String NBT_ITEM_ID = "tiedup_item_id"; public static final String NBT_ITEM_ID = "tiedup_item_id";
/** /**
* Volatile reference to an unmodifiable map. Reload builds a new map * Immutable snapshot combining definitions and their component holders.
* and swaps atomically; consumer threads always see a consistent snapshot. * Swapped atomically via a single volatile write to prevent torn reads.
*/ */
private static volatile Map< private record RegistrySnapshot(
ResourceLocation, Map<ResourceLocation, DataDrivenItemDefinition> definitions,
DataDrivenItemDefinition Map<ResourceLocation, ComponentHolder> holders
> DEFINITIONS = Map.of(); ) {
static final RegistrySnapshot EMPTY = new RegistrySnapshot(Map.of(), Map.of());
}
/** /**
* Parallel cache of instantiated component holders, keyed by item definition ID. * Single volatile reference to the current registry state.
* Rebuilt every time DEFINITIONS changes. * All read methods capture this reference ONCE at the start to ensure
* consistency within a single call.
*/ */
private static volatile Map<ResourceLocation, ComponentHolder> private static volatile RegistrySnapshot SNAPSHOT = RegistrySnapshot.EMPTY;
COMPONENT_HOLDERS = Map.of();
private DataDrivenItemRegistry() {} private DataDrivenItemRegistry() {}
@@ -57,8 +59,9 @@ public final class DataDrivenItemRegistry {
public static void reload( public static void reload(
Map<ResourceLocation, DataDrivenItemDefinition> newDefs Map<ResourceLocation, DataDrivenItemDefinition> newDefs
) { ) {
DEFINITIONS = Collections.unmodifiableMap(new HashMap<>(newDefs)); Map<ResourceLocation, DataDrivenItemDefinition> defs =
COMPONENT_HOLDERS = buildComponentHolders(DEFINITIONS); Collections.unmodifiableMap(new HashMap<>(newDefs));
SNAPSHOT = new RegistrySnapshot(defs, buildComponentHolders(defs));
} }
/** /**
@@ -67,7 +70,8 @@ public final class DataDrivenItemRegistry {
* <p>On an integrated server, both the client (assets/) and server (data/) reload * <p>On an integrated server, both the client (assets/) and server (data/) reload
* listeners populate this registry. Using {@link #reload} would cause the second * listeners populate this registry. Using {@link #reload} would cause the second
* listener to overwrite the first's definitions. This method builds a new map * listener to overwrite the first's definitions. This method builds a new map
* from the existing snapshot + the new entries, then swaps atomically.</p> * from the existing snapshot + the new entries, then swaps atomically as a
* single snapshot to prevent torn reads.</p>
* *
* @param newDefs the definitions to merge (will overwrite existing entries with same key) * @param newDefs the definitions to merge (will overwrite existing entries with same key)
*/ */
@@ -75,11 +79,12 @@ public final class DataDrivenItemRegistry {
Map<ResourceLocation, DataDrivenItemDefinition> newDefs Map<ResourceLocation, DataDrivenItemDefinition> newDefs
) { ) {
Map<ResourceLocation, DataDrivenItemDefinition> merged = new HashMap<>( Map<ResourceLocation, DataDrivenItemDefinition> merged = new HashMap<>(
DEFINITIONS SNAPSHOT.definitions
); );
merged.putAll(newDefs); merged.putAll(newDefs);
DEFINITIONS = Collections.unmodifiableMap(merged); Map<ResourceLocation, DataDrivenItemDefinition> defs =
COMPONENT_HOLDERS = buildComponentHolders(DEFINITIONS); Collections.unmodifiableMap(merged);
SNAPSHOT = new RegistrySnapshot(defs, buildComponentHolders(defs));
} }
/** /**
@@ -90,7 +95,7 @@ public final class DataDrivenItemRegistry {
*/ */
@Nullable @Nullable
public static DataDrivenItemDefinition get(ResourceLocation id) { public static DataDrivenItemDefinition get(ResourceLocation id) {
return DEFINITIONS.get(id); return SNAPSHOT.definitions.get(id);
} }
/** /**
@@ -108,7 +113,7 @@ public final class DataDrivenItemRegistry {
tag.getString(NBT_ITEM_ID) tag.getString(NBT_ITEM_ID)
); );
if (id == null) return null; if (id == null) return null;
return DEFINITIONS.get(id); return SNAPSHOT.definitions.get(id);
} }
/** /**
@@ -117,15 +122,14 @@ public final class DataDrivenItemRegistry {
* @return unmodifiable collection of all definitions * @return unmodifiable collection of all definitions
*/ */
public static Collection<DataDrivenItemDefinition> getAll() { public static Collection<DataDrivenItemDefinition> getAll() {
return DEFINITIONS.values(); return SNAPSHOT.definitions.values();
} }
/** /**
* Clear all definitions. Called on world unload or for testing. * Clear all definitions. Called on world unload or for testing.
*/ */
public static void clear() { public static void clear() {
DEFINITIONS = Map.of(); SNAPSHOT = RegistrySnapshot.EMPTY;
COMPONENT_HOLDERS = Map.of();
} }
// ===== COMPONENT HOLDERS ===== // ===== COMPONENT HOLDERS =====
@@ -133,14 +137,27 @@ public final class DataDrivenItemRegistry {
/** /**
* Get the ComponentHolder for a data-driven item stack. * Get the ComponentHolder for a data-driven item stack.
* *
* <p>Captures the snapshot reference once to ensure consistent reads
* between the definition lookup and the holder lookup.</p>
*
* @param stack the ItemStack to inspect * @param stack the ItemStack to inspect
* @return the holder, or null if the stack is not data-driven or has no definition * @return the holder, or null if the stack is not data-driven or has no definition
*/ */
@Nullable @Nullable
public static ComponentHolder getComponents(ItemStack stack) { public static ComponentHolder getComponents(ItemStack stack) {
DataDrivenItemDefinition def = get(stack); if (stack.isEmpty()) return null;
CompoundTag tag = stack.getTag();
if (tag == null || !tag.contains(NBT_ITEM_ID)) return null;
ResourceLocation id = ResourceLocation.tryParse(
tag.getString(NBT_ITEM_ID)
);
if (id == null) return null;
// Capture snapshot once to ensure definition and holder come from
// the same atomic snapshot, preventing torn reads.
RegistrySnapshot snap = SNAPSHOT;
DataDrivenItemDefinition def = snap.definitions.get(id);
if (def == null) return null; if (def == null) return null;
return COMPONENT_HOLDERS.get(def.id()); return snap.holders.get(def.id());
} }
/** /**
@@ -151,7 +168,7 @@ public final class DataDrivenItemRegistry {
*/ */
@Nullable @Nullable
public static ComponentHolder getComponents(ResourceLocation id) { public static ComponentHolder getComponents(ResourceLocation id) {
return COMPONENT_HOLDERS.get(id); return SNAPSHOT.holders.get(id);
} }
/** /**