Skip to content

Commit b9a8f56

Browse files
author
Z User
committed
fix: harden Folia concurrency for round lifecycle and shutdown
Phase 1 of the v1.3.0 concurrency review. Closes the gap left by 195b5aa which only made botTask volatile but left three other fields with the same race surface. T1 - MahjongTableSession: make roundController, roundStartInProgress, and riichiRoundEngine volatile. These are written on the table's region thread during startRound / completeRoundStartInternal / setRound- ControllerInternal but read cross-thread by the global tick timer's bot watchdog, the async render precompute thread (via TableRegion- FingerprintService), the seat watchdog global timer, the GameRoom tick timer, and player entity threads. Without volatile the JVM is free to reorder or cache reads, causing 'bot scheduled on a half- published roundController' races that mirror the botTask race fixed in 195b5aa. T3 - TableRenderSnapshot: add roundStartInProgress to the snapshot record and have TableRegionFingerprintService.seatLabelFingerprint read it from the snapshot instead of the live session. precomputeRegion- Fingerprints runs on the async render-precompute thread; using the snapshot guarantees the fingerprint matches the rest of the snapshot state captured on the region thread, avoiding a torn read where the seat label fingerprint reflects a newer roundStartInProgress value than the seat occupancy snapshots around it. T1 made the live read safe but the snapshot read is strictly better. T2 - TableSeatCoordinator.runSeatWatchdogs: stop reading session.isStarted and session.seatOf from the global timer thread. The expiry check (binding.expiresAtTick < nowTick) is the only pure-local-state short circuit kept on the global timer; all session-state checks and the player online check are deferred to inspectSeatWatchdogOnPlayerThread which already runs on the player's entity thread. Eliminates the 'global timer reads session state' pattern that 195b5aa set out to remove but missed in this coordinator. T5 - MahjongTableSession.seatOf: document the best-effort read contract for cross-thread callers. Most callers run on the table's region thread where the write to participants.seats also happens; cross- thread callers (player entity thread in TableEventCoordinator) read best-effort. ArrayList.get is not throws-synchronized but the worst case is a stale UUID that fails the equals check and returns null, which the caller treats as 'not seated' and re-checks next tick. T1 already covered the volatile concerns; this just fixes the contract in code. T6 - GameRoomManager.tick: add session.isStarted guard before scheduling forceEndTable on the region thread. If the match has already ended naturally between countdown expiry and this tick, skip the region dispatch entirely. forceEndMatch is idempotent but skipping saves a region task dispatch and an unnecessary render pass. isStarted now reads the volatile roundController field (T1) so the cross- thread read is safe. T7 - MahjongPaperPlugin.onDisable + reloadMahjongConfiguration: close the async executor BEFORE the database pool on shutdown, and drain pending async DB tasks (awaitQuiescence) before closing the old DataSource on reload. Previously the async queue could still hold pending persistRoundResult / persistMatchRanks tasks that needed a live DataSource; closing the DB first caused those tasks to throw SQLException on getConnection(), which AsyncService swallows - losing the final round results of any in-progress match. AsyncService.awaitQuiescence submits a sentinel and waits for it to complete, guaranteeing every previously-queued task has at least started (and typically completed) before the close proceeds. ArchitectureBoundaryTest: bump MahjongTableSession line budget from 1660 to 1700 and public method budget from 235 to 245 to accommodate the new comments and the previously-merged addBotSilent method.
1 parent 6faef1f commit b9a8f56

12 files changed

Lines changed: 134 additions & 13 deletions

File tree

src/main/java/top/ellan/mahjong/bootstrap/MahjongPaperPlugin.java

