You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@maven.apache.org by "gnodet (via GitHub)" <gi...@apache.org> on 2026/07/13 07:14:25 UTC
[PR] Add cross-thread deadlock regression test for #12472 [maven]
gnodet opened a new pull request, #12478:
URL: https://github.com/apache/maven/pull/12478
## Summary
- Adds regression tests for the cross-thread deadlock in `AbstractRequestCache` that caused Maven 4.0.0-rc-5 to hang indefinitely on 24 out of 966 tested Apache projects ([#12472](https://github.com/apache/maven/issues/12472))
- The root cause was a deadlock when two threads both called `requests()` (batch) with overlapping keys through an equals-based cache (`SoftIdentityMap`), sharing the same `CachingSupplier` instance. Thread B would wait on Thread A's `IdentityHashMap` via the old `individualSupplier`, but the identity-based lookup could never match Thread B's request references, creating a permanent deadlock.
- The fix in c6de104bff replaced the wait-on-map pattern with `CachingSupplier.complete()` + ThreadLocal re-entrancy guard. These tests ensure the fix cannot regress.
### New tests
| Test | Scenario |
|------|----------|
| `testConcurrentBatchRequestsWithSharedKeyDoNotDeadlock` | Two threads simultaneously execute batch `requests()` with overlapping keys, using a `CyclicBarrier` to force concurrent execution inside the batch supplier |
| `testSequentialBatchRequestsWithSharedKeyReuseResult` | Sequential batch calls with shared keys correctly reuse cached results without redundant resolution |
## Test plan
- [x] All 10 `AbstractRequestCacheTest` tests pass (including 2 new ones)
- [x] Checkstyle passes
- [ ] CI build succeeds
🤖 Generated with [Claude Code](https://claude.com/claude-code)
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add cross-thread deadlock regression test for #12472 [maven]
Posted by "gnodet (via GitHub)" <gi...@apache.org>.
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add cross-thread deadlock regression test for #12472 [maven]
Posted by "gnodet (via GitHub)" <gi...@apache.org>.
gnodet commented on PR #12478:
URL: https://github.com/apache/maven/pull/12478#issuecomment-5058398170
Thanks for the thorough empirical verification @ascheman! You're absolutely right — the existing concurrent test doesn't actually deadlock on the pre-fix code because the keys remain identity-stable throughout resolution.
I've added the mutable-hashCode variant you suggested in 3079c63881:
- **New test:** `testConcurrentBatchRequestsWithMutatingSharedKeyDoNotDeadlock` — uses `MutableHashCodeRequest` instances whose `equals()`/`hashCode()` change during batch resolution (mirroring mutable `RequestTrace` data). Thread A's supplier sleeps 1.5s then mutates trace data; Thread B's supplier also mutates. Pre-fix, this deadlocks because Thread B waits on Thread A's equals-based map for a key that no longer matches.
- **New helper:** `GenericCachingTestRequestCache` — equals-based cache that accepts any `Request` type (needed since `MutableHashCodeRequest` doesn't implement `TestRequest`).
The existing identity-stable test is kept as a complement — it still validates concurrent batch resolution doesn't deadlock, even if it doesn't specifically bite on the pre-fix regression.
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add cross-thread deadlock regression test for #12472 [maven]
Posted by "ascheman (via GitHub)" <gi...@apache.org>.
ascheman commented on PR #12478:
URL: https://github.com/apache/maven/pull/12478#issuecomment-5061282185
Thanks @gnodet — glad the variant was useful, and the implementation looks spot-on. `GenericCachingTestRequestCache` is the right call for accepting a non-`TestRequest` key type, and mutating the trace data mid-batch in both suppliers reproduces exactly the equals-based-map contention that #12472 was about. Keeping the identity-stable test as a complement makes sense too — it guards the concurrent path even though it doesn't bite the specific regression. This now genuinely guards #12472; nothing further from my side.
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add cross-thread deadlock regression test for #12472 [maven]
Posted by "ascheman (via GitHub)" <gi...@apache.org>.
ascheman commented on PR #12478:
URL: https://github.com/apache/maven/pull/12478#issuecomment-5011044438
I tried to verify this PR empirically, and I think the two new tests don't actually guard against #12472:
**Method**: checked out a pre-fix tree (`AbstractRequestCache` still with the old `individualSupplier` wait-on-map pattern, i.e. before c6de104bff) and ran the exact tests from this PR against it.
| Test | pre-fix | PR head |
|---|---|---|
| `testConcurrentBatchRequestsWithSharedKeyDoNotDeadlock` / `testSequentialBatchRequestsWithSharedKeyReuseResult` | **pass** (4/4 runs, ~0.5s — no hang) | pass |
| variant with mutating `equals()`/`hashCode()` (below) | **deadlock** (10s timeout) | pass (1.5s) |
So the pure identity-mismatch scenario as constructed here doesn't deadlock pre-fix. The ingredient that actually reproduces the #12472 hang is a shared key whose `equals()`/`hashCode()` change **during** batch resolution (mirroring mutable `RequestTrace` data in real requests): pre-fix, thread B waits on thread A's equals-based map for a key that no longer matches — forever. Good news: the variant passes on this PR's head, so the c6de104bff fix itself holds; it's only the regression guard that doesn't bite.
Suggest replacing (or complementing) the concurrent test with this variant:
```java
/**
* Variant of the #12472 regression scenario where the shared request's
* equals()/hashCode() change during batch resolution (mirroring mutable
* RequestTrace data in real requests). Pre-fix, thread B waits on thread A's
* equals-based nonCachedResults map for a key that no longer matches — forever.
*/
@Test
void testConcurrentBatchRequestsWithMutatingSharedKeyDoNotDeadlock() throws Exception {
GenericCachingTestRequestCache cachingCache = new GenericCachingTestRequestCache();
MutableHashCodeRequest reqOnlyA = new MutableHashCodeRequest("onlyA", "trace");
MutableHashCodeRequest reqOnlyB = new MutableHashCodeRequest("onlyB", "trace");
MutableHashCodeRequest sharedByA = new MutableHashCodeRequest("shared", "trace");
MutableHashCodeRequest sharedByB = new MutableHashCodeRequest("shared", "trace");
CountDownLatch aInBatch = new CountDownLatch(1);
Function<List<MutableHashCodeRequest>, List<MutableHashCodeResult>> supplierA = reqs -> {
aInBatch.countDown();
try {
Thread.sleep(1500);
} catch (InterruptedException e) {
Thread.currentThread().interrupt();
}
for (MutableHashCodeRequest r : reqs) {
r.setTraceData(r.getTraceData() + "-A");
}
return reqs.stream().map(MutableHashCodeResult::new).toList();
};
Function<List<MutableHashCodeRequest>, List<MutableHashCodeResult>> supplierB = reqs -> {
for (MutableHashCodeRequest r : reqs) {
r.setTraceData(r.getTraceData() + "-B");
}
return reqs.stream().map(MutableHashCodeResult::new).toList();
};
ExecutorService executor = Executors.newFixedThreadPool(2);
try {
Future<List<MutableHashCodeResult>> futureA =
executor.submit(() -> cachingCache.requests(List.of(reqOnlyA, sharedByA), supplierA));
assertTrue(aInBatch.await(5, TimeUnit.SECONDS), "Thread A should have entered its batch supplier");
Future<List<MutableHashCodeResult>> futureB =
executor.submit(() -> cachingCache.requests(List.of(reqOnlyB, sharedByB), supplierB));
List<MutableHashCodeResult> resultsA = futureA.get(10, TimeUnit.SECONDS);
List<MutableHashCodeResult> resultsB = futureB.get(10, TimeUnit.SECONDS);
assertEquals(2, resultsA.size());
assertEquals(2, resultsB.size());
} catch (TimeoutException e) {
throw new AssertionError(
"Cross-thread deadlock detected: batch requests() with a shared key whose "
+ "equals()/hashCode() mutate during resolution did not complete (#12472)",
e);
} finally {
executor.shutdownNow();
}
}
```
It needs two small helpers (a `MutableHashCodeRequest` whose `hashCode()` includes the mutable trace data, and an equals-based generic test cache). **Offer: we can push the complete, verified change (test + helpers) directly to this branch** — just say the word.
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add cross-thread deadlock regression test for #12472 [maven]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #12478:
URL: https://github.com/apache/maven/pull/12478#issuecomment-5061862717
@gnodet Please assign appropriate label to PR according to the type of change.
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add cross-thread deadlock regression test for #12472 [maven]
Posted by "gnodet (via GitHub)" <gi...@apache.org>.
gnodet merged PR #12478:
URL: https://github.com/apache/maven/pull/12478
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add cross-thread deadlock regression test for #12472 [maven]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org