gnodet commented on code in PR #12478:
URL: https://github.com/apache/maven/pull/12478#discussion_r3638798190
##########
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:
Fixed in 6c8005734a — separated InterruptedException handling to restore the
thread interrupt flag before falling through to the catch-all for barrier
timeout/broken exceptions.
##########
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:
Fixed in 6c8005734a — same treatment applied to supplierB.
##########
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:
Fixed in 6c8005734a — updated Javadoc to say 'sequential' instead of
'concurrent' since Thread A completes before Thread B starts.
--
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]
