Fix #12445: Fix deadlock in AbstractRequestCache when resolving parent POMs#12446
Draft
gnodet wants to merge 5 commits into
Draft
Fix #12445: Fix deadlock in AbstractRequestCache when resolving parent POMs#12446gnodet wants to merge 5 commits into
gnodet wants to merge 5 commits into
Conversation
… parent POMs Replace the wait/notify pattern in AbstractRequestCache.requests() with direct value setting via CachingSupplier.complete(). The old pattern stored a wait-based individualSupplier in CachingSupplier instances cached across calls. When a re-entrant requests() call (e.g., parent POM resolution during artifact resolution) retrieved a CachingSupplier from the cache, it would invoke the outer call's individualSupplier, which waited on a HashMap that would never be notified — deadlock. The fix: - Add CachingSupplier.complete() to set values without invoking the supplier - Pass a single-item fallback supplier to doCache() so newly created CachingSuppliers can independently resolve requests in re-entrant scenarios - After batch resolution, directly set results via complete() - Remove the synchronized wait/notify on HashMap entirely Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…otifyAll coordination Add ThreadLocal re-entrancy detection and CachingSupplier.setBatchResolving() so that concurrent threads wait for an in-progress batch result (via Object.wait/notifyAll) instead of resolving the same request independently, while same-thread re-entrant calls still use the fallback supplier to avoid deadlock. Also fix CachingTestRequestCache to use ConcurrentHashMap, clean up duplicate javadoc, and add concurrent resolution test. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…le hashCode ResolverRequest.hashCode() can change during batch resolution because RequestTrace includes mutable ModelBuilderRequest data (identified in PR apache#12166). Since reqToIndex always uses the same object references for put and get, IdentityHashMap is safe and avoids missed lookups that would cause redundant individual resolutions. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Verify that batch results are correctly delivered via complete() even when request hashCode() changes during resolution (simulating ResolverRequest with mutable RequestTrace/ModelBuilderRequest data). The IdentityHashMap-based reqToIndex ensures lookups succeed by reference identity regardless of hashCode mutations. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR addresses a deadlock in AbstractRequestCache.requests() triggered by re-entrant resolution (e.g., parent POM resolution during artifact resolution), by replacing the previous wait/notify pattern with direct completion of cached suppliers.
Changes:
- Introduces
CachingSupplier.complete(Object)plus abatchResolvingcoordination flag to allow batch results to be published to cached suppliers and to let concurrent readers wait for completion. - Updates
AbstractRequestCache.requests()to batch-resolve non-cached requests and populateCachingSupplierinstances directly, with a ThreadLocal re-entrancy guard to avoid same-thread deadlocks. - Adds targeted tests covering re-entrant behavior, concurrent request/batch coordination, unstable
hashCode()handling, and batch-result caching.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| impl/maven-impl/src/main/java/org/apache/maven/impl/cache/AbstractRequestCache.java | Reworks batch request caching/coordination to avoid re-entrant deadlocks and publish results via complete(). |
| impl/maven-impl/src/main/java/org/apache/maven/impl/cache/CachingSupplier.java | Adds batch-completion API and waiting behavior for concurrent callers during in-progress batch resolution. |
| impl/maven-impl/src/test/java/org/apache/maven/impl/cache/AbstractRequestCacheTest.java | Adds regression and concurrency tests for the deadlock scenario and caching behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+178
to
+188
| } 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); | ||
| } |
Comment on lines
+106
to
+108
| void setBatchResolving(boolean resolving) { | ||
| this.batchResolving = resolving; | ||
| } |
…and safe unblocking - catch(Throwable) now falls through to the collection loop and produces a proper BatchRequestException with per-request results, consistent with the MavenExecutionException path. - setBatchResolving(false) now calls notifyAll() to unblock any threads still waiting in apply() when complete() was never called (e.g., batch supplier returned fewer results than expected). - Add test for partial batch result edge case. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
AbstractRequestCache.requests()that caused Maven 4 to hang indefinitely when building with an empty local repository (e.g.,-Dmaven.repo.local=/tmp/fresh-repo)Object.wait()/notify()pattern with direct value setting viaCachingSupplier.complete(), eliminating the re-entrant deadlock scenarioRoot Cause
The
requests()method used a wait/notify pattern with a localHashMap(nonCachedResults):individualSupplierlambda was created that callednonCachedResults.wait(), expecting the batch resolution to populate results and callnotifyAll()individualSupplierwas stored insideCachingSupplierinstances viadoCache(), which cached them by request keyrequests()call occurred (e.g., parent POM resolution during artifact resolution),doCache()returned aCachingSupplierfrom the outer call — wrapping the outer call'sindividualSupplierCachingSupplier, which waited on the outer call'snonCachedResults— aHashMapthat could never be populated because the outer batch supplier was blocked waiting for the inner call to completeFix
CachingSupplier.complete(Object result)to directly set cached values without invoking the supplierrequests()to pass a single-item fallback supplier todoCache()(instead of the wait-basedindividualSupplier), so any re-entrant retrieval can resolve independentlyCachingSupplierinstances viacomplete()synchronizedwait/notify pattern onHashMapentirelyTest plan
testReentrantRequestsDoesNotDeadlock— reproduces the exact deadlock scenario (with 5-second timeout detection)testBatchResultsAreCached— verifies batch results are properly cached for subsequent callsmaven-implpassmvn clean install -DskipTests) succeeds🤖 Generated with Claude Code