Fix unsafe lazy singleton and database map race in DBHandler#3596
Fix unsafe lazy singleton and database map race in DBHandler#3596MattBDev wants to merge 3 commits into
Conversation
dbHandler() used a non-synchronized, non-volatile double-null-check, so concurrent callers could each construct a DBHandler and see different instances. Replace it with the initialization-on-demand holder idiom, which the JVM guarantees is thread-safe and lazy without synchronizing every access. getDatabase(World) checked the map, constructed a RollbackDatabase, then put it, so two threads racing on the same world could each open a separate SQLite connection to that world's summary.db, leak one of them and risk SQLITE_BUSY. Use putIfAbsent and close the losing instance instead of leaking it. Behaviour on construction failure is unchanged: log and return null without caching, so a later call can retry. Adds DBHandlerConcurrencyTest covering the singleton race. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR hardens com.fastasyncworldedit.core.database.DBHandler against concurrency bugs as part of Phase 1 of the com.fastasyncworldedit.core.history improvement plan, addressing unsafe lazy-singleton initialization and races when creating per-world rollback databases.
Changes:
- Replaces the racy lazy singleton in
dbHandler()with the initialization-on-demand holder idiom. - Switches the per-world database cache to
putIfAbsentand closes the losingRollbackDatabaseinstance to avoid leaking connections. - Adds
DBHandlerConcurrencyTestto guard against singleton regressions (including a structural check ensuring the holder-style pattern is used).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
worldedit-core/src/main/java/com/fastasyncworldedit/core/database/DBHandler.java |
Implements holder-based singleton and updates the per-world database cache insertion logic for concurrency safety. |
worldedit-core/src/test/java/com/fastasyncworldedit/core/database/DBHandlerConcurrencyTest.java |
Adds regression tests validating singleton behavior and structure under concurrent access. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| try { | ||
| database = new RollbackDatabase(world); | ||
| databases.put(world, database); | ||
| return database; | ||
| RollbackDatabase created = new RollbackDatabase(world); | ||
| RollbackDatabase existing = databases.putIfAbsent(world, created); | ||
| if (existing != null) { | ||
| created.close(); |
There was a problem hiding this comment.
Good point, fixed in 56c13bb: switched to computeIfAbsent, which runs its mapping function at most once per key while other callers racing on the same world block on it. So at most one RollbackDatabase (and one SQLite connection) is ever constructed per world, not just per losing-instance-cleanup as with putIfAbsent.
The previous get-then-putIfAbsent still let two threads racing on the same World each construct a RollbackDatabase (and thus each open a SQLite connection) before one was discarded as the loser - the connection-open contention this change was meant to eliminate just became short-lived instead of gone. ConcurrentHashMap.computeIfAbsent runs its mapping function at most once per key, blocking other callers racing on the same key until it completes, so at most one RollbackDatabase is ever constructed per world. The mapping function can't declare checked exceptions, so RollbackDatabase's SQLException/ClassNotFoundException (and anything else, matching the original catch (Throwable) scope) is wrapped in an unchecked type, unwrapped and logged the same way as before, and no value is cached on failure so a later call can still retry. Also adds timeouts to DBHandlerConcurrencyTest's CyclicBarrier#await() and Future#get(), so a stuck thread fails the test instead of hanging it. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
| return databases.computeIfAbsent(world, w -> { | ||
| try { | ||
| return new RollbackDatabase(w); | ||
| } catch (Throwable e) { |
There was a problem hiding this comment.
Fixed in a605898: narrowed to catch (Exception e), so a real Error propagates instead of being logged and swallowed as if it were an ordinary construction failure.
| } | ||
| }); | ||
| } catch (RollbackDatabaseConstructionException e) { | ||
| LOGGER.error("No JDBC driver found!", e.getCause()); |
There was a problem hiding this comment.
Fixed in a605898: the message now names the specific world and includes the cause, instead of the inaccurate blanket 'No JDBC driver found!'.
| // computeIfAbsent (not the earlier get-then-putIfAbsent) serializes construction itself: | ||
| // ConcurrentHashMap guarantees the mapping function runs at most once per key while | ||
| // racing callers for the same world block on it, so at most one RollbackDatabase (and | ||
| // one underlying SQLite connection) is ever constructed per world - putIfAbsent alone | ||
| // still let two threads each open a connection before one was discarded as the loser. |
There was a problem hiding this comment.
The PR description already describes computeIfAbsent, not putIfAbsent - this looks like it was reviewed against an earlier version of the description before that edit landed. Current description: 'getDatabase(World) uses computeIfAbsent, not putIfAbsent...' Let me know if you're still seeing the stale version.
| /** | ||
| * Initialization-on-demand holder: guarantees thread-safe, lazy construction of the | ||
| * {@link DBHandler} singleton without needing explicit synchronization on every access. | ||
| */ |
There was a problem hiding this comment.
Good point, fixed in a605898: the Holder javadoc now explains that in isolation it would be genuinely lazy, but the deprecated IMP field's initializer calling dbHandler() during DBHandler's own static init means INSTANCE is actually constructed eagerly on first reference to DBHandler at all.
| }) | ||
| .collect(Collectors.toList()); | ||
|
|
||
| List<Future<DBHandler>> futures = executor.invokeAll(tasks); |
There was a problem hiding this comment.
Fixed in a605898: uses the timed invokeAll(tasks, 15, TimeUnit.SECONDS) overload.
…und invokeAll getDatabase()'s mapping-function catch was Throwable, which would swallow a real Error (e.g. OutOfMemoryError) and convert it into a logged message plus a retryable null - masking a serious failure instead of letting it propagate. Narrowed to Exception, which is all RollbackDatabase construction needs translated into the retryable null result. The log message "No JDBC driver found!" was inaccurate for most construction failures (SQLException from file/lock issues, etc.), which would send anyone debugging a production failure down the wrong path. Replaced with a message naming the actual world and cause. Holder's javadoc claimed the singleton is "lazy" without qualification, but the deprecated IMP field's initializer calls dbHandler() during DBHandler's own static init, so INSTANCE is actually constructed eagerly on first reference to DBHandler at all. Documented that caveat. executor.invokeAll(tasks) had no timeout and could hang the test indefinitely if a task never started. Uses the timed overload now. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Part of Phase 1 (concurrency hardening) of the
com.fastasyncworldedit.core.historyimprovement plan. One of five independent PRs; this one only touchesDBHandler.The bugs
dbHandler()used a non-synchronized, non-volatiledouble-null-check, so concurrent callers could each construct aDBHandlerand see different instances.getDatabase(World)checked the map, constructed aRollbackDatabase, thenputit. Two threads racing on the same world could each open a separate SQLite connection to that world'ssummary.db, leak one and riskSQLITE_BUSY.The fix (revised after code review — see commit
56c13bb7f)dbHandler()uses the initialization-on-demand holder idiom — thread-safe and lazy, with no synchronization on the hot path. The deprecatedIMPfield still resolves to the same instance.getDatabase(World)usescomputeIfAbsent, notputIfAbsent. The originalputIfAbsentfix still let two threads racing on the same world each construct aRollbackDatabase(and thus each open a SQLite connection) before one was discarded as the loser — the connection-open contention this PR set out to eliminate just became short-lived instead of gone.ConcurrentHashMap.computeIfAbsentruns its mapping function at most once per key, blocking other callers racing on the same key until it completes, so at most oneRollbackDatabaseis ever constructed per world. The mapping function can't declare checked exceptions, soRollbackDatabase'sSQLException/ClassNotFoundException(and anything else, matching the originalcatch (Throwable)scope) is wrapped in an unchecked type internally, then unwrapped and logged the same way as before.nullwithout caching, so a later call can retry.Verification
:worldedit-core:compileJava/compileTestJavapass.DBHandlerConcurrencyTestis a genuine regression test: 1 failure against pre-fix code, 2/2 pass with the fix.dbHandler()singleton race, which is fully testable in isolation. ThegetDatabasemap race is not covered by an automated test — constructing a realRollbackDatabaseneeds a live world/JDBC driver — so that half is verified by inspection only.CyclicBarrier#await()andFuture#get()in the test now take a 10-second timeout, so a stuck thread fails the test instead of hanging it.Notes
git worktreeadditionally need the Grgit fix from Add baseline benchmark for history write path (pre Phase 1 concurrency fixes) #3593; deliberately not included (CI uses a normal checkout).🤖 Generated with Claude Code