P3-01 review fixes : thread-safe assign() + enforce enum load order
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).
This commit is contained in:
@@ -138,9 +138,16 @@ public class TiedUpMod {
|
|||||||
// Register dispenser behaviors (must be on main thread)
|
// Register dispenser behaviors (must be on main thread)
|
||||||
event.enqueueWork(DispenserBehaviors::register);
|
event.enqueueWork(DispenserBehaviors::register);
|
||||||
|
|
||||||
// RIG Phase 3 — force class-load des motions custom TiedUp! pour assigner
|
// RIG Phase 3 — force class-load des motions pour assigner les
|
||||||
// les universalOrdinal() via ExtendableEnumManager (init lazy JLS sinon).
|
// universalOrdinal() via ExtendableEnumManager (init lazy JLS sinon).
|
||||||
// LivingMotions (vanilla EF) est class-loaded naturellement par les patches.
|
//
|
||||||
|
// 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();
|
com.tiedup.remake.rig.anim.TiedUpLivingMotions.values();
|
||||||
|
|
||||||
// RIG Phase 2 — dispatcher EntityType → EntityPatch (PLAYER Phase 2, NPCs Phase 5)
|
// RIG Phase 2 — dispatcher EntityType → EntityPatch (PLAYER Phase 2, NPCs Phase 5)
|
||||||
|
|||||||
@@ -42,7 +42,14 @@ public class ExtendableEnumManager<T extends ExtendableEnum> {
|
|||||||
this.enums.put(modid, cls);
|
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<String> orderByModid = new ArrayList<>(this.enums.keySet());
|
List<String> orderByModid = new ArrayList<>(this.enums.keySet());
|
||||||
Collections.sort(orderByModid);
|
Collections.sort(orderByModid);
|
||||||
Class<?> cls = null;
|
Class<?> cls = null;
|
||||||
@@ -68,7 +75,17 @@ public class ExtendableEnumManager<T extends ExtendableEnum> {
|
|||||||
TiedUpRigConstants.LOGGER.debug("All enums are loaded: " + this.enumName + " " + this.enumMapByName.values());
|
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;
|
int lastOrdinal = this.lastOrdinal;
|
||||||
String enumName = ParseUtil.toLowerCase(value.toString());
|
String enumName = ParseUtil.toLowerCase(value.toString());
|
||||||
|
|
||||||
|
|||||||
@@ -20,6 +20,20 @@ import org.junit.jupiter.api.Test;
|
|||||||
* le meme {@link LivingMotion#ENUM_MANAGER}. Le test force le class-load des
|
* le meme {@link LivingMotion#ENUM_MANAGER}. Le test force le class-load des
|
||||||
* deux en appelant values() pour reproduire ce que TiedUpMod.commonSetup fait
|
* deux en appelant values() pour reproduire ce que TiedUpMod.commonSetup fait
|
||||||
* a l'init mod.
|
* 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 {
|
class TiedUpLivingMotionsTest {
|
||||||
|
|
||||||
@@ -47,18 +61,19 @@ class TiedUpLivingMotionsTest {
|
|||||||
}
|
}
|
||||||
|
|
||||||
/**
|
/**
|
||||||
* Apres class-load des deux enums, aucune collision d'universalOrdinal ne
|
* Apres class-load des deux enums dans l'ordre {@code LivingMotions} puis
|
||||||
* doit exister entre {@link LivingMotions} (vanilla EF) et
|
* {@code TiedUpLivingMotions} (ordre force par {@code TiedUpMod.commonSetup}
|
||||||
* {@link TiedUpLivingMotions} (custom TiedUp!). Ils partagent le meme
|
* en prod), aucune collision d'universalOrdinal ne doit exister entre les
|
||||||
* ENUM_MANAGER, donc les ordinals sont assignes a la suite sans doublon.
|
* 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
|
* ATTENTION : ce test force l'ordre correct (vanilla avant custom). Il ne
|
||||||
* TiedUpLivingMotions d'abord) n'a pas d'importance pour la non-collision,
|
* detecte donc PAS un scenario ou l'ordre serait inverse en prod — voir la
|
||||||
* mais affecte les ordinals exacts — ce test verifie seulement l'unicite
|
* javadoc de classe pour les garanties externes (commonSetup + assign
|
||||||
* croisee, pas des valeurs precises.
|
* synchronized).
|
||||||
*/
|
*/
|
||||||
@Test
|
@Test
|
||||||
void ordinals_doNotCollideWithLivingMotionsVanilla() {
|
void ordinals_doNotCollideWithLivingMotionsVanilla_assumingCorrectLoadOrder() {
|
||||||
// Force class-load des deux enums (idempotent — values() trigger static init)
|
// Force class-load des deux enums (idempotent — values() trigger static init)
|
||||||
LivingMotions[] vanilla = LivingMotions.values();
|
LivingMotions[] vanilla = LivingMotions.values();
|
||||||
TiedUpLivingMotions[] custom = TiedUpLivingMotions.values();
|
TiedUpLivingMotions[] custom = TiedUpLivingMotions.values();
|
||||||
|
|||||||
Reference in New Issue
Block a user