rangareddy commented on code in PR #19486:
URL: https://github.com/apache/hudi/pull/19486#discussion_r3817548188
##########
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:
Taken, together with the unlock/close suggestion. Both checks are
`assertTimeoutPreemptively` now and the `ExecutorService`, `Future` and
`TimeoutException` imports are gone.
##########
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:
Good catch, that was the real gap. Both are now exercised while the monitor
is held. Confirmed it discriminates by reverting each block on its own:
`close()`, `unlock()` and `tryLock()` each fail the test individually now,
where before only `tryLock` did.
--
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]
