slachiewicz commented on code in PR #2108:
URL: https://github.com/apache/maven-resolver/pull/2108#discussion_r3940211135
##########
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:
Addressed in a9d1d5c7 using your alternative suggestion: restored
try-with-resources by wrapping both contexts in a new package-private
`CloseOnceSyncContext` that delegates `close()` at most once. This covers the
creation window for both `RuntimeException` and `Error`, while `resolve()` can
still close the shared context early (before switching to exclusive) without
risking a double-close of the underlying context.
##########
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:
Same fix applied here in a9d1d5c7 — try-with-resources with the
`CloseOnceSyncContext` wrapper, so a failure while creating the exclusive
context closes the shared one, and the early close during the shared→exclusive
switch stays safe.
##########
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:
Good catch — fixed in 0d76d597: the test now uses two distinct counting
instances, with the factory dispatching on the `shared` flag, and asserts each
instance is closed exactly once.
##########
maven-resolver-util/src/main/java/org/eclipse/aether/util/listener/ChainedRepositoryListener.java:
##########
@@ -110,12 +110,14 @@ public void remove(RepositoryListener listener) {
}
}
Review Comment:
Imports cleaned up in 0d76d597 (also in `ChainedTransferListener` and the
tests). JUL is kept deliberately since `maven-resolver-util` has no SLF4J
dependency; happy to switch if adding the dependency is preferred.
--
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]
