From ddaa25b97117b5e899eb9b5b65e5939c8af2f284 Mon Sep 17 00:00:00 2001 From: notevil Date: Thu, 23 Apr 2026 13:16:47 +0200 Subject: [PATCH] P3-01 review fixes : thread-safe assign() + enforce enum load order MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit BUG-001 reviewer: ExtendableEnumManager.assign() non thread-safe, synchronize la méthode. Pas d impact perf (N×11 calls au bootstrap). RISK-001 reviewer: load-order LivingMotions vanilla vs TiedUpLivingMotions custom pouvait faire coller les ordinals si vanilla pas pre-loadé. Force explicit LivingMotions.values() avant TiedUpLivingMotions.values() dans commonSetup. Test renommé pour refléter honnêtement ce qui est testé (vs load order implicitement correct dans le test). --- .../com/tiedup/remake/core/TiedUpMod.java | 13 ++++++-- .../rig/util/ExtendableEnumManager.java | 21 ++++++++++-- .../rig/anim/TiedUpLivingMotionsTest.java | 33 ++++++++++++++----- 3 files changed, 53 insertions(+), 14 deletions(-) diff --git a/src/main/java/com/tiedup/remake/core/TiedUpMod.java b/src/main/java/com/tiedup/remake/core/TiedUpMod.java index e6157e7..6af229d 100644 --- a/src/main/java/com/tiedup/remake/core/TiedUpMod.java +++ b/src/main/java/com/tiedup/remake/core/TiedUpMod.java @@ -138,9 +138,16 @@ public class TiedUpMod { // Register dispenser behaviors (must be on main thread) event.enqueueWork(DispenserBehaviors::register); - // RIG Phase 3 — force class-load des motions custom TiedUp! pour assigner - // les universalOrdinal() via ExtendableEnumManager (init lazy JLS sinon). - // LivingMotions (vanilla EF) est class-loaded naturellement par les patches. + // RIG Phase 3 — force class-load des motions pour assigner les + // universalOrdinal() via ExtendableEnumManager (init lazy JLS sinon). + // + // ORDER MATTERS (review RISK-001 on P3-01 commit 15e405f) : LivingMotions + // vanilla EF doit etre class-loaded AVANT TiedUpLivingMotions, sinon nos + // ordinals prennent 0..N, puis les vanilla arriveraient plus tard avec + // les memes ordinals -> collision silencieuse. Les patches EF ne + // garantissent pas que LivingMotions soit touche avant ce hook, donc on + // le force explicitement ici. + com.tiedup.remake.rig.anim.LivingMotions.values(); com.tiedup.remake.rig.anim.TiedUpLivingMotions.values(); // RIG Phase 2 — dispatcher EntityType → EntityPatch (PLAYER Phase 2, NPCs Phase 5) diff --git a/src/main/java/com/tiedup/remake/rig/util/ExtendableEnumManager.java b/src/main/java/com/tiedup/remake/rig/util/ExtendableEnumManager.java index 0473888..f6c011d 100644 --- a/src/main/java/com/tiedup/remake/rig/util/ExtendableEnumManager.java +++ b/src/main/java/com/tiedup/remake/rig/util/ExtendableEnumManager.java @@ -42,7 +42,14 @@ public class ExtendableEnumManager { this.enums.put(modid, cls); } - public void loadEnum() { + /** + * Synchronized to guard against concurrent class-load of multiple extendable + * enum classes on different threads. The reflective invoke of values() below + * triggers static init which calls {@link #assign(ExtendableEnum)} — also + * synchronized on the same monitor (re-entrant, no deadlock risk). See + * review BUG-001 on P3-01 commit 15e405f. + */ + public synchronized void loadEnum() { List orderByModid = new ArrayList<>(this.enums.keySet()); Collections.sort(orderByModid); Class cls = null; @@ -68,7 +75,17 @@ public class ExtendableEnumManager { TiedUpRigConstants.LOGGER.debug("All enums are loaded: " + this.enumName + " " + this.enumMapByName.values()); } - public int assign(T value) { + /** + * Synchronized because {@code lastOrdinal}, {@code enumMapByOrdinal} + * ({@link Int2ObjectLinkedOpenHashMap}) and {@code enumMapByName} + * ({@link java.util.LinkedHashMap}) are not thread-safe. Concurrent class + * init of extendable enums on different threads (plausible if another mod + * extends LivingMotion at the same bootstrap) would race on the + * read-modify-write of lastOrdinal and on map mutation. No perf impact — + * called N×|motions| times at bootstrap only. See review BUG-001 on P3-01 + * commit 15e405f. + */ + public synchronized int assign(T value) { int lastOrdinal = this.lastOrdinal; String enumName = ParseUtil.toLowerCase(value.toString()); diff --git a/src/test/java/com/tiedup/remake/rig/anim/TiedUpLivingMotionsTest.java b/src/test/java/com/tiedup/remake/rig/anim/TiedUpLivingMotionsTest.java index 98f32e6..dbe5ece 100644 --- a/src/test/java/com/tiedup/remake/rig/anim/TiedUpLivingMotionsTest.java +++ b/src/test/java/com/tiedup/remake/rig/anim/TiedUpLivingMotionsTest.java @@ -20,6 +20,20 @@ import org.junit.jupiter.api.Test; * le meme {@link LivingMotion#ENUM_MANAGER}. Le test force le class-load des * deux en appelant values() pour reproduire ce que TiedUpMod.commonSetup fait * a l'init mod. + * + * NOTE — ces tests n'exercent PAS le scenario prod ou LivingMotions pourrait + * etre charge APRES TiedUpLivingMotions (ce qui ferait coller les ordinals + * silencieusement). Cette garantie est assuree par : + * 1. L'ordre explicite {@code LivingMotions.values()} puis + * {@code TiedUpLivingMotions.values()} dans + * {@code TiedUpMod.commonSetup} (review RISK-001). + * 2. Le {@code synchronized} sur + * {@link com.tiedup.remake.rig.util.ExtendableEnumManager#assign} qui + * empeche la course si un autre mod class-load sa propre extension en + * parallele (review BUG-001). + * + * Un test JVM-fresh (ou l'ordre inverse est force via un ClassLoader isole) + * serait possible mais pas rentable — le verrou est en prod, pas dans le test. */ class TiedUpLivingMotionsTest { @@ -47,18 +61,19 @@ class TiedUpLivingMotionsTest { } /** - * Apres class-load des deux enums, aucune collision d'universalOrdinal ne - * doit exister entre {@link LivingMotions} (vanilla EF) et - * {@link TiedUpLivingMotions} (custom TiedUp!). Ils partagent le meme - * ENUM_MANAGER, donc les ordinals sont assignes a la suite sans doublon. + * Apres class-load des deux enums dans l'ordre {@code LivingMotions} puis + * {@code TiedUpLivingMotions} (ordre force par {@code TiedUpMod.commonSetup} + * en prod), aucune collision d'universalOrdinal ne doit exister entre les + * deux. Ils partagent le meme ENUM_MANAGER, donc les ordinals sont + * assignes a la suite sans doublon. * - * Note : l'ordre de class-load (LivingMotions d'abord ou - * TiedUpLivingMotions d'abord) n'a pas d'importance pour la non-collision, - * mais affecte les ordinals exacts — ce test verifie seulement l'unicite - * croisee, pas des valeurs precises. + * ATTENTION : ce test force l'ordre correct (vanilla avant custom). Il ne + * detecte donc PAS un scenario ou l'ordre serait inverse en prod — voir la + * javadoc de classe pour les garanties externes (commonSetup + assign + * synchronized). */ @Test - void ordinals_doNotCollideWithLivingMotionsVanilla() { + void ordinals_doNotCollideWithLivingMotionsVanilla_assumingCorrectLoadOrder() { // Force class-load des deux enums (idempotent — values() trigger static init) LivingMotions[] vanilla = LivingMotions.values(); TiedUpLivingMotions[] custom = TiedUpLivingMotions.values();