gnodet-bot commented on code in PR #2138:
URL: https://github.com/apache/maven-resolver/pull/2138#discussion_r4451778267
##########
maven-resolver-api/src/main/java/org/eclipse/aether/collection/CollectResult.java:
##########
@@ -93,7 +93,7 @@ public CollectResult addException(Exception exception) {
*
* @return The dependency cycles in the (raw) graph, never {@code null}.
*/
- public List<DependencyCycle> getCycles() {
+ public synchronized List<DependencyCycle> getCycles() {
Review Comment:
⚠️ **Same mutable-list escape as `getExceptions()`** — the returned
`ArrayList` is the live internal list. After the monitor releases, concurrent
`addCycle()` calls race with any iteration of the returned list.
Same fix: use `CopyOnWriteArrayList<DependencyCycle>` initialized eagerly in
the constructor.
##########
maven-resolver-api/src/main/java/org/eclipse/aether/collection/CollectResult.java:
##########
@@ -68,7 +68,7 @@ public CollectRequest getRequest() {
*
* @return The exceptions that occurred, never {@code null}.
*/
- public List<Exception> getExceptions() {
+ public synchronized List<Exception> getExceptions() {
Review Comment:
⚠️ **Mutable list escape — synchronized getter does not prevent concurrent
modification.**
Synchronizing this method protects only the read of the `exceptions` field
reference. The moment the method returns, the monitor is released and the
caller holds a reference to the live `ArrayList`. A concurrent `addException()`
call will call `ArrayList.add()` while the caller iterates —
`ConcurrentModificationException` at runtime.
Fix: return an unmodifiable snapshot under the lock, or switch to
`CopyOnWriteArrayList` (preferred — eliminates synchronized entirely and makes
iteration safe):
```suggestion
public List<Exception> getExceptions() {
return exceptions;
}
```
With `CopyOnWriteArrayList<Exception> exceptions = new
CopyOnWriteArrayList<>()` in the field declaration (initialized eagerly), no
synchronization is needed on any of the four methods, and callers get a
thread-safe view.
##########
maven-resolver-api/src/test/java/org/eclipse/aether/collection/CollectResultTest.java:
##########
@@ -0,0 +1,68 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.eclipse.aether.collection;
+
+import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+import java.util.concurrent.TimeUnit;
+
+import org.junit.jupiter.api.Test;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+class CollectResultTest {
+
+ @Test
+ void testConcurrentAddException() throws Exception {
+ CollectResult result = new CollectResult(new CollectRequest());
+
+ int threads = 10;
+ int additionsPerThread = 100;
+
+ ExecutorService executor = Executors.newFixedThreadPool(threads);
+ CountDownLatch start = new CountDownLatch(1);
+
+ try {
+ for (int i = 0; i < threads; i++) {
+ executor.submit(() -> {
+ try {
+ start.await();
+
+ for (int j = 0; j < additionsPerThread; j++) {
+ result.addException(new Exception());
+ }
+ } catch (InterruptedException e) {
+ Thread.currentThread().interrupt();
+ }
+ });
+ }
+
+ start.countDown();
+ executor.shutdown();
+
+ assertTrue(executor.awaitTermination(30, TimeUnit.SECONDS),
"Executor did not terminate in time");
+
+ assertEquals(threads * additionsPerThread,
result.getExceptions().size());
Review Comment:
⚠️ **Pre-fix test validation: this test does not reliably fail without the
`synchronized` fix.**
The test asserts that 10 threads × 100 `addException()` calls produce
exactly 1,000 exceptions. With an unsynchronized `ArrayList`, this *can* pass —
the primary failure mode is a `ConcurrentModificationException` or lost
updates, but neither is guaranteed with 10 threads on fast hardware. The test
may be green both before and after the fix, meaning it doesn't actually
validate the fix.
A more reliable approach for catching the mutable-escape race: while threads
are adding exceptions, concurrently iterate `result.getExceptions()` in a
separate thread and assert no `ConcurrentModificationException` is thrown. That
scenario reliably fails with the current approach (`synchronized` getter + live
`ArrayList`) and reliably passes with `CopyOnWriteArrayList`.
##########
maven-resolver-api/src/test/java/org/eclipse/aether/collection/CollectResultTest.java:
##########
@@ -0,0 +1,68 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.eclipse.aether.collection;
+
+import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+import java.util.concurrent.TimeUnit;
+
+import org.junit.jupiter.api.Test;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+class CollectResultTest {
+
+ @Test
+ void testConcurrentAddException() throws Exception {
Review Comment:
⚠️ **Missing concurrent test for `addCycle()`/`getCycles()`.**
This test only validates concurrent `addException()` calls. The
`addCycle()`/`getCycles()` pair has the same synchronization logic — and the
same mutable-escape problem — but is left uncovered by a concurrent test. A
regression in cycle-collection synchronization would pass unnoticed.
Add an analogous `testConcurrentAddCycle()` (or parameterize this test to
cover both collection types).
--
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]
