voonhous commented on code in PR #19486:
URL: https://github.com/apache/hudi/pull/19486#discussion_r3773085020
##########
hudi-client/hudi-client-common/src/test/java/org/apache/hudi/client/transaction/lock/TestFileSystemBasedLockProvider.java:
##########
@@ -81,6 +86,60 @@ public void testAcquireAndReleaseLock() {
}
}
+ /**
+ * The lock-file operations used to synchronize on the {@code "lock"} String
constant, which is interned
+ * and therefore shared JVM-wide with every other {@code "lock"} literal.
Unrelated code holding that
+ * monitor blocked lock acquisition outright. {@code
FileSystemBasedLockProviderTestClass} in this very
+ * repo declares {@code static final String LOCK = "lock"} and so aliases it.
+ */
+ @Test
+ public void testAcquisitionIsNotBlockedByTheInternedLockLiteral() throws
Exception {
+ StorageConfiguration<?> storageConf =
HoodieTestUtils.getDefaultStorageConf();
+ FileSystemBasedLockProvider provider =
+ new FileSystemBasedLockProvider(lockConfiguration(lockDir("interned"),
0), storageConf);
+ CountDownLatch holding = new CountDownLatch(1);
+ CountDownLatch release = new CountDownLatch(1);
+ // stands in for any other class in the JVM doing synchronized ("lock")
+ Thread unrelated = new Thread(() -> {
+ synchronized ("lock") {
+ holding.countDown();
+ try {
+ release.await();
+ } catch (InterruptedException e) {
+ Thread.currentThread().interrupt();
+ }
+ }
+ });
+ unrelated.setDaemon(true);
+ unrelated.start();
+ assertTrue(holding.await(10, TimeUnit.SECONDS), "the unrelated thread
should hold the interned monitor");
+
+ try {
+ ExecutorService executor = Executors.newSingleThreadExecutor();
+ try {
+ Future<Boolean> acquired = executor.submit(() -> provider.tryLock(1,
TimeUnit.SECONDS));
+ boolean gotLock;
+ try {
+ gotLock = acquired.get(10, TimeUnit.SECONDS);
+ } catch (TimeoutException e) {
+ // The bare TimeoutException says nothing about why, and this is the
failure the regression
+ // produces: tryLock is parked on a monitor an unrelated thread
holds, so it never returns.
+ throw new AssertionError("tryLock never returned - it is blocked on
the monitor held by the "
+ + "unrelated thread, which means the provider is synchronizing
on the interned \"lock\" "
+ + "literal again rather than on a private monitor", e);
+ }
+ assertTrue(gotLock,
+ "acquisition must not wait on a monitor held by code that has
nothing to do with Hudi");
+ } finally {
+ executor.shutdownNow();
+ }
+ } finally {
+ release.countDown();
+ provider.unlock();
+ provider.close();
Review Comment:
The test guards `tryLock` only, not the other two blocks this PR converts.
`release.countDown()` runs before `provider.unlock()` and
`provider.close()`, so neither ever executes while the unrelated thread holds
the interned monitor. Restore `synchronized (LOCK_FILE_NAME)` in only
`unlock()` (L160) or only `close()` (L115) and this test still passes. Three
blocks converted, one covered.
Put the release path through the same gate before dropping the latch:
```suggestion
assertTrue(gotLock,
"acquisition must not wait on a monitor held by code that has
nothing to do with Hudi");
// unlock() and close() take the same monitor, so exercise them
while it is still held.
Future<?> released = executor.submit(() -> {
provider.unlock();
provider.close();
});
try {
released.get(10, TimeUnit.SECONDS);
} catch (TimeoutException e) {
throw new AssertionError("unlock/close never returned - they are
blocked on the monitor held "
+ "by the unrelated thread, which means unlock()/close() are
synchronizing on the interned "
+ "\"lock\" literal again rather than on a private monitor",
e);
}
} finally {
executor.shutdownNow();
}
} finally {
release.countDown();
provider.unlock();
provider.close();
```
The repeated `unlock()`/`close()` in the outer finally stays safe:
`testUnlockWithoutLockIsNoOp` already covers both being no-ops when no lock
file is present.
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/client/transaction/lock/FileSystemBasedLockProvider.java:
##########
@@ -63,14 +63,29 @@
@Slf4j
public class FileSystemBasedLockProvider implements LockProvider<String>,
Serializable {
private static final String LOCK_FILE_NAME = "lock";
+ /**
+ * Guards this provider's lock-file operations.
+ *
+ * <p>These blocks used to synchronize on {@link #LOCK_FILE_NAME}. That is a
compile-time String constant,
+ * so it is interned: any class anywhere in the JVM that synchronizes on the
same {@code "lock"} literal
+ * contends on the very same monitor and silently couples itself to Hudi's
lock acquisition. A private
+ * object cannot be aliased that way.
+ *
+ * <p>Kept static so the mutual-exclusion scope is unchanged by this fix.
Review Comment:
"the mutual-exclusion scope is unchanged" is not quite true, and the
exception is reachable.
An interned literal lives in the JVM-wide string table and is shared across
every classloader (JLS 3.10.5); a `static final Object` is per loaded class.
Where two copies of this class are loaded, the old monitor was shared and the
new one is not.
That configuration exists in practice: `StreamerUtil.java:235-241` installs
this provider as Flink's default when a lock is required and none is
configured, and a Flink session cluster gives each job its own user-code
classloader inside one TaskManager JVM.
This is not a correctness regression for supported deployments, since the
atomic create on storage is the real mutual exclusion and `acquireLock()`
handles the loser through `FileAlreadyExistsException` -> `HoodieIOException`
-> `return false` at L152. But this comment is what the next reader will trust,
so it should state what is actually guaranteed:
```suggestion
* <p>Kept static so that, within a classloader, the mutual-exclusion
scope is unchanged by this fix.
* The interned literal was additionally shared across classloaders; that
was never a correctness
* guarantee, since the atomic create on storage is the real mutual
exclusion.
```
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/client/transaction/lock/FileSystemBasedLockProvider.java:
##########
@@ -63,14 +63,29 @@
@Slf4j
public class FileSystemBasedLockProvider implements LockProvider<String>,
Serializable {
private static final String LOCK_FILE_NAME = "lock";
+ /**
+ * Guards this provider's lock-file operations.
+ *
+ * <p>These blocks used to synchronize on {@link #LOCK_FILE_NAME}. That is a
compile-time String constant,
+ * so it is interned: any class anywhere in the JVM that synchronizes on the
same {@code "lock"} literal
+ * contends on the very same monitor and silently couples itself to Hudi's
lock acquisition. A private
+ * object cannot be aliased that way.
+ *
+ * <p>Kept static so the mutual-exclusion scope is unchanged by this fix.
+ */
+ private static final Object LOCK_FILE_MONITOR = new Object();
private final int lockTimeoutMinutes;
private final transient HoodieStorage storage;
private final transient StoragePath lockFile;
protected LockConfiguration lockConfiguration;
private final SimpleDateFormat sdf;
private final LockInfo lockInfo;
+ /**
+ * Written while holding {@link #LOCK_FILE_MONITOR} in {@code tryLock}, but
read through the generated
+ * getter without it, so the read needs to be volatile for the value to be
visible to other threads.
+ */
@Getter
- private String currentOwnerLockInfo;
+ private volatile String currentOwnerLockInfo;
Review Comment:
Three notes on the "Still open under HUDI-9254" section, since that list is
what the next person will work from.
1. "`BaseZookeeperBasedLockProvider` and `HiveMetastoreBasedLockProvider`
**have since** had their mutable fields made `volatile`" -- they were volatile
in their original commits, not added later. `git show 74241947c123 --
'*ZookeeperBasedLockProvider.java'` (HUDI-845, #2374) introduces `private
volatile InterProcessMutex lock = null;`, and `git show d7b18783bdd6 --
'*HiveMetastoreBasedLockProvider.java'` introduces `private volatile
LockResponse lock = null;`. The check-then-act point is correct; only the
history is wrong.
2. "`DynamoDBBasedLockProvider` still holds its lock item as instance state"
-- true, but that field is volatile too
(`DynamoDBBasedLockProviderBase.java:80`, `protected volatile LockItem lock`),
so the contrast drawn with ZK and Hive does not hold. Its defect is the same
check-then-act shape, not visibility.
3. Worth adding to punted item (a): the reason the monitor has to stay
static is the expiry path, not the create path. Two threads can both evaluate
`checkIfExpired()` true at L141, both call `deleteFile` at L142, and the second
delete removes the lock file the first just created, leaving both to return
true from `storage.exists` at L150. Stating that converts the hedge into an
invariant and tells whoever narrows the scope later exactly what they have to
prove.
##########
hudi-client/hudi-client-common/src/test/java/org/apache/hudi/client/transaction/lock/TestFileSystemBasedLockProvider.java:
##########
@@ -81,6 +86,60 @@ public void testAcquireAndReleaseLock() {
}
}
+ /**
+ * The lock-file operations used to synchronize on the {@code "lock"} String
constant, which is interned
+ * and therefore shared JVM-wide with every other {@code "lock"} literal.
Unrelated code holding that
+ * monitor blocked lock acquisition outright. {@code
FileSystemBasedLockProviderTestClass} in this very
+ * repo declares {@code static final String LOCK = "lock"} and so aliases it.
+ */
+ @Test
+ public void testAcquisitionIsNotBlockedByTheInternedLockLiteral() throws
Exception {
+ StorageConfiguration<?> storageConf =
HoodieTestUtils.getDefaultStorageConf();
+ FileSystemBasedLockProvider provider =
+ new FileSystemBasedLockProvider(lockConfiguration(lockDir("interned"),
0), storageConf);
+ CountDownLatch holding = new CountDownLatch(1);
+ CountDownLatch release = new CountDownLatch(1);
+ // stands in for any other class in the JVM doing synchronized ("lock")
+ Thread unrelated = new Thread(() -> {
+ synchronized ("lock") {
+ holding.countDown();
+ try {
+ release.await();
+ } catch (InterruptedException e) {
+ Thread.currentThread().interrupt();
+ }
+ }
+ });
+ unrelated.setDaemon(true);
+ unrelated.start();
+ assertTrue(holding.await(10, TimeUnit.SECONDS), "the unrelated thread
should hold the interned monitor");
+
+ try {
Review Comment:
If the assertion on L115 fails, the interned monitor is never released.
`unrelated.start()` and the `holding.await` assertion sit outside the `try`
whose `finally` calls `release.countDown()`. On assertion failure the daemon
thread still enters `synchronized ("lock")` and parks on `release.await()`
forever, holding the JVM-wide monitor for the remaining life of the fork --
surefire runs `forkCount=1` / `reuseForks=true` (pom.xml:2212-2213). That turns
one red test into a hung module.
Move the start and the await inside the try so the latch always drops:
```suggestion
unrelated.setDaemon(true);
try {
unrelated.start();
assertTrue(holding.await(10, TimeUnit.SECONDS), "the unrelated thread
should hold the interned monitor");
```
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/client/transaction/lock/FileSystemBasedLockProvider.java:
##########
@@ -63,14 +63,29 @@
@Slf4j
public class FileSystemBasedLockProvider implements LockProvider<String>,
Serializable {
private static final String LOCK_FILE_NAME = "lock";
+ /**
+ * Guards this provider's lock-file operations.
+ *
+ * <p>These blocks used to synchronize on {@link #LOCK_FILE_NAME}. That is a
compile-time String constant,
+ * so it is interned: any class anywhere in the JVM that synchronizes on the
same {@code "lock"} literal
+ * contends on the very same monitor and silently couples itself to Hudi's
lock acquisition. A private
+ * object cannot be aliased that way.
+ *
+ * <p>Kept static so the mutual-exclusion scope is unchanged by this fix.
+ */
+ private static final Object LOCK_FILE_MONITOR = new Object();
private final int lockTimeoutMinutes;
private final transient HoodieStorage storage;
private final transient StoragePath lockFile;
protected LockConfiguration lockConfiguration;
private final SimpleDateFormat sdf;
private final LockInfo lockInfo;
+ /**
+ * Written while holding {@link #LOCK_FILE_MONITOR} in {@code tryLock}, but
read through the generated
+ * getter without it, so the read needs to be volatile for the value to be
visible to other threads.
+ */
Review Comment:
Two small things about this `volatile`.
The stated reason does not hold in-repo: both readers run on the same thread
that called `tryLock`. `LockManager.lock()` (L76-90) goes through
`RetryHelper.start`, which invokes `func.get()` inline (`RetryHelper.java:92`,
no executor), and `TimeGeneratorBase` has the same shape. `git grep
getCurrentOwnerLockInfo` returns only those two call sites plus the interface
default and one test. The defensible justification is that
`getCurrentOwnerLockInfo()` is public `LockProvider` API and a plugged-in
caller may read it from another thread, which is a fine reason to keep the
modifier, just not the one written here.
Second, `volatile` on a private instance field changes the class's default
`serialVersionUID`, and this class is `Serializable` (L64) with no explicit
UID. Compiling the same class name with and without the modifier:
```
baseline 4315981554152328805
+ private static final Object field 4315981554152328805 (private static
is excluded from the hash)
+ volatile on the instance field 7607095523244839677
```
That only bites on mixed-version deserialization, and the provider's
`storage`/`lockFile` are transient anyway, so I would not change the code.
Please reword the Javadoc to the public-API reason, and add a line to the PR
description noting the UID change so it is not a surprise later.
##########
hudi-client/hudi-client-common/src/test/java/org/apache/hudi/client/transaction/lock/TestFileSystemBasedLockProvider.java:
##########
@@ -81,6 +86,60 @@ public void testAcquireAndReleaseLock() {
}
}
+ /**
+ * The lock-file operations used to synchronize on the {@code "lock"} String
constant, which is interned
+ * and therefore shared JVM-wide with every other {@code "lock"} literal.
Unrelated code holding that
+ * monitor blocked lock acquisition outright. {@code
FileSystemBasedLockProviderTestClass} in this very
+ * repo declares {@code static final String LOCK = "lock"} and so aliases it.
+ */
+ @Test
+ public void testAcquisitionIsNotBlockedByTheInternedLockLiteral() throws
Exception {
+ StorageConfiguration<?> storageConf =
HoodieTestUtils.getDefaultStorageConf();
+ FileSystemBasedLockProvider provider =
+ new FileSystemBasedLockProvider(lockConfiguration(lockDir("interned"),
0), storageConf);
+ CountDownLatch holding = new CountDownLatch(1);
+ CountDownLatch release = new CountDownLatch(1);
+ // stands in for any other class in the JVM doing synchronized ("lock")
+ Thread unrelated = new Thread(() -> {
+ synchronized ("lock") {
Review Comment:
nit, feel free to ignore: after this PR the test helper is the only `"lock"`
monitor left in the tree.
`FileSystemBasedLockProviderTestClass.java:45` declares `private static
final String LOCK = "lock"`, synchronizes on it at :64/:77/:95 and calls
`LOCK.wait(retryWaitTimeMs)` at :79. It is no longer a hazard to production now
that this provider has its own monitor, but it is the same defect, and this
test deliberately adds a second aliaser.
Splitting that field into `private static final Object LOCK = new Object()`
for the monitor and a separate `LOCK_FILE_NAME` string for the path removes the
bug class from the repo entirely. Worth folding into this PR since it is the
same fix; a follow-up is fine too.
##########
hudi-client/hudi-client-common/src/test/java/org/apache/hudi/client/transaction/lock/TestFileSystemBasedLockProvider.java:
##########
@@ -81,6 +86,60 @@ public void testAcquireAndReleaseLock() {
}
}
+ /**
+ * The lock-file operations used to synchronize on the {@code "lock"} String
constant, which is interned
+ * and therefore shared JVM-wide with every other {@code "lock"} literal.
Unrelated code holding that
+ * monitor blocked lock acquisition outright. {@code
FileSystemBasedLockProviderTestClass} in this very
+ * repo declares {@code static final String LOCK = "lock"} and so aliases it.
Review Comment:
nit, feel free to ignore: this is the reference you agreed to drop from the
production Javadoc.
The `FileSystemBasedLockProviderTestClass` name and its `static final String
LOCK = "lock"` detail survive here after being removed from
`FileSystemBasedLockProvider`. Same rot risk, and because it is `{@code}`
rather than `{@link}`, a rename of that class fails silently instead of
breaking the build.
Either drop the last sentence and keep the mechanism description, or write
it as `{@link
org.apache.hudi.client.transaction.FileSystemBasedLockProviderTestClass}` so a
rename is caught at compile time.
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/client/transaction/lock/FileSystemBasedLockProvider.java:
##########
@@ -120,7 +135,7 @@ public void close() {
@Override
public boolean tryLock(long time, TimeUnit unit) {
try {
- synchronized (LOCK_FILE_NAME) {
+ synchronized (LOCK_FILE_MONITOR) {
Review Comment:
Adjacent pre-existing bug in the method this `volatile` publishes.
`reloadCurrentOwnerLockInfo()` (L206-216) evaluates
`storage.open(this.lockFile)` in the try-with-resources header, before the
`storage.exists` check on L208. `HoodieHadoopStorage.open` is `fs.open(path)`,
which throws `FileNotFoundException` on a missing path, so the `else {
this.currentOwnerLockInfo = ""; }` on L211 is dead code: a vanished lock file
throws instead of resetting the field. The exception is swallowed by this
method's caller at L152, the field keeps the previous owner's payload, and
`LockManager.java:83` then prints it as "Current lock owner information".
Making the field volatile publishes that stale value more reliably rather than
fixing it.
This is the surviving half of a two-part regression: `5faefcd01fa8` ([MINOR]
#10411) hoisted both `fs.create` and `fs.open` above their guards, and #19222
fixed only the `acquireLock` half.
Either fix it here while you are in the file:
```java
public void reloadCurrentOwnerLockInfo() {
try {
if (!storage.exists(this.lockFile)) {
this.currentOwnerLockInfo = "";
return;
}
try (InputStream is = storage.open(this.lockFile)) {
this.currentOwnerLockInfo = FileIOUtils.readAsUTFString(is);
}
} catch (IOException e) {
throw new
HoodieIOException(generateLogStatement(LockState.FAILED_TO_ACQUIRE), e);
}
}
```
or open a separate issue for it and link that from the "Still open" list.
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/client/transaction/lock/FileSystemBasedLockProvider.java:
##########
@@ -63,14 +63,29 @@
@Slf4j
public class FileSystemBasedLockProvider implements LockProvider<String>,
Serializable {
private static final String LOCK_FILE_NAME = "lock";
+ /**
+ * Guards this provider's lock-file operations.
+ *
+ * <p>These blocks used to synchronize on {@link #LOCK_FILE_NAME}. That is a
compile-time String constant,
+ * so it is interned: any class anywhere in the JVM that synchronizes on the
same {@code "lock"} literal
+ * contends on the very same monitor and silently couples itself to Hudi's
lock acquisition. A private
+ * object cannot be aliased that way.
+ *
+ * <p>Kept static so the mutual-exclusion scope is unchanged by this fix.
+ */
+ private static final Object LOCK_FILE_MONITOR = new Object();
Review Comment:
The commit message trailer will auto-close #16943 on merge.
`git log -1 --format=%B` ends with `Closes #16943`, but the PR description
says "Part of #16943 ... this PR fixes one concrete, self-contained defect ...
and deliberately leaves the rest". #16943 is open, labelled
`priority:critical`, and covers five providers plus the close-during-unlock
semantics.
Please reword the trailer to `Part of #16943`. GitHub only auto-closes on
`Closes`/`Fixes`/`Resolves`, so that keeps the issue open for the remaining
work.
##########
hudi-client/hudi-client-common/src/test/java/org/apache/hudi/client/transaction/lock/TestFileSystemBasedLockProvider.java:
##########
@@ -81,6 +86,60 @@ public void testAcquireAndReleaseLock() {
}
}
+ /**
+ * The lock-file operations used to synchronize on the {@code "lock"} String
constant, which is interned
+ * and therefore shared JVM-wide with every other {@code "lock"} literal.
Unrelated code holding that
+ * monitor blocked lock acquisition outright. {@code
FileSystemBasedLockProviderTestClass} in this very
+ * repo declares {@code static final String LOCK = "lock"} and so aliases it.
+ */
+ @Test
+ public void testAcquisitionIsNotBlockedByTheInternedLockLiteral() throws
Exception {
+ StorageConfiguration<?> storageConf =
HoodieTestUtils.getDefaultStorageConf();
+ FileSystemBasedLockProvider provider =
+ new FileSystemBasedLockProvider(lockConfiguration(lockDir("interned"),
0), storageConf);
+ CountDownLatch holding = new CountDownLatch(1);
+ CountDownLatch release = new CountDownLatch(1);
+ // stands in for any other class in the JVM doing synchronized ("lock")
+ Thread unrelated = new Thread(() -> {
+ synchronized ("lock") {
+ holding.countDown();
+ try {
+ release.await();
+ } catch (InterruptedException e) {
+ Thread.currentThread().interrupt();
+ }
+ }
+ });
+ unrelated.setDaemon(true);
+ unrelated.start();
+ assertTrue(holding.await(10, TimeUnit.SECONDS), "the unrelated thread
should hold the interned monitor");
+
+ try {
+ ExecutorService executor = Executors.newSingleThreadExecutor();
Review Comment:
nit, feel free to ignore: this executor plumbing is
`assertTimeoutPreemptively`.
L118-135 hand-roll what `Assertions.assertTimeoutPreemptively(Duration,
ThrowingSupplier<T>, String)` already does, custom failure message included,
and that is the existing idiom for "must not block" in this package
(`TestZookeeperBasedLockProvider.java:213`). It is present and not deprecated
in the pinned JUnit 5.14.1.
If you take the unlock/close suggestion below, this collapses to two
`assertTimeoutPreemptively` calls and lets you drop the `ExecutorService`,
`Future` and `TimeoutException` imports. Entirely optional, the current code is
correct as written.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
