This is an automated email from the ASF dual-hosted git repository.
gnodet pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/maven.git
The following commit(s) were added to refs/heads/master by this push:
new c6de104bff Fix #12445: Fix deadlock in AbstractRequestCache when
resolving parent POMs (#12446)
c6de104bff is described below
commit c6de104bff92fc0c4572813bac82935470441dc4
Author: Guillaume Nodet <[email protected]>
AuthorDate: Sat Jul 11 18:48:37 2026 +0200
Fix #12445: Fix deadlock in AbstractRequestCache when resolving parent POMs
(#12446)
* Fix #12445: Fix deadlock in AbstractRequestCache when resolving 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 <[email protected]>
* Fix #12445: Prevent duplicate concurrent resolutions via wait/notifyAll
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 <[email protected]>
* Fix #12445: Use IdentityHashMap for reqToIndex to handle unstable hashCode
ResolverRequest.hashCode() can change during batch resolution because
RequestTrace includes mutable ModelBuilderRequest data (identified in
PR #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 <[email protected]>
* Fix #12445: Add test for batch resolution with unstable hashCode
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 <[email protected]>
* Fix #12445: Address Copilot review — consistent error handling 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 <[email protected]>
---------
Co-authored-by: Claude Opus 4.6 <[email protected]>
---
.../maven/impl/cache/AbstractRequestCache.java | 122 +++++--
.../apache/maven/impl/cache/CachingSupplier.java | 67 +++-
.../maven/impl/cache/AbstractRequestCacheTest.java | 391 +++++++++++++++++++++
3 files changed, 546 insertions(+), 34 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 0b7fac393b..d1706a5250 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,9 +19,11 @@
package org.apache.maven.impl.cache;
import java.util.ArrayList;
-import java.util.HashMap;
+import java.util.HashSet;
+import java.util.IdentityHashMap;
import java.util.List;
import java.util.Map;
+import java.util.Set;
import java.util.function.Function;
import org.apache.maven.api.cache.BatchRequestException;
@@ -43,6 +45,27 @@
*/
public abstract class AbstractRequestCache implements RequestCache {
+ /**
+ * Tracks which {@link CachingSupplier} instances are currently being
batch-resolved
+ * on the current thread. Used by {@link CachingSupplier#apply} to detect
re-entrant
+ * calls and avoid deadlock: if the current thread is already resolving a
CachingSupplier,
+ * {@code apply()} will invoke the fallback supplier instead of waiting for
+ * {@link CachingSupplier#complete} (which would never arrive on the same
thread).
+ */
+ private static final ThreadLocal<Set<CachingSupplier<?, ?>>>
RESOLVING_ON_THREAD =
+ ThreadLocal.withInitial(HashSet::new);
+
+ /**
+ * Checks whether the given CachingSupplier is currently being
batch-resolved
+ * on the calling thread.
+ *
+ * @param cs the CachingSupplier to check
+ * @return {@code true} if a batch resolution for {@code cs} is in
progress on this thread
+ */
+ static boolean isResolvingOnCurrentThread(CachingSupplier<?, ?> cs) {
+ return RESOLVING_ON_THREAD.get().contains(cs);
+ }
+
/**
* Executes and optionally caches a single request.
* <p>
@@ -70,8 +93,20 @@ public <REQ extends Request<?>, REP extends Result<REQ>> REP
request(REQ req, Fu
* only the non-cached requests using the provided supplier function.
* </p>
* <p>
- * If any request in the batch fails, a {@link BatchRequestException} is
thrown, containing
- * details of all failed requests.
+ * This implementation uses {@link CachingSupplier#complete} with {@code
wait/notifyAll}
+ * to coordinate batch results, and a {@link ThreadLocal} re-entrancy
guard to prevent
+ * deadlocks when batch resolution triggers nested calls (e.g., parent POM
resolution
+ * during artifact resolution).
+ * </p>
+ * <p>
+ * <b>Concurrent calls</b> for the same request on different threads: the
first thread
+ * performs the resolution; the second thread's {@link
CachingSupplier#apply} blocks
+ * (via {@code Object.wait()}) until {@code complete()} is called,
avoiding duplicate work.
+ * </p>
+ * <p>
+ * <b>Re-entrant calls</b> on the same thread: detected via {@link
#RESOLVING_ON_THREAD}.
+ * {@link CachingSupplier#apply} skips the wait and invokes the fallback
supplier directly
+ * so the inner call can complete without waiting for the outer call's
batch result.
* </p>
*
* @param <REQ> The request type
@@ -85,57 +120,84 @@ 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.
+ // We mark all non-cached requests as failed and fall
through to the
+ // collection loop, which produces a consistent
BatchRequestException
+ // with per-request RequestResult details — same as
MavenExecutionException.
+ 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();
+ }
+ } finally {
+ for (CachingSupplier<REQ, REP> cs : nonCachedSuppliers) {
+ cs.setBatchResolving(false);
+ resolving.remove(cs);
}
}
}
// Collect results in original order
+ List<RequestResult<REQ, REP>> allResults = new
ArrayList<>(reqs.size());
boolean hasFailures = false;
for (int i = 0; i < reqs.size(); i++) {
REQ req = reqs.get(i);
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 d1960b1944..ea39b37e45 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
@@ -39,6 +39,25 @@ public Object getValue() {
return value;
}
+ /**
+ * Directly sets the cached value without invoking the supplier, then
notifies
+ * any threads blocked in {@link #apply(Object)} waiting for the result.
+ * <p>
+ * This is used by {@link AbstractRequestCache#requests} to set
batch-resolved results
+ * on CachingSupplier instances. Concurrent callers blocked in {@code
apply()} will
+ * be woken up and see this value instead of invoking the supplier
redundantly.
+ *
+ * @param result the result to cache (may be a normal result or an {@link
AltRes} for errors)
+ */
+ public void complete(Object result) {
+ synchronized (this) {
+ if (value == null) {
+ value = result;
+ }
+ this.notifyAll();
+ }
+ }
+
@Override
@SuppressWarnings({"unchecked", "checkstyle:InnerAssignment"})
public REP apply(REQ req) {
@@ -46,10 +65,27 @@ public REP apply(REQ req) {
if ((v = value) == null) {
synchronized (this) {
if ((v = value) == null) {
- try {
- v = value = supplier.apply(req);
- } catch (Exception e) {
- v = value = new AltRes(e);
+ // If a batch resolution is in progress on another thread,
wait for
+ // complete() rather than invoking the supplier
redundantly.
+ // Re-entrant calls on the SAME thread must NOT wait (that
would deadlock),
+ // so AbstractRequestCache marks CachingSuppliers it is
currently resolving
+ // in a ThreadLocal; those are excluded from waiting.
+ if (batchResolving &&
!AbstractRequestCache.isResolvingOnCurrentThread(this)) {
+ while ((v = value) == null && batchResolving) {
+ try {
+ this.wait();
+ } catch (InterruptedException e) {
+ Thread.currentThread().interrupt();
+ break;
+ }
+ }
+ }
+ if ((v = value) == null) {
+ try {
+ v = value = supplier.apply(req);
+ } catch (Exception e) {
+ v = value = new AltRes(e);
+ }
}
}
}
@@ -60,6 +96,29 @@ 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.
+ * <p>
+ * When clearing the flag ({@code resolving = false}), any threads still
blocked
+ * in {@link #apply(Object)} are notified so they can proceed to the
fallback
+ * supplier. This handles edge cases where {@link #complete} was never
called
+ * (e.g., batch supplier returned fewer results than expected).
+ *
+ * @param resolving {@code true} when batch resolution starts, {@code
false} when it ends
+ */
+ void setBatchResolving(boolean resolving) {
+ this.batchResolving = resolving;
+ if (!resolving) {
+ synchronized (this) {
+ this.notifyAll();
+ }
+ }
+ }
+
+ private volatile boolean batchResolving;
+
/**
* Special holder class for exceptions that occur during supplier
execution.
* Allows caching and re-throwing of exceptions on subsequent calls.
diff --git
a/impl/maven-impl/src/test/java/org/apache/maven/impl/cache/AbstractRequestCacheTest.java
b/impl/maven-impl/src/test/java/org/apache/maven/impl/cache/AbstractRequestCacheTest.java
index 5de697c15e..e52a048ff2 100644
---
a/impl/maven-impl/src/test/java/org/apache/maven/impl/cache/AbstractRequestCacheTest.java
+++
b/impl/maven-impl/src/test/java/org/apache/maven/impl/cache/AbstractRequestCacheTest.java
@@ -20,6 +20,14 @@
import java.util.Arrays;
import java.util.List;
+import java.util.Map;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+import java.util.concurrent.Future;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.TimeoutException;
import java.util.function.Function;
import org.apache.maven.api.ProtoSession;
@@ -159,6 +167,285 @@ void testSuccessfulBatchRequestDoesNotThrowException() {
assertEquals(request2, results.get(1).getRequest());
}
+ /**
+ * Tests that re-entrant calls to {@code requests()} do not deadlock.
+ * <p>
+ * This reproduces the scenario from issue #12445: an outer {@code
requests()} call
+ * creates CachingSupplier instances that are stored in the cache. During
batch resolution
+ * (inside the outer call's batch supplier), a nested {@code requests()}
call is triggered
+ * (e.g., parent POM resolution during artifact resolution). If the inner
call hits the
+ * same cache entry (same request key), it gets back the CachingSupplier
from the outer call.
+ * <p>
+ * Before the fix, the CachingSupplier wrapped a wait-based supplier that
referenced the
+ * outer call's {@code nonCachedResults} HashMap. The inner call would
wait on that HashMap
+ * forever, since the outer call couldn't populate it until the inner call
completed.
+ */
+ @Test
+ void testReentrantRequestsDoesNotDeadlock() throws Exception {
+ // Use a caching implementation that stores CachingSuppliers in a
shared map
+ CachingTestRequestCache cachingCache = new CachingTestRequestCache();
+
+ // "parentPom" is the request that will be resolved by both the outer
and inner calls
+ TestRequest artifact = createTestRequest("artifact");
+ TestRequest parentPom = createTestRequest("parentPom");
+
+ // The outer batch supplier resolves requests, but during resolution
of "artifact",
+ // it triggers a nested requests() call for "parentPom"
+ Function<List<TestRequest>, List<TestResult>> outerBatchSupplier =
reqs -> {
+ List<TestResult> results = new java.util.ArrayList<>();
+ for (TestRequest req : reqs) {
+ if (req.equals(artifact)) {
+ // Simulate parent POM resolution: re-entrant call for
"parentPom"
+ List<TestResult> innerResults = cachingCache.requests(
+ List.of(parentPom),
+ innerReqs ->
innerReqs.stream().map(TestResult::new).toList());
+ // After inner call completes, outer resolution succeeds
+ assertEquals(1, innerResults.size());
+ }
+ results.add(new TestResult(req));
+ }
+ return results;
+ };
+
+ // Execute with a timeout to detect deadlock
+ ExecutorService executor = Executors.newSingleThreadExecutor();
+ try {
+ Future<List<TestResult>> future =
+ executor.submit(() ->
cachingCache.requests(List.of(artifact, parentPom), outerBatchSupplier));
+
+ // If this deadlocks, the future will time out
+ List<TestResult> results = future.get(5, TimeUnit.SECONDS);
+
+ assertEquals(2, results.size());
+ assertEquals(artifact, results.get(0).getRequest());
+ assertEquals(parentPom, results.get(1).getRequest());
+ } catch (TimeoutException e) {
+ throw new AssertionError(
+ "Deadlock detected: re-entrant requests() call did not
complete within 5 seconds", e);
+ } finally {
+ executor.shutdownNow();
+ }
+ }
+
+ /**
+ * Tests that a concurrent singular {@code request()} call waits for an
+ * in-progress batch resolution instead of invoking the supplier
independently.
+ * <p>
+ * Thread A starts a batch resolution via {@code requests()} (which marks
the
+ * CachingSupplier as {@code batchResolving} and registers it on the
current thread).
+ * While Thread A is still inside its batch supplier, Thread B calls
{@code request()}
+ * for the same key. Thread B's {@code cs.apply()} should see {@code
batchResolving == true},
+ * wait for {@code complete()}, and return the batch result without
running its own supplier.
+ */
+ @Test
+ void testConcurrentRequestDoesNotDuplicateResolution() throws Exception {
+ CachingTestRequestCache cachingCache = new CachingTestRequestCache();
+
+ TestRequest sharedReq = createTestRequest("shared");
+
+ java.util.concurrent.atomic.AtomicInteger resolutionCount = new
java.util.concurrent.atomic.AtomicInteger(0);
+ CountDownLatch batchStarted = new CountDownLatch(1);
+ CountDownLatch proceedWithBatch = new CountDownLatch(1);
+
+ // Thread A's batch supplier: signals when it starts, then waits
before completing
+ Function<List<TestRequest>, List<TestResult>> slowBatchSupplier = reqs
-> {
+ resolutionCount.incrementAndGet();
+ batchStarted.countDown();
+ try {
+ proceedWithBatch.await(5, TimeUnit.SECONDS);
+ } catch (InterruptedException e) {
+ Thread.currentThread().interrupt();
+ }
+ return reqs.stream().map(TestResult::new).toList();
+ };
+
+ ExecutorService executor = Executors.newFixedThreadPool(2);
+ try {
+ // Thread A: starts batch resolution via requests(), pauses inside
the supplier
+ Future<List<TestResult>> futureA =
+ executor.submit(() ->
cachingCache.requests(List.of(sharedReq), slowBatchSupplier));
+
+ // Wait for Thread A's batch supplier to start
+ assertTrue(batchStarted.await(5, TimeUnit.SECONDS), "Thread A's
batch should have started");
+
+ // Thread B: calls request() (singular) for the same key.
+ // It gets the same CachingSupplier from the cache, sees
batchResolving == true,
+ // and should wait for Thread A's complete() instead of invoking
its own supplier.
+ Future<TestResult> futureB = executor.submit(() ->
cachingCache.request(sharedReq, req -> {
+ resolutionCount.incrementAndGet();
+ return new TestResult(req);
+ }));
+
+ // Give Thread B time to enter apply() and start waiting
+ Thread.sleep(200);
+
+ // Let Thread A's batch complete — this calls complete() which
wakes Thread B
+ proceedWithBatch.countDown();
+
+ // Both should complete
+ List<TestResult> resultsA = futureA.get(5, TimeUnit.SECONDS);
+ TestResult resultB = futureB.get(5, TimeUnit.SECONDS);
+
+ assertEquals(1, resultsA.size());
+ assertNotNull(resultB);
+
+ // The shared request should have been resolved only once (by
Thread A's batch).
+ // Thread B should have waited for Thread A's complete() call, not
invoked its
+ // own supplier.
+ assertEquals(1, resolutionCount.get(), "Request should be resolved
only once, not duplicated");
+ } finally {
+ executor.shutdownNow();
+ }
+ }
+
+ /**
+ * Tests that batch resolution is resilient to requests whose {@code
hashCode()} changes
+ * during resolution (e.g., because {@code RequestTrace} includes mutable
+ * {@code ModelBuilderRequest} data).
+ * <p>
+ * The {@code reqToIndex} map inside {@code requests()} uses {@code
IdentityHashMap}
+ * specifically to handle this: after the batch supplier mutates request
data,
+ * lookups still succeed because they compare by reference identity, not by
+ * {@code equals()/hashCode()}.
+ */
+ @Test
+ void testBatchResolutionWithUnstableHashCode() {
+ // Use identity-based cache to match real DefaultRequestCache behavior
+ IdentityCachingTestRequestCache cachingCache = new
IdentityCachingTestRequestCache();
+
+ // Create requests with mutable trace data that affects hashCode()
+ MutableHashCodeRequest req1 = new MutableHashCodeRequest("req1",
"traceA");
+ MutableHashCodeRequest req2 = new MutableHashCodeRequest("req2",
"traceB");
+
+ int originalHash1 = req1.hashCode();
+ int originalHash2 = req2.hashCode();
+
+ java.util.concurrent.atomic.AtomicInteger supplierCallCount = new
java.util.concurrent.atomic.AtomicInteger(0);
+
+ Function<List<MutableHashCodeRequest>, List<MutableHashCodeResult>>
batchSupplier = reqs -> {
+ supplierCallCount.incrementAndGet();
+ // Mutate trace data during resolution — this changes hashCode()
+ for (MutableHashCodeRequest r : reqs) {
+ r.setTraceData(r.getTraceData() + "-mutated");
+ }
+ return reqs.stream().map(MutableHashCodeResult::new).toList();
+ };
+
+ // Resolve the batch — hashCode changes inside the supplier
+ List<MutableHashCodeResult> results =
cachingCache.requests(List.of(req1, req2), batchSupplier);
+
+ // Verify results were delivered despite hashCode mutation
+ assertEquals(2, results.size());
+ assertEquals(req1, results.get(0).getRequest());
+ assertEquals(req2, results.get(1).getRequest());
+ assertEquals(1, supplierCallCount.get());
+
+ // Verify hashCode actually changed
+ assertTrue(
+ req1.hashCode() != originalHash1 || req2.hashCode() !=
originalHash2,
+ "hashCode should have changed after mutation");
+ }
+
+ /**
+ * Tests that a concurrent {@code request()} call does not hang when the
batch
+ * supplier returns fewer results than expected (partial batch).
+ * <p>
+ * In this scenario, {@link CachingSupplier#complete} is never called for
one
+ * of the CachingSuppliers. The waiting thread in {@code apply()} must
still
+ * be unblocked when {@code setBatchResolving(false)} is called in the
+ * {@code finally} block, which calls {@code notifyAll()} to wake any
waiters.
+ * The unblocked thread then falls through to the fallback supplier.
+ */
+ @Test
+ void testConcurrentRequestUnblockedOnPartialBatchResult() throws Exception
{
+ CachingTestRequestCache cachingCache = new CachingTestRequestCache();
+
+ TestRequest req1 = createTestRequest("req1");
+ TestRequest req2 = createTestRequest("req2");
+
+ CountDownLatch batchStarted = new CountDownLatch(1);
+ CountDownLatch proceedWithBatch = new CountDownLatch(1);
+
+ // Batch supplier that only resolves req1, "forgetting" req2
+ Function<List<TestRequest>, List<TestResult>> partialBatchSupplier =
reqs -> {
+ batchStarted.countDown();
+ try {
+ proceedWithBatch.await(5, TimeUnit.SECONDS);
+ } catch (InterruptedException e) {
+ Thread.currentThread().interrupt();
+ }
+ // Return only the first result — req2's CachingSupplier never
gets complete()
+ return List.of(new TestResult(reqs.get(0)));
+ };
+
+ ExecutorService executor = Executors.newFixedThreadPool(2);
+ try {
+ // Thread A: starts batch resolution for [req1, req2]
+ Future<List<TestResult>> futureA =
+ executor.submit(() -> cachingCache.requests(List.of(req1,
req2), partialBatchSupplier));
+
+ // Wait for Thread A's batch supplier to start
+ assertTrue(batchStarted.await(5, TimeUnit.SECONDS), "Batch should
have started");
+
+ // Thread B: calls request() for req2 — gets the same
CachingSupplier,
+ // sees batchResolving == true, and waits in apply()
+ Future<TestResult> futureB = executor.submit(() ->
cachingCache.request(req2, req -> {
+ // Fallback supplier — should be invoked after
setBatchResolving(false)
+ return new TestResult(req);
+ }));
+
+ // Give Thread B time to enter apply() and start waiting
+ Thread.sleep(200);
+
+ // Let Thread A's batch complete — only resolves req1
+ proceedWithBatch.countDown();
+
+ // Thread A should complete (req2 gets resolved via fallback in
collection loop)
+ List<TestResult> resultsA = futureA.get(5, TimeUnit.SECONDS);
+ assertNotNull(resultsA);
+
+ // Thread B should also complete — setBatchResolving(false) +
notifyAll()
+ // unblocks it, and it falls through to its fallback supplier
+ TestResult resultB = futureB.get(5, TimeUnit.SECONDS);
+ assertNotNull(resultB);
+ assertEquals(req2, resultB.getRequest());
+ } catch (TimeoutException e) {
+ throw new AssertionError("Thread hung: setBatchResolving(false)
did not unblock waiting thread", e);
+ } finally {
+ executor.shutdownNow();
+ }
+ }
+
+ /**
+ * Tests that batch results are properly cached in CachingSupplier
instances
+ * so subsequent calls return the cached values.
+ */
+ @Test
+ void testBatchResultsAreCached() {
+ CachingTestRequestCache cachingCache = new CachingTestRequestCache();
+
+ TestRequest req1 = createTestRequest("req1");
+ TestRequest req2 = createTestRequest("req2");
+
+ java.util.concurrent.atomic.AtomicInteger supplierCallCount = new
java.util.concurrent.atomic.AtomicInteger(0);
+
+ Function<List<TestRequest>, List<TestResult>> batchSupplier = reqs -> {
+ supplierCallCount.incrementAndGet();
+ return reqs.stream().map(TestResult::new).toList();
+ };
+
+ // First call should invoke the batch supplier
+ List<TestResult> results1 = cachingCache.requests(List.of(req1, req2),
batchSupplier);
+ assertEquals(2, results1.size());
+ assertEquals(1, supplierCallCount.get());
+
+ // Second call with same requests should use cached values
+ List<TestResult> results2 = cachingCache.requests(List.of(req1, req2),
batchSupplier);
+ assertEquals(2, results2.size());
+ // Supplier should not have been called again
+ assertEquals(1, supplierCallCount.get());
+ }
+
// Helper methods and test classes
private TestRequest createTestRequest(String id) {
@@ -228,6 +515,110 @@ public TestRequest getRequest() {
}
}
+ /**
+ * A cache implementation that stores CachingSupplier instances in a
shared map,
+ * simulating the real DefaultRequestCache behavior where the same
CachingSupplier
+ * can be returned for the same request key across different requests()
calls.
+ */
+ static class CachingTestRequestCache extends AbstractRequestCache {
+ private final Map<TestRequest, CachingSupplier<?, ?>> cache = new
ConcurrentHashMap<>();
+
+ @Override
+ @SuppressWarnings("unchecked")
+ protected <REQ extends Request<?>, REP extends Result<REQ>>
CachingSupplier<REQ, REP> doCache(
+ REQ req, Function<REQ, REP> supplier) {
+ return (CachingSupplier<REQ, REP>)
+ cache.computeIfAbsent((TestRequest) req, r -> new
CachingSupplier<>(supplier));
+ }
+ }
+
+ /**
+ * A request implementation whose hashCode() depends on mutable trace data,
+ * simulating ResolverRequest with mutable
RequestTrace/ModelBuilderRequest data.
+ */
+ static class MutableHashCodeRequest implements Request<ProtoSession> {
+ private final String id;
+ private String traceData;
+
+ MutableHashCodeRequest(String id, String traceData) {
+ this.id = id;
+ this.traceData = traceData;
+ }
+
+ String getTraceData() {
+ return traceData;
+ }
+
+ void setTraceData(String traceData) {
+ this.traceData = traceData;
+ }
+
+ @Override
+ @Nonnull
+ public ProtoSession getSession() {
+ return mock(ProtoSession.class);
+ }
+
+ @Override
+ public RequestTrace getTrace() {
+ return null;
+ }
+
+ @Override
+ public boolean equals(Object obj) {
+ if (this == obj) {
+ return true;
+ }
+ if (obj == null || getClass() != obj.getClass()) {
+ return false;
+ }
+ MutableHashCodeRequest that = (MutableHashCodeRequest) obj;
+ return java.util.Objects.equals(id, that.id) &&
java.util.Objects.equals(traceData, that.traceData);
+ }
+
+ @Override
+ public int hashCode() {
+ return java.util.Objects.hash(id, traceData);
+ }
+
+ @Override
+ @Nonnull
+ public String toString() {
+ return "MutableHashCodeRequest[" + id + ", " + traceData + "]";
+ }
+ }
+
+ static class MutableHashCodeResult implements
Result<MutableHashCodeRequest> {
+ private final MutableHashCodeRequest request;
+
+ MutableHashCodeResult(MutableHashCodeRequest request) {
+ this.request = request;
+ }
+
+ @Override
+ @Nonnull
+ public MutableHashCodeRequest getRequest() {
+ return request;
+ }
+ }
+
+ /**
+ * Cache implementation using identity-based storage (IdentityHashMap).
+ * Simulates real DefaultRequestCache behavior where request identity —
+ * not equals/hashCode — determines cache hits.
+ */
+ static class IdentityCachingTestRequestCache extends AbstractRequestCache {
+ private final java.util.IdentityHashMap<Object, CachingSupplier<?, ?>>
cache =
+ new java.util.IdentityHashMap<>();
+
+ @Override
+ @SuppressWarnings("unchecked")
+ protected <REQ extends Request<?>, REP extends Result<REQ>>
CachingSupplier<REQ, REP> doCache(
+ REQ req, Function<REQ, REP> supplier) {
+ return (CachingSupplier<REQ, REP>) cache.computeIfAbsent(req, r ->
new CachingSupplier<>(supplier));
+ }
+ }
+
static class TestRequestCache extends AbstractRequestCache {
private final java.util.Map<TestRequest, RuntimeException> failures =
new java.util.HashMap<>();
