noy-solvin commented on code in PR #19826:
URL: https://github.com/apache/pinot/pull/19826#discussion_r4265620781
##########
pinot-core/src/test/java/org/apache/pinot/core/data/manager/BaseTableDataManagerTest.java:
##########
@@ -515,6 +520,112 @@ public void testReloadSegmentForceDownload()
assertEquals(segmentMetadata.getTotalDocs(), 5);
}
+ @Test
+ public void testReloadSegmentAcquiresSegmentLockBeforeSemaphore()
+ throws Exception {
+ TestSegmentReloadSemaphore reloadSemaphore = new
TestSegmentReloadSemaphore(1);
+ BaseTableDataManager tableDataManager =
createTableManager(reloadSemaphore);
+
+ Lock segmentLock = tableDataManager.getSegmentLock(SEGMENT_NAME);
+ segmentLock.lock();
+ ExecutorService executor = Executors.newCachedThreadPool();
+ Thread reloadThread = new Thread(() -> {
+ try {
+ tableDataManager.reloadSegment(SEGMENT_NAME, new IndexLoadingConfig(),
mock(SegmentZKMetadata.class),
+ mock(SegmentMetadata.class), false);
+ } catch (Exception e) {
+ throw new RuntimeException(e);
+ }
+ }, "test-reload-thread");
+
+ try {
+ reloadThread.start();
+ TestUtils.waitForCondition(aVoid -> ((ReentrantLock)
segmentLock).hasQueuedThread(reloadThread), 10, 5000,
+ "Reload thread did not wait on segment lock");
+
+ // Verify lock acquisition ordering:
+ // In refresh (doReplaceSegment), segmentLock is acquired first, then
the reload semaphore.
+ // To avoid AB-BA deadlock between refresh and reload on the same
segment, reloadSegment must also acquire
+ // segmentLock before acquiring the reload semaphore.
+ // Under unpatched code (the bug), reloadSegment acquires the semaphore
before waiting on segmentLock,
+ // exhausting permits (availablePermits == 0) and deadlocking any
concurrent refresh.
+ // Under patched code, reloadSegment blocks on segmentLock first,
leaving the semaphore available (1 permit).
+ assertEquals(reloadSemaphore.availablePermits(), 1,
+ "Reload thread must not acquire the reload semaphore before
acquiring segment lock");
+
+ // Verify that another operation (simulating refresh holding
segmentLock) can acquire the semaphore
+ // without deadlocking.
+ Future<Boolean> refreshSemaphoreAcquire = executor.submit(() -> {
+ reloadSemaphore.acquire(SEGMENT_NAME, tableDataManager.getLogger());
+ reloadSemaphore.release();
+ return true;
+ });
+ assertTrue(refreshSemaphoreAcquire.get(2, TimeUnit.SECONDS),
+ "Thread holding segment lock must be able to acquire reload
semaphore without deadlocking");
+ } finally {
+ segmentLock.unlock();
+ reloadThread.join(5000);
+ executor.shutdownNow();
+ }
+ }
+
+ @Test
+ public void testReloadSegmentInterruptedDuringSemaphoreAcquire()
+ throws Exception {
+ TestSegmentReloadSemaphore reloadSemaphore = new
TestSegmentReloadSemaphore(1);
+ BaseTableDataManager tableDataManager =
createTableManager(reloadSemaphore);
+
+ // Acquire the only permit so that reloadSegment blocks when attempting to
acquire the semaphore.
+ reloadSemaphore.acquire("otherSegment", tableDataManager.getLogger());
+ assertEquals(reloadSemaphore.availablePermits(), 0);
+
+ AtomicBoolean caughtInterruptedException = new AtomicBoolean(false);
+ Thread reloadThread = new Thread(() -> {
+ try {
+ tableDataManager.reloadSegment(SEGMENT_NAME, new IndexLoadingConfig(),
mock(SegmentZKMetadata.class),
+ mock(SegmentMetadata.class), false);
+ } catch (InterruptedException e) {
+ caughtInterruptedException.set(true);
+ } catch (Exception e) {
+ if (e.getCause() instanceof InterruptedException) {
+ caughtInterruptedException.set(true);
+ }
+ }
+ }, "test-reload-interrupted-thread");
+
+ try {
+ reloadThread.start();
+ // Wait until reloadThread is blocked waiting to acquire the semaphore.
+ TestUtils.waitForCondition(
+ aVoid -> reloadThread.getState() == Thread.State.WAITING &&
reloadSemaphore.isAcquireAttempted(), 10, 5000,
+ "Reload thread did not wait on reload semaphore");
+
+ // Interrupt reloadThread while it is waiting in acquire()
+ reloadThread.interrupt();
+ reloadThread.join(5000);
+ assertTrue(caughtInterruptedException.get(), "Reload thread should have
caught InterruptedException");
+
+ // Verify exception safety:
+ // 1. segmentLock must NOT be leaked (must be unlocked)
+ Lock segmentLock = tableDataManager.getSegmentLock(SEGMENT_NAME);
Review Comment:
Thank you for the thorough review and catch. Updated
`testReloadSegmentInterruptedDuringSemaphoreAcquire` in
`pinot-core/src/test/java/org/apache/pinot/core/data/manager/BaseTableDataManagerTest.java`
to pre-capture `Lock segmentLock =
tableDataManager.getSegmentLock(SEGMENT_NAME);` on the main test thread
immediately after initializing `tableDataManager` and prior to starting
`reloadThread`. Holding this strong reference on the test thread stack ensures
the lock entry in `SegmentLocks` (which uses a weak-value cache) is not evicted
upon GC when `reloadThread` exits, guaranteeing that post-join assertions
evaluate the exact lock instance rather than a newly allocated lock. Verified
with dynamic unit tests, style checks, and full regression test suites passing
cleanly.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
