fix(lock): stop guarding the filesystem lock with an interned string literal and make the provider serializable - #19486
Conversation
hudi-agent
left a comment
There was a problem hiding this comment.
Thanks for working on this! The PR replaces the interned "lock" string literal used as a synchronization monitor in FileSystemBasedLockProvider with a private static Object, and makes currentOwnerLockInfo volatile so its value is visible to the (unsynchronized) getter readers. I traced all three converted synchronized blocks, the write site (reloadCurrentOwnerLockInfo), and both external readers (LockManager, TimeGeneratorBase) — the static scope is preserved so mutual exclusion is unchanged, and the volatile read is sufficient for these diagnostic single-reads. No correctness issues found. A few style/readability suggestions in the inline comments. Please take a look, and this should be ready for a Hudi committer or PMC member to take it from here. One small nit on the production Javadoc; otherwise the fix is clean and well-documented.
cc @yihua
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #19486 +/- ##
============================================
+ Coverage 77.49% 77.86% +0.36%
- Complexity 32786 33193 +407
============================================
Files 2522 2532 +10
Lines 139142 140056 +914
Branches 16723 17413 +690
============================================
+ Hits 107827 109052 +1225
+ Misses 23754 23383 -371
- Partials 7561 7621 +60
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
hudi-agent
left a comment
There was a problem hiding this comment.
Thanks for working on this! This PR fixes a real concurrency defect in FileSystemBasedLockProvider by replacing the interned "lock" String-literal monitor with a private static Object, and makes currentOwnerLockInfo volatile so its value is visibly published to the unsynchronized getter readers. I traced the monitor usage across close()/tryLock()/unlock() and the write/read paths for currentOwnerLockInfo (writers in reloadCurrentOwnerLockInfo() under the monitor, readers in LockManager and TimeGeneratorBase without it) — the change is well-scoped and consistent, preserves the prior mutual-exclusion scope, and introduces no new lock-ordering or serialization concerns. No issues flagged from this automated pass — a Hudi committer or PMC member can take it from here for a final review.
cc @yihua
9d0b775 to
a639f73
Compare
hudi-agent
left a comment
There was a problem hiding this comment.
Thanks for working on this! This PR fixes FileSystemBasedLockProvider so its lock-file operations synchronize on a private static LOCK_FILE_MONITOR object instead of the interned "lock" String literal, and makes currentOwnerLockInfo volatile since it's read through the Lombok getter outside the monitor. The mutual-exclusion scope is preserved (still static), and the volatile read correctly restores visibility. No issues flagged from this automated pass — a Hudi committer or PMC member can take it from here for a final review.
cc @yihua
voonhous
left a comment
There was a problem hiding this comment.
Read the monitor swap, the volatile change and the new test against master. The fix is right, and the scope-preservation argument holds within a classloader, which is the case that matters.
All notes are inline, roughly in priority order: (1)-(3) are worth addressing before merge, (4)-(5) are accuracy fixes to the commit message and description, (6)-(7) are adjacent issues you may want to punt explicitly rather than leave implicit, (8)-(10) are nits. Nothing here objects to the approach.
…literal
FileSystemBasedLockProvider synchronized its three lock-file blocks on LOCK_FILE_NAME.
That is a compile-time String constant, so it is interned and shared JVM-wide: any class
anywhere that synchronizes on the same "lock" literal contends on the very same monitor,
and unrelated code holding it blocks Hudi's lock acquisition outright.
The aliasing is not hypothetical. In this repo,
hudi-client-common's FileSystemBasedLockProviderTestClass declares
private static final String LOCK = "lock";
and not only synchronizes on it but calls LOCK.wait(retryWaitTimeMs) inside that block, so
an unrelated lock provider implementation waits and notifies on the same monitor the
production provider takes.
Synchronize on a private static final Object instead; a private object cannot be aliased.
Kept static so the mutual-exclusion scope is exactly what it was - all instances continue
to serialize their lock-file operations - and a static monitor also survives the
Serializable round-trip that a transient instance field would not.
currentOwnerLockInfo becomes volatile. It is assigned while holding the monitor in tryLock
but read through the Lombok-generated getter without it, so without volatile the write is
not guaranteed visible to a reader on another thread.
Regression test: an unrelated daemon thread takes synchronized ("lock") and holds it while
tryLock runs on a separate executor thread. On master the acquisition never returns and the
test fails with a message naming the cause; with the fix it acquires immediately.
before (production file at master, test applied): 10 run, 1 failed
AssertionError: tryLock never returned - it is blocked on the monitor held by the
unrelated thread ...
after: 10 run, 0 failed
Also in this file, reloadCurrentOwnerLockInfo() opened the lock file in the
try-with-resources header, ahead of its own existence check. storage.open throws
FileNotFoundException on a missing path, so the empty-string branch was unreachable: a
lock file that vanished threw instead of clearing the field, the caller swallowed it,
and LockManager then reported the previous owner as the current one.
The test helper FileSystemBasedLockProviderTestClass had the same interned-literal
defect, and was the last "lock" monitor in the tree. Its LOCK field is split into a
private Object monitor and a LOCK_FILE_NAME string.
The regression test now exercises unlock() and close() while the interned monitor is
held, not just tryLock(): reverting any one of the three blocks fails it.
Whole transaction package: 257 tests, 0 failures. checkstyle 0, apache-rat 0.
Part of apache#16943
a639f73 to
5821946
Compare
hudi-agent
left a comment
There was a problem hiding this comment.
Thanks for working on this! This PR replaces the interned "lock" string monitor in FileSystemBasedLockProvider (and its test helper) with a private static object, makes currentOwnerLockInfo volatile, and reorders reloadCurrentOwnerLockInfo() to check existence before opening the lock file. I traced the monitor scope, the two getCurrentOwnerLockInfo() consumers, and the reload failure path — the changes are correct and the earlier rounds already covered the test-cleanup and doc points. No new issues flagged from this automated pass — a Hudi committer or PMC member can take it from here for a final review.
cc @yihua
…re Serializable Review rounds 2 and 3 on apache#19486. - acquireLock's storage.create(path, false) is the provider's cross-process mutual exclusion on stores whose create is atomic, and no public-API test could reach the already-exists arm: tryLock returns early on an existing file, and in-JVM contenders serialize on the monitor (which is also why the multi-writer suites in hudi-spark and hudi-java-client never reach it). Flipping the false to true left the transaction package green. acquireLock is package-private @VisibleForTesting now, and a test pins the loser failing on FileAlreadyExistsException with the winner's payload intact. The javadocs say "exclusive-mode create" rather than claiming atomicity: HDFS guarantees it, local FS's create is a check-then-act. - reloadCurrentOwnerLockInfo checked exists() before open(), which closed the already-gone case but still threw when the file vanished between the two calls, leaving the stale owner to be reported by LockManager. The exists() check is gone; a FileNotFoundException from the open, or from the first read on a lazy-fetch store, now clears the field. One RPC instead of two on the hot path. The vanish window is pinned via a hoodie.storage.class stub whose exists() says true while open() throws FileNotFoundException, and HoodieStorage.open now documents the FileNotFoundException contract the catch relies on. - tryLock's catch arm, the clean-false outcome a losing writer sees, had never executed in any test. Pointing the lock directory at an existing regular file reaches it deterministically. - The class declared Serializable but threw NotSerializableException whenever it had locked: lockInfo holds a LockInfo, which is not Serializable (since HUDI-5377). The marker is load-bearing, the HUDI-7782 bug class: CleanActionExecutor's Spark closure captures the transaction manager, and from there the provider. lockInfo and sdf are transient now, rebuilt on demand in initLockInfo, serialVersionUID is pinned at 1L, and a round trip of a lock-holding provider is covered by test. A deserialized copy still cannot lock (storage and lockFile stay transient-null, as before this change); the comment says so. - The monitor javadoc claimed the lost cross-classloader scope "was never a correctness guarantee, since the atomic create on storage is the real mutual exclusion". That is wrong for the expiry-reclaim sequence, which the atomic create does not protect. It now says what is true: that sequence is already unsafe across processes, and the monitor is defense in depth, not the correctness mechanism. - Folded the standalone reload test into testConcurrentProvidersCannotBothHoldLock, where the contender, not the holder reading back its own file, observes the cleared owner info. The interned-literal test's finally no longer unlocks/closes a provider the test body already closed; that double close is safe only while HoodieHadoopStorage close() stays a no-op. Each pin was reverted individually to confirm it discriminates: create(path, true) fails the atomic-create test; non-transient lockInfo fails the round trip with NotSerializableException; restoring the exists-precheck reload fails the vanish-stub test; rethrowing from tryLock's catch fails the failing-create test. Whole transaction package: 260 tests, 0 failures. checkstyle 0, apache-rat 0, on both touched modules (hudi-client-common, hudi-io). Part of apache#16943
…ure path Review round 4 on apache#19486, run on a second model. - The reload's two-catch split was only half pinned: widening the FileNotFoundException catch to plain IOException, silently clearing the owner on any IO error, left every test green. A stub whose open() throws a plain IOException now pins that a non-missing-file failure escapes as HoodieIOException instead of clearing the last known owner. - checkIfExpired's catch, degrade a stat failure to "not expired", had never executed in a test and turned out to be cheaply reachable with the same stub pattern: a storage failing only getPathInfo. The new test kills both mutants of that catch: treating the failure as expired steals the holder's live lock (asserted via the lock file surviving), and rethrowing skips the owner-info reload (asserted via the loser still reporting the owner). HUDI-4505 already broke this expression once. - initLockInfo() lost the final-field safe-publication guarantee when sdf and lockInfo went lazy; its body is now synchronized on the monitor, which also covers SimpleDateFormat not being thread safe for any off-monitor caller of this public method. - currentOwnerLockInfo initializes to the empty string, so LockManager never logs "Current lock owner information : null" on the paths that fail before any reload. - Comment accuracy: the constructor used to build LockInfo eagerly, so every instance failed serialization, not just lock-holding ones; that is what makes the pinned serialVersionUID compat-safe (no old-UID stream can exist) and the comments now say so. The monitor javadoc names both windows the exclusive create does not protect, not just expiry reclaim. The HoodieStorage.open contract now covers lazily fetching implementations surfacing the miss at first read. - Test hygiene: the storage-class-injecting tests disable the Hadoop FileSystem cache so the fork-wide cache cannot retain their per-test conf, and a comment marks that conf as deliberately unshared; Files.createFile replaces the irrelevant one-byte payload; the new test names drop articles to match the file. Each new pin was red-checked: the widened catch fails the IO-failure test; both checkIfExpired mutants (return true, rethrow) fail the stat-failure test on the intended assertion. Whole transaction package: 262 tests, 0 failures. checkstyle 0, apache-rat 0 on both touched modules. Part of apache#16943
hudi-agent
left a comment
There was a problem hiding this comment.
Thanks for working on this! The PR swaps the filesystem lock provider's monitor from the JVM-interned "lock" string literal to a private static Object, makes currentOwnerLockInfo volatile, marks lockInfo/sdf transient with lazy init to fix the serialization defect, and reworks reloadCurrentOwnerLockInfo to treat a missing lock file as no-owner rather than throwing. I traced the monitor scope, the lazy-init path under the lock, the volatile publication, and the FileNotFoundException handling, and the changes hold up; the open points from earlier rounds already cover the Javadoc/contract details. No new correctness issues flagged from this automated pass — a Hudi committer or PMC member can take it from here for a final review.
cc @yihua
… commit's claims Round 5 of review on apache#19486, fixing the prior commit's own additions. - The IO-failure reload test only pinned "it throws"; a mutant that both throws and clears the owner survived. FailingOpenStorage is togglable now, the test seeds a real owner first, and the failure must keep it. - The Files.exists assertion in the stat-failure test could not fail: under the treat-as-expired mutant the contender re-creates the lock file at the same path. Replaced by a byte-identical payload comparison, which kills that mutant on its own. The prior commit's message wrongly credited the existence check with that kill; tryLock returning true is what caught it. - currentOwnerLockInfo's new empty-string initializer was itself unpinned; the failing-create test, the one path that never reloads, now asserts it. - The prior commit's message also said HUDI-4505 "broke" the expiry expression; it fixed an int overflow there. The serialization comment overclaimed "no serialized form can exist": 0.12.x did serialize this class, but its computed UID was already unreadable against every 0.13.0+ build, which is the actual reason pinning 1L is compat-safe. Both corrected in the comments. - checkIfExpired documents the fail-safe-toward-the-holder choice, citing StorageBasedLockProvider's LockGetResult.UNKNOWN_ERROR arm, and its catch logs ACQUIRING instead of the contradictory ALREADY_RELEASED. The monitor javadoc names the release path among the windows the exclusive create does not protect. - The fs.file.impl.disable.cache lines are gone: no precedent in the tree, the conf is fresh per test, and the cached-conf channel is unreachable for a plain LocalFileSystem. The stub-injection comment cites TestRequestHandler instead. Red-checked: the throw-and-clear mutant, the removed initializer, and both checkIfExpired mutants each fail exactly the assertion written for them. Whole transaction package: 262 tests, 0 failures. checkstyle 0, apache-rat 0. Part of apache#16943
|
Pushed three commits addressing review rounds 2 through 5 (the later rounds reviewed the fixes themselves, on two different models); the PR description has been updated to match the final diff, and the notes below carry the definitive squash message for the merging committer.
Every pin was reverted individually to confirm it fails exactly the assertion written for it. Transaction package: 262 tests, 0 failures; checkstyle and rat clean on both touched modules. The serialVersionUID paragraph in the description should be replaced by a note that the class has not been Java-serializable at all since 0.13.0. |
|
Notes for future reference: Mutation ledger. Every production change is pinned by a test that was run against the reverted change to confirm it fails: the three monitor blocks (each reverted alone times out the interned-literal test), Serialization compatibility. 0.12.x is the only release line that ever serialized this class (HUDI-5377 landed in 0.13.0). Its streams carry a computed |
…literal and make the provider serializable (apache#19486) FileSystemBasedLockProvider synchronized tryLock/unlock/close on the "lock" String literal. Compile-time constants are interned, so any class in the JVM synchronizing on the same literal contends on the very same monitor; the in-repo test helper FileSystemBasedLockProviderTestClass did exactly that, waiting on the monitor the production provider takes. The guard is a private static Object now, same scope within a classloader, no aliasing. The test helper got the same fix. Also fixed while in the file, each pinned by a test that fails without it: - reloadCurrentOwnerLockInfo evaluated storage.open in its try-with-resources header, so a vanished lock file threw out of the reload instead of clearing the field, and LockManager reported the previous owner as current. The reload now treats FileNotFoundException (at open, or at first read on lazily fetching stores) as "no owner" and lets any other IOException escape as HoodieIOException. - The class declared Serializable but every instance since 0.13.0 (HUDI-5377) failed to serialize: lockInfo held a non-Serializable LockInfo built eagerly in the constructor, breaking Spark closure capture (the HUDI-7782 bug class). lockInfo and sdf are transient and lazily rebuilt under the monitor; serialVersionUID is pinned at 1L. - currentOwnerLockInfo is volatile (getCurrentOwnerLockInfo is public LockProvider API and may be read from another thread) and defaults to "" so LockManager never logs a null owner. - acquireLock is package-private @VisibleForTesting: storage.create(path, false) is the provider's cross-process mutual exclusion on stores with atomic create, and no public-API test can reach its already-exists arm. - checkIfExpired documents that a stat failure degrades to "not expired", the same fail-safe choice as StorageBasedLockProvider's UNKNOWN_ERROR arm, and its catch logs ACQUIRING instead of the contradictory ALREADY_RELEASED. - HoodieStorage.open documents the FileNotFoundException contract the reload relies on; TestHoodieStorageBase already asserts it. New tests: interned-monitor non-blocking for tryLock/unlock/close, atomic-create loser with the winner's payload intact, tryLock returning false when the create fails, reload clearing on a vanished lock file, reload keeping the last known owner on a real IO error, a stat failure during the expiry check neither stealing a live lock nor skipping the owner reload, and a Java serialization round trip of a lock-holding provider. Part of apache#16943 (cherry picked from commit bbb2a3a)
Describe the issue this Pull Request addresses
Part of #16943 (HUDI-9254). This PR fixes one self-contained defect in
FileSystemBasedLockProvider, plus what review turned up in the same file; the rest of the issue is listed at the bottom.tryLock,unlockandcloseguarded their lock-file operations withsynchronized (LOCK_FILE_NAME), whereLOCK_FILE_NAMEis the compile-time constant"lock". String constants are interned, so that monitor is the JVM-wide canonical"lock"instance: any code anywhere in the process that synchronizes on the same literal blocks Hudi's lock acquisition, and Hudi blocks it in turn. Not hypothetical: this repo's ownFileSystemBasedLockProviderTestClasssynchronized and waited on the identical literal. The correct form already exists inKafkaConnectControlAgent: a privateObjectmonitor.Summary and Changelog
LOCK_FILE_MONITORobject: same scope within a classloader, no aliasing. The test helper got the same fix, so no"lock"monitor remains in the tree.currentOwnerLockInfobecomesvolatile(getCurrentOwnerLockInfo()is publicLockProviderAPI and may be read from another thread) and defaults to""soLockManagernever logs a null owner.Added over review rounds (commits
771e03fa09a9,428a7197aee0,831aed976cbf, pushed by @voonhous):reloadCurrentOwnerLockInfo()could report a stale owner.storage.openwas evaluated in the try-with-resources header, so a vanished lock file threw instead of clearing the field andLockManagerprinted the previous owner as current -- the surviving half of the5faefcd01fa8regression; test(client): add unit coverage for client utilities and services #19222 fixed only theacquireLockhalf. Final shape:FileNotFoundException, atopenor at first read on a lazily fetching store, means "no owner"; any otherIOExceptionescapes so a real IO error is never mistaken for a missing file. One storage RPC instead of two.Serializablebut every instance since 0.13.0 failed to serialize. HUDI-5377 added an eagerly built, non-serializableLockInfo, breaking Spark closure capture (the HUDI-7782 bug class).lockInfo/sdfaretransientwith lazy rebuild;serialVersionUIDis pinned at1L. A deserialized copy still cannot lock; the durable fix is listed below.acquireLock()is package-private@VisibleForTesting. Its exclusivestorage.create(path, false)is the cross-process mutual exclusion, and its already-exists arm had never executed in any test.checkIfExpired()documents that a stat failure fails safe toward the holder (the same choice asStorageBasedLockProvider'sUNKNOWN_ERRORarm) and logsACQUIRINGinstead of the contradictoryALREADY_RELEASED.HoodieStorage.open(hudi-io) documents theFileNotFoundExceptioncontract the reload relies on;TestHoodieStorageBasealready asserts it.Verification
Every production change is pinned by a test that fails with the change reverted. The full mutation ledger, the serialization-compat detail, and a definitive squash message for the merging committer are in this comment.
Still open under HUDI-9254, not in this PR
StorageSchemes.FILEwrongly claims atomic creation for local FS (its create is check-then-act) -- that claim deserves its own issue.hoodie.write.lock.filesystem.expireabove 35,791,394 minutes wraps negative and every live lock reads as expired (HUDI-4505 moved the wrap point, did not remove it). One-character fix (60L) plus a test.transient lockProviderinLockManager/TimeGeneratorBase, which already build it lazily.close()deletes the lock file unconditionally, so a loser exhausting retries deletes the winner's lock viaTransactionManagertry-with-resources close.acquireLockcan leak an unreclaimable lock file if the payload write fails after the create, at the defaultexpire=0.InterProcessMutex; HUDI-8005's revert of DynamoDB's HUDI-7782 serialization fix.unlock/closecatch arms; the 3-arg constructor discardsHoodieLockMetrics.Impact
No config or table format change.
acquireLockwidens to package-private (@VisibleForTesting);HoodieStorage.open's javadoc states behavior its implementations already have. Serializing a lock-holding provider used to throwNotSerializableException; it now round-trips (the copy stays unusable for locking, as its transient storage handles always were).Risk Level
low -- one monitor swapped at the same scope, a
volatile, two transient fields with lazy rebuild, and a mutation-tested reload restructure. Regression surface covered by the full transaction/lock package.Documentation Update
none (javadoc only)
Contributor's checklist
831aed976cbf