From 1327e3bfc3b0edc8bd0b1fcbe2c9e260682d0fc9 Mon Sep 17 00:00:00 2001 From: NotEvil Date: Tue, 14 Apr 2026 02:25:57 +0200 Subject: [PATCH] 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. --- .../datadriven/DataDrivenItemRegistry.java | 71 ++++++++++++------- 1 file changed, 44 insertions(+), 27 deletions(-) diff --git a/src/main/java/com/tiedup/remake/v2/bondage/datadriven/DataDrivenItemRegistry.java b/src/main/java/com/tiedup/remake/v2/bondage/datadriven/DataDrivenItemRegistry.java index 070ef7f..e0e25ec 100644 --- a/src/main/java/com/tiedup/remake/v2/bondage/datadriven/DataDrivenItemRegistry.java +++ b/src/main/java/com/tiedup/remake/v2/bondage/datadriven/DataDrivenItemRegistry.java @@ -18,9 +18,9 @@ import org.jetbrains.annotations.Nullable; * Thread-safe registry for data-driven bondage item definitions. * *

Populated by the reload listener that scans {@code tiedup_items/} JSON files. - * Uses volatile atomic swap (same pattern as {@link - * com.tiedup.remake.client.animation.context.ContextGlbRegistry}) to ensure - * the render thread always sees a consistent snapshot.

+ * Uses a single volatile snapshot reference to ensure readers always see a + * consistent pair of definitions + component holders. This prevents torn reads + * where one map is updated but the other is stale.

* *

Lookup methods accept either a {@link ResourceLocation} ID directly * or an {@link ItemStack} (reads the {@code tiedup_item_id} NBT tag).

@@ -31,20 +31,22 @@ public final class DataDrivenItemRegistry { public static final String NBT_ITEM_ID = "tiedup_item_id"; /** - * Volatile reference to an unmodifiable map. Reload builds a new map - * and swaps atomically; consumer threads always see a consistent snapshot. + * Immutable snapshot combining definitions and their component holders. + * Swapped atomically via a single volatile write to prevent torn reads. */ - private static volatile Map< - ResourceLocation, - DataDrivenItemDefinition - > DEFINITIONS = Map.of(); + private record RegistrySnapshot( + Map definitions, + Map holders + ) { + static final RegistrySnapshot EMPTY = new RegistrySnapshot(Map.of(), Map.of()); + } /** - * Parallel cache of instantiated component holders, keyed by item definition ID. - * Rebuilt every time DEFINITIONS changes. + * Single volatile reference to the current registry state. + * All read methods capture this reference ONCE at the start to ensure + * consistency within a single call. */ - private static volatile Map - COMPONENT_HOLDERS = Map.of(); + private static volatile RegistrySnapshot SNAPSHOT = RegistrySnapshot.EMPTY; private DataDrivenItemRegistry() {} @@ -57,8 +59,9 @@ public final class DataDrivenItemRegistry { public static void reload( Map newDefs ) { - DEFINITIONS = Collections.unmodifiableMap(new HashMap<>(newDefs)); - COMPONENT_HOLDERS = buildComponentHolders(DEFINITIONS); + Map defs = + Collections.unmodifiableMap(new HashMap<>(newDefs)); + SNAPSHOT = new RegistrySnapshot(defs, buildComponentHolders(defs)); } /** @@ -67,7 +70,8 @@ public final class DataDrivenItemRegistry { *

On an integrated server, both the client (assets/) and server (data/) reload * listeners populate this registry. Using {@link #reload} would cause the second * listener to overwrite the first's definitions. This method builds a new map - * from the existing snapshot + the new entries, then swaps atomically.

+ * from the existing snapshot + the new entries, then swaps atomically as a + * single snapshot to prevent torn reads.

* * @param newDefs the definitions to merge (will overwrite existing entries with same key) */ @@ -75,11 +79,12 @@ public final class DataDrivenItemRegistry { Map newDefs ) { Map merged = new HashMap<>( - DEFINITIONS + SNAPSHOT.definitions ); merged.putAll(newDefs); - DEFINITIONS = Collections.unmodifiableMap(merged); - COMPONENT_HOLDERS = buildComponentHolders(DEFINITIONS); + Map defs = + Collections.unmodifiableMap(merged); + SNAPSHOT = new RegistrySnapshot(defs, buildComponentHolders(defs)); } /** @@ -90,7 +95,7 @@ public final class DataDrivenItemRegistry { */ @Nullable 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) ); 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 */ public static Collection getAll() { - return DEFINITIONS.values(); + return SNAPSHOT.definitions.values(); } /** * Clear all definitions. Called on world unload or for testing. */ public static void clear() { - DEFINITIONS = Map.of(); - COMPONENT_HOLDERS = Map.of(); + SNAPSHOT = RegistrySnapshot.EMPTY; } // ===== COMPONENT HOLDERS ===== @@ -133,14 +137,27 @@ public final class DataDrivenItemRegistry { /** * Get the ComponentHolder for a data-driven item stack. * + *

Captures the snapshot reference once to ensure consistent reads + * between the definition lookup and the holder lookup.

+ * * @param stack the ItemStack to inspect * @return the holder, or null if the stack is not data-driven or has no definition */ @Nullable 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; - return COMPONENT_HOLDERS.get(def.id()); + return snap.holders.get(def.id()); } /** @@ -151,7 +168,7 @@ public final class DataDrivenItemRegistry { */ @Nullable public static ComponentHolder getComponents(ResourceLocation id) { - return COMPONENT_HOLDERS.get(id); + return SNAPSHOT.holders.get(id); } /**