From 0873e99ebc7cecf072bf2384a0ff6c499c4288a4 Mon Sep 17 00:00:00 2001 From: Guillaume Nodet Date: Wed, 27 May 2026 09:41:23 +0200 Subject: [PATCH 1/3] Fix deadlock in AbstractRequestCache.requests() due to unstable hashCode The `requests()` method uses a HashMap to coordinate batch results between the batch supplier and the individual CachingSupplier instances. When a request object's hashCode() changes between HashMap.put() and HashMap.containsKey() (e.g., because ResolverRequest includes a RequestTrace with mutable ModelBuilderRequest data), the key is stored in one bucket but looked up in another, causing containsKey() to return false. The individualSupplier then waits forever for a result that is already in the map but unreachable. Switch to IdentityHashMap which uses System.identityHashCode() and reference equality (==), both of which are stable regardless of mutable object state. This is safe because the code always uses the same object reference for put and get. Co-Authored-By: Claude Opus 4.6 --- .../org/apache/maven/impl/cache/AbstractRequestCache.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/impl/maven-impl/src/main/java/org/apache/maven/impl/cache/AbstractRequestCache.java b/impl/maven-impl/src/main/java/org/apache/maven/impl/cache/AbstractRequestCache.java index 0b7fac393bf8..0dc5a7d0f044 100644 --- a/impl/maven-impl/src/main/java/org/apache/maven/impl/cache/AbstractRequestCache.java +++ b/impl/maven-impl/src/main/java/org/apache/maven/impl/cache/AbstractRequestCache.java @@ -19,7 +19,7 @@ package org.apache.maven.impl.cache; import java.util.ArrayList; -import java.util.HashMap; +import java.util.IdentityHashMap; import java.util.List; import java.util.Map; import java.util.function.Function; @@ -85,7 +85,7 @@ public , REP extends Result> REP request(REQ req, Fu @SuppressWarnings("unchecked") public , REP extends Result> List requests( List reqs, Function, List> supplier) { - final Map nonCachedResults = new HashMap<>(); + final Map nonCachedResults = new IdentityHashMap<>(); List> allResults = new ArrayList<>(reqs.size()); Function individualSupplier = req -> { From 343ea608e5d609c5c8af50352dbc8af86b72a766 Mon Sep 17 00:00:00 2001 From: Guillaume Nodet Date: Thu, 28 May 2026 23:12:14 +0200 Subject: [PATCH 2/3] Fix re-entrant self-deadlock in CachingSupplier during batch cache computation When requests() stores CachingSuppliers wrapping individualSupplier in the session cache, a re-entrant call from the same thread can self-deadlock: the batch computation triggers request() which finds the cached entry, calls individualSupplier.apply() which wait()s for the batch to complete, but the batch can't complete because this thread is now blocked. Track the computing thread in CachingSupplier and throw CyclicCacheAccessException on re-entrant access. The request() method catches this and falls back to computing directly with the caller's supplier, bypassing the cache for that one call. Co-Authored-By: Claude Opus 4.6 --- .../impl/cache/AbstractRequestCache.java | 9 ++++++++- .../maven/impl/cache/CachingSupplier.java | 20 +++++++++++++++++++ 2 files changed, 28 insertions(+), 1 deletion(-) diff --git a/impl/maven-impl/src/main/java/org/apache/maven/impl/cache/AbstractRequestCache.java b/impl/maven-impl/src/main/java/org/apache/maven/impl/cache/AbstractRequestCache.java index 0dc5a7d0f044..4d655c762083 100644 --- a/impl/maven-impl/src/main/java/org/apache/maven/impl/cache/AbstractRequestCache.java +++ b/impl/maven-impl/src/main/java/org/apache/maven/impl/cache/AbstractRequestCache.java @@ -60,7 +60,14 @@ public abstract class AbstractRequestCache implements RequestCache { @SuppressWarnings("all") public , REP extends Result> REP request(REQ req, Function supplier) { CachingSupplier cs = doCache(req, supplier); - return cs.apply(req); + try { + return cs.apply(req); + } catch (CachingSupplier.CyclicCacheAccessException e) { + // Re-entrant access from the same thread (e.g., a batch requests() computation + // triggered a singular request() that found the same cached entry). Compute + // directly with the caller's supplier to break the cycle. + return supplier.apply(req); + } } /** diff --git a/impl/maven-impl/src/main/java/org/apache/maven/impl/cache/CachingSupplier.java b/impl/maven-impl/src/main/java/org/apache/maven/impl/cache/CachingSupplier.java index d1960b194484..85b2411e0169 100644 --- a/impl/maven-impl/src/main/java/org/apache/maven/impl/cache/CachingSupplier.java +++ b/impl/maven-impl/src/main/java/org/apache/maven/impl/cache/CachingSupplier.java @@ -30,6 +30,8 @@ public class CachingSupplier implements Function { protected final Function supplier; protected volatile Object value; + // Guarded by synchronized(this) — tracks which thread is currently computing + private Thread computingThread; public CachingSupplier(Function supplier) { this.supplier = supplier; @@ -46,10 +48,18 @@ public REP apply(REQ req) { if ((v = value) == null) { synchronized (this) { if ((v = value) == null) { + if (computingThread == Thread.currentThread()) { + throw new CyclicCacheAccessException(); + } + computingThread = Thread.currentThread(); try { v = value = supplier.apply(req); + } catch (CyclicCacheAccessException e) { + throw e; } catch (Exception e) { v = value = new AltRes(e); + } finally { + computingThread = null; } } } @@ -60,6 +70,16 @@ public REP apply(REQ req) { return (REP) v; } + /** + * Thrown when a re-entrant call is detected on the same thread that is already + * computing this supplier's value. Prevents self-deadlock when a batch + * {@code requests()} computation triggers a singular {@code request()} that + * finds the same CachingSupplier in the cache. + */ + public static class CyclicCacheAccessException extends RuntimeException { + CyclicCacheAccessException() {} + } + /** * Special holder class for exceptions that occur during supplier execution. * Allows caching and re-throwing of exceptions on subsequent calls. From 2b09790cba70f63dc34633e50e1ddc91c59daced Mon Sep 17 00:00:00 2001 From: Guillaume Nodet Date: Fri, 29 May 2026 10:41:06 +0200 Subject: [PATCH 3/3] Always snapshot system/user properties in ModelBuilderRequest When systemProperties or userProperties are null, the constructor fell through to session.getSystemProperties() without Map.copyOf(). That returns a live view over java.util.Properties, whose hashCode() mutates if System.setProperty() is called. Since ModelBuilderRequest ends up as RequestTrace.data in parent traces, and ResolverRequest.hashCode() recurses through the entire trace chain, this made cache keys unstable. Always wrap in Map.copyOf() regardless of the source. Co-Authored-By: Claude Opus 4.6 --- .../org/apache/maven/api/services/ModelBuilderRequest.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/api/maven-api-core/src/main/java/org/apache/maven/api/services/ModelBuilderRequest.java b/api/maven-api-core/src/main/java/org/apache/maven/api/services/ModelBuilderRequest.java index 826ffe8fc4c5..bb8f524749da 100644 --- a/api/maven-api-core/src/main/java/org/apache/maven/api/services/ModelBuilderRequest.java +++ b/api/maven-api-core/src/main/java/org/apache/maven/api/services/ModelBuilderRequest.java @@ -332,8 +332,8 @@ private static class DefaultModelBuilderRequest extends BaseRequest imp this.activeProfileIds = activeProfileIds != null ? List.copyOf(activeProfileIds) : List.of(); this.inactiveProfileIds = inactiveProfileIds != null ? List.copyOf(inactiveProfileIds) : List.of(); this.systemProperties = - systemProperties != null ? Map.copyOf(systemProperties) : session.getSystemProperties(); - this.userProperties = userProperties != null ? Map.copyOf(userProperties) : session.getUserProperties(); + Map.copyOf(systemProperties != null ? systemProperties : session.getSystemProperties()); + this.userProperties = Map.copyOf(userProperties != null ? userProperties : session.getUserProperties()); this.repositoryMerging = repositoryMerging; this.repositories = repositories != null ? List.copyOf(validate(repositories)) : null; this.lifecycleBindingsInjector = lifecycleBindingsInjector;