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/08 16:07:17 UTC
[PR] Fix #12445: Fix deadlock in AbstractRequestCache when resolving parent POMs [maven]
gnodet opened a new pull request, #12446:
URL: https://github.com/apache/maven/pull/12446
## Summary
- Fix deadlock in `AbstractRequestCache.requests()` that caused Maven 4 to hang indefinitely when building with an empty local repository (e.g., `-Dmaven.repo.local=/tmp/fresh-repo`)
- Replace the `Object.wait()/notify()` pattern with direct value setting via `CachingSupplier.complete()`, eliminating the re-entrant deadlock scenario
- Add tests for the re-entrant scenario and batch caching behavior
## Root Cause
The `requests()` method used a wait/notify pattern with a local `HashMap` (`nonCachedResults`):
1. An `individualSupplier` lambda was created that called `nonCachedResults.wait()`, expecting the batch resolution to populate results and call `notifyAll()`
2. This `individualSupplier` was stored inside `CachingSupplier` instances via `doCache()`, which cached them by request key
3. When a re-entrant `requests()` call occurred (e.g., parent POM resolution during artifact resolution), `doCache()` returned a `CachingSupplier` from the outer call — wrapping the outer call's `individualSupplier`
4. The inner call's collection loop invoked this `CachingSupplier`, which waited on the outer call's `nonCachedResults` — a `HashMap` that could never be populated because the outer batch supplier was blocked waiting for the inner call to complete
## Fix
- Added `CachingSupplier.complete(Object result)` to directly set cached values without invoking the supplier
- Changed `requests()` to pass a single-item fallback supplier to `doCache()` (instead of the wait-based `individualSupplier`), so any re-entrant retrieval can resolve independently
- After batch resolution, results are set directly on `CachingSupplier` instances via `complete()`
- Removed the `synchronized` wait/notify pattern on `HashMap` entirely
## Test plan
- [x] Added `testReentrantRequestsDoesNotDeadlock` — reproduces the exact deadlock scenario (with 5-second timeout detection)
- [x] Added `testBatchResultsAreCached` — verifies batch results are properly cached for subsequent calls
- [x] All 499 existing tests in `maven-impl` pass
- [x] Full reactor build (`mvn clean install -DskipTests`) 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] Fix #12445: Fix deadlock in AbstractRequestCache when resolving parent POMs [maven]
Posted by "gnodet (via GitHub)" <gi...@apache.org>.
gnodet merged PR #12446:
URL: https://github.com/apache/maven/pull/12446
--
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] Fix #12445: Fix deadlock in AbstractRequestCache when resolving parent POMs [maven]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #12446:
URL: https://github.com/apache/maven/pull/12446#issuecomment-4947842874
@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] Fix #12445: Fix deadlock in AbstractRequestCache when resolving parent POMs [maven]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #12446:
URL: https://github.com/apache/maven/pull/12446#discussion_r3546143192
##########
impl/maven-impl/src/main/java/org/apache/maven/impl/cache/AbstractRequestCache.java:
##########
@@ -85,57 +120,82 @@ public <REQ extends Request<?>, REP extends Result<REQ>> REP request(REQ req, Fu
@SuppressWarnings("unchecked")
public <REQ extends Request<?>, REP extends Result<REQ>> List<REP> requests(
List<REQ> reqs, Function<List<REQ>, List<REP>> supplier) {
- final Map<REQ, Object> nonCachedResults = new HashMap<>();
- List<RequestResult<REQ, REP>> allResults = new ArrayList<>(reqs.size());
-
- Function<REQ, REP> individualSupplier = req -> {
- synchronized (nonCachedResults) {
- while (!nonCachedResults.containsKey(req)) {
- try {
- nonCachedResults.wait();
- } catch (InterruptedException e) {
- Thread.currentThread().interrupt();
- throw new RuntimeException(e);
- }
- }
- Object val = nonCachedResults.get(req);
- if (val instanceof CachingSupplier.AltRes altRes) {
- uncheckedThrow(altRes.throwable);
- }
- return (REP) val;
- }
- };
+ // Create a fallback supplier that can resolve individual requests independently.
+ // This is stored in newly created CachingSupplier instances and used only when
+ // a re-entrant call on the same thread needs to resolve a request whose batch
+ // resolution is still in progress higher up the call stack.
+ Function<REQ, REP> singleSupplier = req -> supplier.apply(List.of(req)).get(0);
List<CachingSupplier<REQ, REP>> suppliers = new ArrayList<>(reqs.size());
List<REQ> nonCached = new ArrayList<>();
for (REQ req : reqs) {
- CachingSupplier<REQ, REP> cs = doCache(req, individualSupplier);
+ CachingSupplier<REQ, REP> cs = doCache(req, singleSupplier);
suppliers.add(cs);
if (cs.getValue() == null) {
nonCached.add(req);
}
}
+ // Resolve non-cached requests in batch and directly set results on CachingSuppliers
if (!nonCached.isEmpty()) {
- synchronized (nonCachedResults) {
+ // Use IdentityHashMap: request objects may have unstable hashCode() due to
+ // mutable RequestTrace/ModelBuilderRequest data that changes during batch resolution.
+ // Since we always use the same object references for put and get, identity is safe.
+ Map<REQ, Integer> reqToIndex = new IdentityHashMap<>();
+ List<CachingSupplier<REQ, REP>> nonCachedSuppliers = new ArrayList<>(nonCached.size());
+ for (int i = 0; i < reqs.size(); i++) {
+ if (suppliers.get(i).getValue() == null) {
+ reqToIndex.put(reqs.get(i), i);
+ nonCachedSuppliers.add(suppliers.get(i));
+ }
+ }
+
+ // Mark these CachingSuppliers as being batch-resolved, and register them
+ // on this thread so that re-entrant apply() calls skip the wait.
+ Set<CachingSupplier<?, ?>> resolving = RESOLVING_ON_THREAD.get();
+ for (CachingSupplier<REQ, REP> cs : nonCachedSuppliers) {
+ cs.setBatchResolving(true);
+ resolving.add(cs);
+ }
+ try {
try {
List<REP> reps = supplier.apply(nonCached);
for (int i = 0; i < reps.size(); i++) {
- nonCachedResults.put(nonCached.get(i), reps.get(i));
+ Integer idx = reqToIndex.get(nonCached.get(i));
+ if (idx != null) {
+ suppliers.get(idx).complete(reps.get(i));
+ }
}
} catch (MavenExecutionException e) {
// If batch request fails, mark all non-cached requests as failed
+ CachingSupplier.AltRes failure = new CachingSupplier.AltRes(e.getCause());
+ for (REQ req : nonCached) {
+ Integer idx = reqToIndex.get(req);
+ if (idx != null) {
+ suppliers.get(idx).complete(failure);
+ }
+ }
+ } catch (Throwable e) {
+ // Ensure waiting concurrent threads are unblocked on unexpected errors
+ CachingSupplier.AltRes failure = new CachingSupplier.AltRes(e);
for (REQ req : nonCached) {
- nonCachedResults.put(
- req, new CachingSupplier.AltRes(e.getCause())); // Mark as processed but failed
+ Integer idx = reqToIndex.get(req);
+ if (idx != null) {
+ suppliers.get(idx).complete(failure);
+ }
}
- } finally {
- nonCachedResults.notifyAll();
+ uncheckedThrow(e);
+ }
Review Comment:
In requests(), the catch(Throwable) path rethrows the original exception after populating CachingSuppliers. This bypasses the method’s documented behavior of throwing BatchRequestException with per-request RequestResult details, and changes observable behavior compared to the existing MavenExecutionException path. It also means callers won’t get the aggregated failure information they rely on.
##########
impl/maven-impl/src/main/java/org/apache/maven/impl/cache/CachingSupplier.java:
##########
@@ -60,6 +96,19 @@ public REP apply(REQ req) {
return (REP) v;
}
+ /**
+ * Marks this supplier as having a batch resolution in progress.
+ * Concurrent threads calling {@link #apply} will wait for {@link #complete}
+ * instead of invoking the supplier independently.
+ *
+ * @param resolving {@code true} when batch resolution starts, {@code false} when it ends
+ */
+ void setBatchResolving(boolean resolving) {
+ this.batchResolving = resolving;
+ }
Review Comment:
setBatchResolving(false) can end a wait condition in apply(), but it does not notify threads that are currently blocked in wait(). If batchResolving is cleared without complete() having set a value (e.g., due to a partial/mismatched batch result list or future code paths), those threads can block indefinitely. Notifying on transition to non-resolving makes the coordination robust and avoids hangs.
--
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