gnodet commented on code in PR #2108:
URL: https://github.com/apache/maven-resolver/pull/2108#discussion_r3908432905
##########
maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/DefaultArtifactResolver.java:
##########
@@ -203,20 +203,18 @@ public List<ArtifactResult> resolveArtifacts(
throws ArtifactResolutionException {
requireNonNull(session, "session cannot be null");
requireNonNull(requests, "requests cannot be null");
- try (SyncContext shared = syncContextFactory.newInstance(session,
true);
- SyncContext exclusive =
syncContextFactory.newInstance(session, false)) {
- Collection<Artifact> artifacts = new ArrayList<>(requests.size());
- SystemDependencyScope systemDependencyScope =
session.getSystemDependencyScope();
- for (ArtifactRequest request : requests) {
- if (systemDependencyScope != null
- &&
systemDependencyScope.getSystemPath(request.getArtifact()) != null) {
- continue;
- }
- artifacts.add(request.getArtifact());
+ SyncContext shared = syncContextFactory.newInstance(session, true);
+ SyncContext exclusive = syncContextFactory.newInstance(session, false);
Review Comment:
**Resource leak (HIGH):** Removing try-with-resources introduces a resource
leak. If `syncContextFactory.newInstance(session, false)` on the next line
throws, the `shared` SyncContext created here is never closed — the `resolve()`
method's finally block only runs if `resolve()` is actually entered.
The original try-with-resources guaranteed both contexts were closed
regardless of where an exception occurred. Since `SyncContext` wraps named
locks, a leaked context could hold a lock open.
One approach:
```java
SyncContext shared = syncContextFactory.newInstance(session, true);
SyncContext exclusive;
try {
exclusive = syncContextFactory.newInstance(session, false);
} catch (RuntimeException | Error e) {
shared.close();
throw e;
}
```
Or alternatively, keep try-with-resources for the creation but move the
close logic into the `resolve()` method's finally block using a pattern that
handles both the try-with-resources close and the explicit close.
##########
maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/DefaultMetadataResolver.java:
##########
@@ -139,15 +139,14 @@ public List<MetadataResult> resolveMetadata(
RepositorySystemSession session, Collection<? extends
MetadataRequest> requests) {
requireNonNull(session, "session cannot be null");
requireNonNull(requests, "requests cannot be null");
- try (SyncContext shared = syncContextFactory.newInstance(session,
true);
- SyncContext exclusive =
syncContextFactory.newInstance(session, false)) {
- Collection<Metadata> metadata = new ArrayList<>(requests.size());
- for (MetadataRequest request : requests) {
- metadata.add(request.getMetadata());
- }
-
- return resolve(shared, exclusive, metadata, session, requests);
+ SyncContext shared = syncContextFactory.newInstance(session, true);
+ SyncContext exclusive = syncContextFactory.newInstance(session, false);
Review Comment:
**Resource leak (HIGH):** Same pattern as `DefaultArtifactResolver` — if
`newInstance(session, false)` throws, the `shared` context from the previous
line is never closed. The `resolve()` method's finally block only runs if
`resolve()` is entered.
##########
maven-resolver-util/src/main/java/org/eclipse/aether/util/listener/ChainedRepositoryListener.java:
##########
@@ -110,12 +110,14 @@ public void remove(RepositoryListener listener) {
}
}
Review Comment:
**JUL vs SLF4J (LOW):** This uses `java.util.logging.Logger` while the
project's `maven-resolver-impl` module uses SLF4J exclusively. The choice is
understandable since `maven-resolver-util` doesn't depend on SLF4J, but these
WARNING messages will not appear in SLF4J-configured logging setups unless a
JUL-to-SLF4J bridge is installed.
Also, several fully-qualified class names are used inline
(`java.util.logging.Logger`, `java.util.logging.Level.WARNING`) instead of
proper import statements — same applies in `ChainedTransferListener.java` and
the test files. Using imports would be more consistent with the rest of the
codebase.
##########
maven-resolver-impl/src/test/java/org/eclipse/aether/internal/impl/DefaultArtifactResolverTest.java:
##########
@@ -1083,4 +1083,39 @@ public void add(RepositorySystemSession session,
LocalMetadataRegistration reque
// message should contain present=true, available=false, filter message
assertTrue(ex.getMessage().contains("gid:aid:ext:ver (present, but
unavailable): REFUSED"));
}
+
+ @Test
+ void testSyncContextIsClosedExactlyOnce() throws Exception {
+ java.util.concurrent.atomic.AtomicInteger closeCount = new
java.util.concurrent.atomic.AtomicInteger(0);
+
+ org.eclipse.aether.SyncContext countingSyncContext = new
org.eclipse.aether.SyncContext() {
+ @Override
+ public void acquire(
+ Collection<? extends Artifact> artifacts,
+ Collection<? extends org.eclipse.aether.metadata.Metadata>
metadatas) {}
+
+ @Override
+ public void close() {
+ closeCount.incrementAndGet();
Review Comment:
**Misleading test (MEDIUM):** The factory `(s, shared) ->
countingSyncContext` ignores its parameters and always returns the same
instance. So `shared` and `exclusive` inside the resolver both point to the
same object.
The comment on line 1119 says "2 instances were created" but only 1 exists —
it is closed twice. If the code had a bug where it closed one context twice and
the other never, this test would not catch it.
Consider using two separate instances with separate counters:
```java
AtomicInteger sharedCloseCount = new AtomicInteger();
AtomicInteger exclusiveCloseCount = new AtomicInteger();
// ... factory returns distinct instances based on the `shared` parameter
```
##########
maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/DefaultArtifactResolver.java:
##########
@@ -203,20 +203,18 @@ public List<ArtifactResult> resolveArtifacts(
throws ArtifactResolutionException {
requireNonNull(session, "session cannot be null");
requireNonNull(requests, "requests cannot be null");
- try (SyncContext shared = syncContextFactory.newInstance(session,
true);
- SyncContext exclusive =
syncContextFactory.newInstance(session, false)) {
- Collection<Artifact> artifacts = new ArrayList<>(requests.size());
- SystemDependencyScope systemDependencyScope =
session.getSystemDependencyScope();
- for (ArtifactRequest request : requests) {
- if (systemDependencyScope != null
- &&
systemDependencyScope.getSystemPath(request.getArtifact()) != null) {
- continue;
- }
- artifacts.add(request.getArtifact());
+ SyncContext shared = syncContextFactory.newInstance(session, true);
+ SyncContext exclusive = syncContextFactory.newInstance(session, false);
Review Comment:
**Resource leak (HIGH):** Removing try-with-resources introduces a resource
leak. If `syncContextFactory.newInstance(session, false)` on the next line
throws, the `shared` SyncContext created here is never closed — the `resolve()`
method's finally block only runs if `resolve()` is actually entered.
The original try-with-resources guaranteed both contexts were closed
regardless of where an exception occurred. Since `SyncContext` wraps named
locks, a leaked context could hold a lock open.
One approach:
```java
SyncContext shared = syncContextFactory.newInstance(session, true);
SyncContext exclusive;
try {
exclusive = syncContextFactory.newInstance(session, false);
} catch (RuntimeException | Error e) {
shared.close();
throw e;
}
```
Or alternatively, keep try-with-resources for the creation but move the
close logic into the `resolve()` method's finally block using a pattern that
handles both the try-with-resources close and the explicit close.
##########
maven-resolver-impl/src/test/java/org/eclipse/aether/internal/impl/DefaultArtifactResolverTest.java:
##########
@@ -1083,4 +1083,39 @@ public void add(RepositorySystemSession session,
LocalMetadataRegistration reque
// message should contain present=true, available=false, filter message
assertTrue(ex.getMessage().contains("gid:aid:ext:ver (present, but
unavailable): REFUSED"));
}
+
+ @Test
+ void testSyncContextIsClosedExactlyOnce() throws Exception {
+ java.util.concurrent.atomic.AtomicInteger closeCount = new
java.util.concurrent.atomic.AtomicInteger(0);
+
+ org.eclipse.aether.SyncContext countingSyncContext = new
org.eclipse.aether.SyncContext() {
+ @Override
+ public void acquire(
+ Collection<? extends Artifact> artifacts,
+ Collection<? extends org.eclipse.aether.metadata.Metadata>
metadatas) {}
+
+ @Override
+ public void close() {
+ closeCount.incrementAndGet();
Review Comment:
**Misleading test (MEDIUM):** The factory `(s, shared) ->
countingSyncContext` ignores its parameters and always returns the same
instance. So `shared` and `exclusive` inside the resolver both point to the
same object.
The comment on line 1119 says "2 instances were created" but only 1 exists —
it is closed twice. If the code had a bug where it closed one context twice and
the other never, this test would not catch it.
Consider using two separate instances with separate counters:
```java
AtomicInteger sharedCloseCount = new AtomicInteger();
AtomicInteger exclusiveCloseCount = new AtomicInteger();
// ... factory returns distinct instances based on the `shared` parameter
```
##########
maven-resolver-util/src/main/java/org/eclipse/aether/util/listener/ChainedRepositoryListener.java:
##########
@@ -110,12 +110,14 @@ public void remove(RepositoryListener listener) {
}
}
Review Comment:
**JUL vs SLF4J (LOW):** This uses `java.util.logging.Logger` while the
project's `maven-resolver-impl` module uses SLF4J exclusively. The choice is
understandable since `maven-resolver-util` doesn't depend on SLF4J, but these
WARNING messages will not appear in SLF4J-configured logging setups unless a
JUL-to-SLF4J bridge is installed.
Also, several fully-qualified class names are used inline
(`java.util.logging.Logger`, `java.util.logging.Level.WARNING`) instead of
proper import statements — same applies in `ChainedTransferListener.java` and
the test files. Using imports would be more consistent with the rest of the
codebase.
##########
maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/DefaultMetadataResolver.java:
##########
@@ -139,15 +139,14 @@ public List<MetadataResult> resolveMetadata(
RepositorySystemSession session, Collection<? extends
MetadataRequest> requests) {
requireNonNull(session, "session cannot be null");
requireNonNull(requests, "requests cannot be null");
- try (SyncContext shared = syncContextFactory.newInstance(session,
true);
- SyncContext exclusive =
syncContextFactory.newInstance(session, false)) {
- Collection<Metadata> metadata = new ArrayList<>(requests.size());
- for (MetadataRequest request : requests) {
- metadata.add(request.getMetadata());
- }
-
- return resolve(shared, exclusive, metadata, session, requests);
+ SyncContext shared = syncContextFactory.newInstance(session, true);
+ SyncContext exclusive = syncContextFactory.newInstance(session, false);
Review Comment:
**Resource leak (HIGH):** Same pattern as `DefaultArtifactResolver` — if
`newInstance(session, false)` throws, the `shared` context from the previous
line is never closed. The `resolve()` method's finally block only runs if
`resolve()` is entered.
--
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]