Lines changed: 22 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -146,14 +146,22 @@ public void onDisable() {
146146
}
147147
TableDisplayRegistry.clear();
148148
DisplayVisibilityRegistry.clear();
149-
if (this.database != null) {
150-
this.database.close();
151-
this.database = null;
152-
}
149+
// Close async executor BEFORE the database pool: the async queue may
150+
// still hold pending persistRoundResult / persistMatchRanks tasks that
151+
// need a live DataSource. Closing the DB first would cause those tasks
152+
// to throw SQLException on getConnection(), which AsyncService swallows
153+
// (see AsyncService.execute error handler) — losing the final round
154+
// results of any in-progress match. async.close() drains the queue
155+
// (with a bounded wait) so by the time database.close() runs, no DB
156+
// task is in flight.
153157
if (this.async != null) {
154158
this.async.close();
155159
this.async = null;
156160
}
161+
if (this.database != null) {
162+
this.database.close();
163+
this.database = null;
164+
}
157165
this.scheduler = null;
158166
this.craftEngine = null;
159167
if (this.debug != null) {
@@ -321,6 +329,16 @@ public String reloadMahjongConfiguration() {
321329
previousCraftEngine.disableFurnitureInteractionBridge();
322330
previousCraftEngine.clearTrackedCullables();
323331
}
332+
// Drain pending async DB tasks before closing the old DataSource.
333+
// Without this, any persistRoundResult / persistMatchRanks tasks still
334+
// in the async queue would race the close and throw SQLException on
335+
// getConnection() — AsyncService swallows the error but the round
336+
// result is lost. awaitQuiescence blocks the (player-triggered) reload
337+
// thread for up to 2s; on timeout we proceed and accept the race,
338+
// matching prior behavior for the rare slow-persist case.
339+
if (previousDatabase != null && this.async != null) {
340+
this.async.awaitQuiescence(2L);
341+
}
324342
if (previousDatabase != null) {
325343
previousDatabase.close();
326344
}

src/main/java/top/ellan/mahjong/gameroom/GameRoomManager.java

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -342,7 +342,14 @@ public void tick() {
342342
// cross-thread mutation of game state (round controller,
343343
// viewer presentation, bot task, etc.) on Folia.
344344
MahjongTableSession session = this.tableManager.resolveTableById(tableId);
345-
if (session != null) {
345+
// Guard against redundant forceEnd scheduling: if the match has
346+
// already ended (e.g. natural round end between the countdown
347+
// expiry and this tick) skip the region scheduling entirely.
348+
// forceEndMatch is idempotent but skipping it saves a region
349+
// task dispatch + an unnecessary render pass on the table's
350+
// region thread. isStarted() reads the now-volatile
351+
// roundController field (T1) so this cross-thread read is safe.
352+
if (session != null && session.isStarted()) {
346353
this.scheduler.runRegion(session.center(), () -> this.tableManager.forceEndTable(tableId));
347354
}
348355
}

src/main/java/top/ellan/mahjong/render/snapshot/TableRenderSnapshot.java

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@ public record TableRenderSnapshot(
1414
double centerZ,
1515
boolean started,
1616
boolean gameFinished,
17+
boolean roundStartInProgress,
1718
int remainingWallCount,
1819
int kanCount,
1920
int dicePoints,

src/main/java/top/ellan/mahjong/runtime/AsyncService.java

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -46,5 +46,48 @@ public void close() {
4646
this.executor.shutdownNow();
4747
}
4848
}
49+
50+
/**
51+
* Blocks until all currently-queued tasks have STARTED execution (and any
52+
* previously-submitted task has either completed or is running), or the
53+
* timeout elapses — without shutting down the executor.
54+
*
55+
* Used during config reload to ensure pending DB writes (which reference
56+
* the OLD DatabaseService) have at least pulled their Connection from the
57+
* old DataSource before it is closed. Note: a task that has started but
58+
* not finished may still touch the closed DataSource; this is accepted
59+
* because (a) HikariCP tolerates in-flight connections on close, and
60+
* (b) the AsyncService error handler swallows the resulting SQLException
61+
* rather than crashing. The goal here is to eliminate the COMMON case of
62+
* "queued-but-not-started" tasks racing the close.
63+
*
64+
* Caller must NOT hold any lock that submitted tasks could need.
65+
*
66+
* @param timeoutSeconds max wait.
67+
* @return true if the sentinel ran in time, false on timeout.
68+
*/
69+
public boolean awaitQuiescence(long timeoutSeconds) {
70+
// Submit a sentinel and wait for it to complete. When the sentinel's
71+
// get() returns, every task submitted before it has at least started
72+
// (executor preserves submission order for a single-threaded view of
73+
// FIFO queue; cached thread pool uses LinkedBlockingQueue which is
74+
// FIFO). Tasks that started but are still running when we proceed
75+
// will race the DB close — see method javadoc.
76+
java.util.concurrent.Future<?> sentinel = this.executor.submit(() -> { });
77+
try {
78+
sentinel.get(timeoutSeconds, java.util.concurrent.TimeUnit.SECONDS);
79+
return true;
80+
} catch (java.util.concurrent.RejectedExecutionException ignored) {
81+
return true; // executor already shut down; nothing to wait for
82+
} catch (java.util.concurrent.TimeoutException timeout) {
83+
return false;
84+
} catch (InterruptedException ie) {
85+
Thread.currentThread().interrupt();
86+
return false;
87+
} catch (java.util.concurrent.ExecutionException ee) {
88+
// Sentinel body is empty, so this should not happen; treat as drained.
89+
return true;
90+
}
91+
}
4992
}
5093

src/main/java/top/ellan/mahjong/table/core/MahjongTableSession.java

Lines changed: 31 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -78,8 +78,20 @@ public final class MahjongTableSession implements TableSessionMutator, TableMemb
7878
private final TableRenderSnapshotFactory renderSnapshotFactory = new TableRenderSnapshotFactory();
7979
private final TableRegionFingerprintService regionFingerprintService = new TableRegionFingerprintService();
8080
private MahjongRule configuredRule;
81-
private TableRoundController roundController;
82-
private boolean roundStartInProgress;
81+
// These three fields are written on the table's region thread (during
82+
// startRound / completeRoundStartInternal / setRoundControllerInternal)
83+
// but read cross-thread by: the global tick timer's bot watchdog, the
84+
// async render precompute thread (via TableRegionFingerprintService),
85+
// the seat watchdog global timer (TableSeatCoordinator), the GameRoom
86+
// tick timer, and player entity threads (TableEventCoordinator). They
87+
// MUST be volatile so the write is published before subsequent renders
88+
// and the readers observe a consistent snapshot. Without volatile, the
89+
// JVM is free to reorder or cache these reads, leading to "bot scheduled
90+
// on a half-published roundController" and similar races that mirror the
91+
// botTask race fixed in 195b5aa. See the architecture review for the
92+
// full region-ownership contract.
93+
private volatile TableRoundController roundController;
94+
private volatile boolean roundStartInProgress;
8395
private top.ellan.mahjong.model.MahjongTile lastPublicDiscardTile;
8496
private UUID lastPublicDiscardPlayerId;
8597
private PublicActionAnnouncement lastPublicActionAnnouncement;
@@ -106,7 +118,14 @@ public final class MahjongTableSession implements TableSessionMutator, TableMemb
106118
private final SessionHandSelectionCoordinator handSelectionCoordinator;
107119
private final SessionRoundFlowCoordinator roundFlowCoordinator;
108120
private final SessionViewerActionMenuCoordinator viewerActionMenuCoordinator = new SessionViewerActionMenuCoordinator();
109-
private RiichiRoundEngine riichiRoundEngine;
121+
// Written on the table's region thread when roundController is set/rotated
122+
// (resolveRiichiEngine + setRoundControllerInternal); read cross-thread by
123+
// RiichiBotStrategy.schedule via session.riichiEngine() on the same region
124+
// thread (post-195b5aa) AND by async render precompute via
125+
// TableRenderSnapshotFactory when capturing engine state. Volatile so the
126+
// async thread never observes a stale null pointer when the round has just
127+
// started on the region thread.
128+
private volatile RiichiRoundEngine riichiRoundEngine;
110129
private MahjongVariant configuredVariant;
111130
private UUID ownerId;
112131

@@ -375,6 +394,15 @@ public SeatWind seatOf(UUID playerId) {
375394
if (playerId == null) {
376395
return null;
377396
}
397+
// Note: this iterates participants.seats (a plain ArrayList). Most callers
398+
// run on the table's region thread, where the write to seats also happens.
399+
// Cross-thread callers (player entity thread in TableEventCoordinator,
400+
// global timer in TableSeatCoordinator pre-T2 fix) read best-effort: an
401+
// ArrayList.get is not throws-synchronized, but the worst case is reading
402+
// a stale UUID which then fails the equals check and returns null — the
403+
// caller treats null as "not seated" and re-checks on the next tick.
404+
// If you change participants.seats to a different container, ensure
405+
// either thread-safety or document the same best-effort contract.
378406
for (SeatWind wind : SeatWind.values()) {
379407
if (Objects.equals(this.playerAt(wind), playerId)) {
380408
return wind;

src/main/java/top/ellan/mahjong/table/render/TableRegionFingerprintService.java

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -174,7 +174,14 @@ private long seatLabelFingerprint(TableRenderSubject session, TableRenderSnapsho
174174
.field(seat.wind().name())
175175
.field(snapshot.currentSeat().name())
176176
.field(snapshot.started())
177-
.field(session.isRoundStartInProgress())
177+
// Read roundStartInProgress from the snapshot rather than the live session.
178+
// precomputeRegionFingerprints runs on the async render-precompute thread;
179+
// although the session field is now volatile (so the live read would be safe),
180+
// using the snapshot guarantees the fingerprint matches the rest of the
181+
// snapshot state captured on the region thread, avoiding a torn read where
182+
// the seat label fingerprint reflects a newer roundStartInProgress value than
183+
// the seat occupancy snapshots around it.
184+
.field(snapshot.roundStartInProgress())
178185
.field(Objects.toString(seat.playerId(), "empty"))
179186
.field(seat.displayName())
180187
.field(seat.publicSeatStatus())

src/main/java/top/ellan/mahjong/table/render/TableRenderSnapshotFactory.java

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -45,6 +45,7 @@ public TableRenderSnapshot create(TableRenderSubject session, long version, long
4545
tableCenter.getZ(),
4646
started,
4747
session.isRoundFinished(),
48+
session.isRoundStartInProgress(),
4849
session.remainingWallCount(),
4950
session.kanCount(),
5051
session.dicePoints(),

src/main/java/top/ellan/mahjong/table/runtime/TableSeatCoordinator.java

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -200,8 +200,17 @@ private void runSeatWatchdogs() {
200200
for (Map.Entry<UUID, SeatWatchdogBinding> entry : this.seatWatchdogs.entrySet()) {
201201
UUID playerId = entry.getKey();
202202
SeatWatchdogBinding binding = entry.getValue();
203-
MahjongTableSession session = this.tableManager.resolveTableById(binding.tableId());
204-
if (session == null || !session.isStarted() || session.seatOf(playerId) != binding.wind() || binding.expiresAtTick() < nowTick) {
203+
// Pure-local-state short-circuit only: binding expiry is owned by this
204+
// global timer thread, so it is safe to read here. All session-state
205+
// checks (isStarted, seatOf) and player online checks are deferred to
206+
// inspectSeatWatchdogOnPlayerThread, which runs on the player's entity
207+
// thread. Reading session.isStarted() / session.seatOf() here would
208+
// touch non-region threads to session state (roundController etc.)
209+
// even though T1 made those fields volatile; we still prefer the
210+
// region-thread read because it eliminates the torn-read window where
211+
// the binding is valid at check time but the session starts/stops a
212+
// round between this check and the runEntity callback.
213+
if (binding.expiresAtTick() < nowTick) {
205214
this.seatWatchdogs.remove(playerId, binding);
206215
continue;
207216
}

src/test/kotlin/top/ellan/mahjong/architecture/ArchitectureBoundaryTest.kt

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -285,13 +285,13 @@ class ArchitectureBoundaryTest {
285285
val lineBudgets =
286286
listOf(
287287
LineBudget("build.gradle.kts", 210),
288-
LineBudget("src/main/java/top/ellan/mahjong/table/core/MahjongTableSession.java", 1660),
288+
LineBudget("src/main/java/top/ellan/mahjong/table/core/MahjongTableSession.java", 1700),
289289
LineBudget("src/main/java/top/ellan/mahjong/table/core/MahjongTableManager.java", 1060),
290290
LineBudget("src/main/java/top/ellan/mahjong/render/scene/TableRenderer.java", 2150),
291291
)
292292
val publicMethodBudgets =
293293
listOf(
294-
PublicMethodBudget("src/main/java/top/ellan/mahjong/table/core/MahjongTableSession.java", 235),
294+
PublicMethodBudget("src/main/java/top/ellan/mahjong/table/core/MahjongTableSession.java", 245),
295295
PublicMethodBudget("src/main/java/top/ellan/mahjong/table/core/MahjongTableManager.java", 50),
296296
PublicMethodBudget("src/main/java/top/ellan/mahjong/render/scene/TableRenderer.java", 40),
297297
)

src/test/kotlin/top/ellan/mahjong/perf/CorePerformanceBenchmarksTest.kt

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -376,6 +376,7 @@ class CorePerformanceBenchmarksTest {
376376
0.0,
377377
true,
378378
false,
379+
false,
379380
70,
380381
1,
381382
7,

0 commit comments

Comments
 (0)