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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
