ascheman commented on PR #12478:
URL: https://github.com/apache/maven/pull/12478#issuecomment-5011044438
I tried to verify this PR empirically, and I think the two new tests don't
actually guard against #12472:
**Method**: checked out a pre-fix tree (`AbstractRequestCache` still with
the old `individualSupplier` wait-on-map pattern, i.e. before c6de104bff) and
ran the exact tests from this PR against it.
| Test | pre-fix | PR head |
|---|---|---|
| `testConcurrentBatchRequestsWithSharedKeyDoNotDeadlock` /
`testSequentialBatchRequestsWithSharedKeyReuseResult` | **pass** (4/4 runs,
~0.5s — no hang) | pass |
| variant with mutating `equals()`/`hashCode()` (below) | **deadlock** (10s
timeout) | pass (1.5s) |
So the pure identity-mismatch scenario as constructed here doesn't deadlock
pre-fix. The ingredient that actually reproduces the #12472 hang is a shared
key whose `equals()`/`hashCode()` change **during** batch resolution (mirroring
mutable `RequestTrace` data in real requests): pre-fix, thread B waits on
thread A's equals-based map for a key that no longer matches — forever. Good
news: the variant passes on this PR's head, so the c6de104bff fix itself holds;
it's only the regression guard that doesn't bite.
Suggest replacing (or complementing) the concurrent test with this variant:
```java
/**
* Variant of the #12472 regression scenario where the shared request's
* equals()/hashCode() change during batch resolution (mirroring mutable
* RequestTrace data in real requests). Pre-fix, thread B waits on thread A's
* equals-based nonCachedResults map for a key that no longer matches —
forever.
*/
@Test
void testConcurrentBatchRequestsWithMutatingSharedKeyDoNotDeadlock() throws
Exception {
GenericCachingTestRequestCache cachingCache = new
GenericCachingTestRequestCache();
MutableHashCodeRequest reqOnlyA = new MutableHashCodeRequest("onlyA",
"trace");
MutableHashCodeRequest reqOnlyB = new MutableHashCodeRequest("onlyB",
"trace");
MutableHashCodeRequest sharedByA = new MutableHashCodeRequest("shared",
"trace");
MutableHashCodeRequest sharedByB = new MutableHashCodeRequest("shared",
"trace");
CountDownLatch aInBatch = new CountDownLatch(1);
Function<List<MutableHashCodeRequest>, List<MutableHashCodeResult>>
supplierA = reqs -> {
aInBatch.countDown();
try {
Thread.sleep(1500);
} catch (InterruptedException e) {
Thread.currentThread().interrupt();
}
for (MutableHashCodeRequest r : reqs) {
r.setTraceData(r.getTraceData() + "-A");
}
return reqs.stream().map(MutableHashCodeResult::new).toList();
};
Function<List<MutableHashCodeRequest>, List<MutableHashCodeResult>>
supplierB = reqs -> {
for (MutableHashCodeRequest r : reqs) {
r.setTraceData(r.getTraceData() + "-B");
}
return reqs.stream().map(MutableHashCodeResult::new).toList();
};
ExecutorService executor = Executors.newFixedThreadPool(2);
try {
Future<List<MutableHashCodeResult>> futureA =
executor.submit(() ->
cachingCache.requests(List.of(reqOnlyA, sharedByA), supplierA));
assertTrue(aInBatch.await(5, TimeUnit.SECONDS), "Thread A should
have entered its batch supplier");
Future<List<MutableHashCodeResult>> futureB =
executor.submit(() ->
cachingCache.requests(List.of(reqOnlyB, sharedByB), supplierB));
List<MutableHashCodeResult> resultsA = futureA.get(10,
TimeUnit.SECONDS);
List<MutableHashCodeResult> resultsB = futureB.get(10,
TimeUnit.SECONDS);
assertEquals(2, resultsA.size());
assertEquals(2, resultsB.size());
} catch (TimeoutException e) {
throw new AssertionError(
"Cross-thread deadlock detected: batch requests() with a
shared key whose "
+ "equals()/hashCode() mutate during resolution did
not complete (#12472)",
e);
} finally {
executor.shutdownNow();
}
}
```
It needs two small helpers (a `MutableHashCodeRequest` whose `hashCode()`
includes the mutable trace data, and an equals-based generic test cache).
**Offer: we can push the complete, verified change (test + helpers) directly to
this branch** — just say the word.
--
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]
