fix(lock): stop guarding the filesystem lock with an interned string literal - #19486
fix(lock): stop guarding the filesystem lock with an interned string literal#19486rangareddy wants to merge 2 commits into
Conversation
…literal Part of HUDI-9254. FileSystemBasedLockProvider guarded tryLock, unlock and close with synchronized (LOCK_FILE_NAME), where LOCK_FILE_NAME is the compile-time constant "lock". String constants are interned, so that monitor is shared JVM-wide with every other "lock" literal, and any unrelated code holding it blocks lock acquisition outright. This is not hypothetical: FileSystemBasedLockProviderTestClass in this repo declares its own static final String LOCK = "lock" and therefore contends on exactly the same object. KafkaConnectControlAgent already uses the correct form, private static final Object LOCK = new Object(). Guard on a private object instead. It is kept static, so the mutual-exclusion scope is unchanged and this is purely about which object is used. currentOwnerLockInfo is also made volatile: it is written while holding the monitor but read through the Lombok-generated getter without it, so the value was not guaranteed visible to the thread reading it. Not addressed here, and still open under HUDI-9254: the monitor being static serialises unrelated tables in one JVM, the other providers' instance state, and the close-during-unlock semantics the Jira describes.
hudi-agent
left a comment
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 76.97% 76.96% -0.01%
Complexity 33855 33855
============================================
Files 2575 2575
Lines 143378 143379 +1
Branches 17573 17573
============================================
- Hits 110362 110355 -7
- Misses 24755 24762 +7
- Partials 8261 8262 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Review nit: a production javadoc referencing a test class by name goes stale silently if the test is renamed. The durable point is the mechanism - any class interning the same literal shares the monitor - so state that instead. The concrete instance in this repo stays in the commit message and the PR.
hudi-agent
left a comment
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
4 participants
Describe the issue this Pull Request addresses
Part of #16943 (HUDI-9254), "Ensure lock providers are thread safe". That issue covers five providers and
the close-during-unlock semantics; this PR fixes one concrete, self-contained defect in
FileSystemBasedLockProviderand deliberately leaves the rest. What still needs doing is listed at thebottom.
tryLock,unlockandcloseall guarded their lock-file operations with:LOCK_FILE_NAMEis a compile-time String constant, so it is interned. The monitor is therefore theJVM-wide canonical
"lock"instance, shared with every other"lock"literal in the process — Hudi's,Spark's, or the user's. Any code that happens to synchronize on that literal blocks Hudi from acquiring or
releasing its lock, and Hudi blocks it in turn.
This is not hypothetical.
FileSystemBasedLockProviderTestClass, in this repo, declares:and synchronizes on it. Two classes with no relationship share one monitor purely because they chose the
same string.
The correct form already exists in the codebase —
KafkaConnectControlAgentusesprivate static final Object LOCK = new Object().Summary and Changelog
LOCK_FILE_MONITORobject. It is kept static, so the mutual-exclusion scope isexactly what it was; the only thing that changes is which object is used, and that it can no longer be
aliased from outside the class.
currentOwnerLockInfobecomesvolatile. It is written while holding the monitor but read through theLombok
@Getterwithout it —LockManagerreads it to report the current lock holder — so the value wasnot guaranteed visible to the reading thread.
Verification
New
testAcquisitionIsNotBlockedByTheInternedLockLiteralstarts a thread that does nothing butsynchronized ("lock") { … }, standing in for any unrelated code in the JVM, then acquires the lock fromanother thread. With the fix it acquires immediately. With
synchronized (LOCK_FILE_NAME)restored itblocks for the full budget and the test fails:
That 10-second block is the defect, reproduced.
TestFileSystemBasedLockProvider10 tests green, and the wholeorg.apache.hudi.client.transaction.**package 256 tests green — worth running in full here because
FileSystemBasedLockProviderTestClassaliasedthis monitor, so anything that had come to depend on the accidental coupling would show up there.
checkstyle:checkandapache-rat:checkclean.Still open under HUDI-9254, not in this PR
FileSystemBasedLockProviderinstances in a JVM serialise even fordifferent tables and different lock files. Narrowing it to per-instance would be correct only if
acquireLockis genuinely atomic for every supported scheme — the constructor does reject schemeswithout atomic creation, so it looks safe, but that is a behaviour change in a lock provider and wants a
maintainer's call rather than mine.
BaseZookeeperBasedLockProviderandHiveMetastoreBasedLockProviderhave sincehad their mutable fields made
volatile, which fixes visibility but not the check-then-act sequencesaround them (for example ZK's
ValidationUtils.checkArgument(this.lock == null, …)followed by creatingand assigning a new mutex).
DynamoDBBasedLockProviderstill holds its lock item as instance state.sharing one provider across threads is supported at all.
I would rather land the unambiguous fix than bundle it with changes that need a design decision. Happy to
follow up on any of the above.
Impact
No API, config or table format change. For a JVM where nothing else synchronizes on
"lock", behaviour isidentical. Where something does, Hudi's lock operations stop being blocked by it.
Risk Level
low — one monitor object swapped for a private one at the same scope, plus a
volatile. The scope isdeliberately unchanged, and the regression surface is covered by the full transaction/lock package.
Documentation Update
none
Contributor's checklist