Copilot commented on code in PR #12478:
URL: https://github.com/apache/maven/pull/12478#discussion_r3611144507
##########
impl/maven-impl/src/test/java/org/apache/maven/impl/cache/AbstractRequestCacheTest.java:
##########
@@ -416,6 +417,125 @@ void testConcurrentRequestUnblockedOnPartialBatchResult()
throws Exception {
}
}
+ /**
+ * Tests that two concurrent {@code requests()} (batch) calls with
overlapping keys
+ * do not deadlock when they share a CachingSupplier through an
equals-based cache.
+ * <p>
+ * This is the exact regression test for issue
+ * <a href="https://github.com/apache/maven/issues/12472";>#12472</a>:
+ * Maven 4.0.0-rc-5 hangs indefinitely during multi-module builds because
two threads
+ * both call {@code requests()} with overlapping request keys. Through the
equals-based
+ * cache (old {@code SoftIdentityMap} which used {@code equals()}, not
identity), both
+ * threads get back the <b>same</b> CachingSupplier for the shared key.
+ * <p>
+ * Before the fix (commit c6de104bff), the old {@code individualSupplier}
lambda would
+ * wait on the outer call's {@code IdentityHashMap} using {@code
synchronized + wait()}.
+ * Thread B, entering the shared CachingSupplier's {@code apply()}, would
wait on
+ * that map — but since Thread B held a different set of request object
references,
+ * the identity-based map lookup could never match, creating a permanent
deadlock.
+ * <p>
+ * The fix replaced the wait-on-map pattern with {@link
CachingSupplier#complete}
+ * (direct value setting + notifyAll) and a ThreadLocal re-entrancy guard,
eliminating
+ * the cross-thread dependency cycle.
+ */
+ @Test
+ void testConcurrentBatchRequestsWithSharedKeyDoNotDeadlock() throws
Exception {
+ // Use equals-based cache: two threads will get the same
CachingSupplier for matching keys
+ CachingTestRequestCache cachingCache = new CachingTestRequestCache();
+
+ TestRequest reqOnlyA = createTestRequest("onlyA");
+ TestRequest reqOnlyB = createTestRequest("onlyB");
+ // Both threads request "shared" — different objects, but equals()
returns true
+ TestRequest sharedByA = createTestRequest("shared");
+ TestRequest sharedByB = createTestRequest("shared");
+
+ // Barrier ensures both threads are inside their batch supplier
simultaneously,
+ // maximizing the chance of the deadlock scenario
+ CyclicBarrier bothInSupplier = new CyclicBarrier(2);
+
+ Function<List<TestRequest>, List<TestResult>> supplierA = reqs -> {
+ try {
+ bothInSupplier.await(5, TimeUnit.SECONDS);
+ } catch (Exception e) {
+ // Thread B may be blocked waiting on the shared
CachingSupplier instead
+ // of reaching its supplier — that's what we're testing against
+ }
+ return reqs.stream().map(TestResult::new).toList();
+ };
+
+ Function<List<TestRequest>, List<TestResult>> supplierB = reqs -> {
+ try {
+ bothInSupplier.await(5, TimeUnit.SECONDS);
+ } catch (Exception e) {
+ // Same — Thread A may not reach the barrier
+ }
Review Comment:
Same as supplierA: catching Exception here hides InterruptedException
without restoring the interrupt flag. Restore the interrupt and only swallow
the expected barrier exceptions (timeout/broken barrier).
##########
impl/maven-impl/src/test/java/org/apache/maven/impl/cache/AbstractRequestCacheTest.java:
##########
@@ -416,6 +417,125 @@ void testConcurrentRequestUnblockedOnPartialBatchResult()
throws Exception {
}
}
+ /**
+ * Tests that two concurrent {@code requests()} (batch) calls with
overlapping keys
+ * do not deadlock when they share a CachingSupplier through an
equals-based cache.
+ * <p>
+ * This is the exact regression test for issue
+ * <a href="https://github.com/apache/maven/issues/12472";>#12472</a>:
+ * Maven 4.0.0-rc-5 hangs indefinitely during multi-module builds because
two threads
+ * both call {@code requests()} with overlapping request keys. Through the
equals-based
+ * cache (old {@code SoftIdentityMap} which used {@code equals()}, not
identity), both
+ * threads get back the <b>same</b> CachingSupplier for the shared key.
+ * <p>
+ * Before the fix (commit c6de104bff), the old {@code individualSupplier}
lambda would
+ * wait on the outer call's {@code IdentityHashMap} using {@code
synchronized + wait()}.
+ * Thread B, entering the shared CachingSupplier's {@code apply()}, would
wait on
+ * that map — but since Thread B held a different set of request object
references,
+ * the identity-based map lookup could never match, creating a permanent
deadlock.
+ * <p>
+ * The fix replaced the wait-on-map pattern with {@link
CachingSupplier#complete}
+ * (direct value setting + notifyAll) and a ThreadLocal re-entrancy guard,
eliminating
+ * the cross-thread dependency cycle.
+ */
+ @Test
+ void testConcurrentBatchRequestsWithSharedKeyDoNotDeadlock() throws
Exception {
+ // Use equals-based cache: two threads will get the same
CachingSupplier for matching keys
+ CachingTestRequestCache cachingCache = new CachingTestRequestCache();
+
+ TestRequest reqOnlyA = createTestRequest("onlyA");
+ TestRequest reqOnlyB = createTestRequest("onlyB");
+ // Both threads request "shared" — different objects, but equals()
returns true
+ TestRequest sharedByA = createTestRequest("shared");
+ TestRequest sharedByB = createTestRequest("shared");
+
+ // Barrier ensures both threads are inside their batch supplier
simultaneously,
+ // maximizing the chance of the deadlock scenario
+ CyclicBarrier bothInSupplier = new CyclicBarrier(2);
+
+ Function<List<TestRequest>, List<TestResult>> supplierA = reqs -> {
+ try {
+ bothInSupplier.await(5, TimeUnit.SECONDS);
+ } catch (Exception e) {
+ // Thread B may be blocked waiting on the shared
CachingSupplier instead
+ // of reaching its supplier — that's what we're testing against
+ }
+ return reqs.stream().map(TestResult::new).toList();
+ };
+
+ Function<List<TestRequest>, List<TestResult>> supplierB = reqs -> {
+ try {
+ bothInSupplier.await(5, TimeUnit.SECONDS);
+ } catch (Exception e) {
+ // Same — Thread A may not reach the barrier
+ }
+ return reqs.stream().map(TestResult::new).toList();
+ };
+
+ ExecutorService executor = Executors.newFixedThreadPool(2);
+ try {
+ Future<List<TestResult>> futureA =
+ executor.submit(() ->
cachingCache.requests(List.of(reqOnlyA, sharedByA), supplierA));
+ Future<List<TestResult>> futureB =
+ executor.submit(() ->
cachingCache.requests(List.of(reqOnlyB, sharedByB), supplierB));
+
+ // If the old cross-thread deadlock exists, both futures will hang
forever
+ List<TestResult> resultsA = futureA.get(10, TimeUnit.SECONDS);
+ List<TestResult> resultsB = futureB.get(10, TimeUnit.SECONDS);
+
+ assertEquals(2, resultsA.size());
+ assertEquals(2, resultsB.size());
+ } catch (TimeoutException e) {
+ throw new AssertionError(
+ "Cross-thread deadlock detected: two concurrent batch
requests() calls "
+ + "with shared keys did not complete within 10
seconds "
+ + "(regression for #12472)",
+ e);
+ } finally {
+ executor.shutdownNow();
+ }
+ }
+
+ /**
+ * Tests that two concurrent {@code requests()} calls with overlapping keys
+ * correctly deliver results to both callers, even when one thread's batch
+ * completes before the other begins.
+ * <p>
Review Comment:
The Javadoc for this test says "two concurrent requests() calls", but the
test is explicitly sequential (Thread A completes before Thread B starts).
Updating the wording avoids confusion and keeps the documentation aligned with
the scenario being asserted.
##########
impl/maven-impl/src/test/java/org/apache/maven/impl/cache/AbstractRequestCacheTest.java:
##########
@@ -416,6 +417,125 @@ void testConcurrentRequestUnblockedOnPartialBatchResult()
throws Exception {
}
}
+ /**
+ * Tests that two concurrent {@code requests()} (batch) calls with
overlapping keys
+ * do not deadlock when they share a CachingSupplier through an
equals-based cache.
+ * <p>
+ * This is the exact regression test for issue
+ * <a href="https://github.com/apache/maven/issues/12472";>#12472</a>:
+ * Maven 4.0.0-rc-5 hangs indefinitely during multi-module builds because
two threads
+ * both call {@code requests()} with overlapping request keys. Through the
equals-based
+ * cache (old {@code SoftIdentityMap} which used {@code equals()}, not
identity), both
+ * threads get back the <b>same</b> CachingSupplier for the shared key.
+ * <p>
+ * Before the fix (commit c6de104bff), the old {@code individualSupplier}
lambda would
+ * wait on the outer call's {@code IdentityHashMap} using {@code
synchronized + wait()}.
+ * Thread B, entering the shared CachingSupplier's {@code apply()}, would
wait on
+ * that map — but since Thread B held a different set of request object
references,
+ * the identity-based map lookup could never match, creating a permanent
deadlock.
+ * <p>
+ * The fix replaced the wait-on-map pattern with {@link
CachingSupplier#complete}
+ * (direct value setting + notifyAll) and a ThreadLocal re-entrancy guard,
eliminating
+ * the cross-thread dependency cycle.
+ */
+ @Test
+ void testConcurrentBatchRequestsWithSharedKeyDoNotDeadlock() throws
Exception {
+ // Use equals-based cache: two threads will get the same
CachingSupplier for matching keys
+ CachingTestRequestCache cachingCache = new CachingTestRequestCache();
+
+ TestRequest reqOnlyA = createTestRequest("onlyA");
+ TestRequest reqOnlyB = createTestRequest("onlyB");
+ // Both threads request "shared" — different objects, but equals()
returns true
+ TestRequest sharedByA = createTestRequest("shared");
+ TestRequest sharedByB = createTestRequest("shared");
+
+ // Barrier ensures both threads are inside their batch supplier
simultaneously,
+ // maximizing the chance of the deadlock scenario
+ CyclicBarrier bothInSupplier = new CyclicBarrier(2);
+
+ Function<List<TestRequest>, List<TestResult>> supplierA = reqs -> {
+ try {
+ bothInSupplier.await(5, TimeUnit.SECONDS);
+ } catch (Exception e) {
+ // Thread B may be blocked waiting on the shared
CachingSupplier instead
+ // of reaching its supplier — that's what we're testing against
+ }
Review Comment:
The barrier await swallows InterruptedException (via catch-all Exception)
without restoring the thread interrupt flag. In this test class, other
concurrency helpers restore interrupts; doing the same here avoids leaking an
interrupted status into the executor thread and makes failures easier to
diagnose.
--
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]
