diff --git a/maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/DefaultRepositorySystem.java b/maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/DefaultRepositorySystem.java index 915df50f6..45336315c 100644 --- a/maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/DefaultRepositorySystem.java +++ b/maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/DefaultRepositorySystem.java @@ -220,55 +220,88 @@ public DefaultRepositorySystem( public VersionResult resolveVersion(RepositorySystemSession session, VersionRequest request) throws VersionResolutionException { requireNonNull(request, "request cannot be null"); - if (!isReentrant(request.getTrace())) { + Runnable exitGuard = null; + if (!isReentrant(request.getTrace(), session)) { validateSession(session); repositorySystemValidator.validateVersionRequest(session, request); request.setTrace(stampReentrancyMarker(request.getTrace())); + exitGuard = enterSessionScope(session); + } + try { + return versionResolver.resolveVersion(session, request); + } finally { + if (exitGuard != null) { + exitGuard.run(); + } } - return versionResolver.resolveVersion(session, request); } @Override public VersionRangeResult resolveVersionRange(RepositorySystemSession session, VersionRangeRequest request) throws VersionRangeResolutionException { requireNonNull(request, "request cannot be null"); - if (!isReentrant(request.getTrace())) { + Runnable exitGuard = null; + if (!isReentrant(request.getTrace(), session)) { validateSession(session); repositorySystemValidator.validateVersionRangeRequest(session, request); request.setTrace(stampReentrancyMarker(request.getTrace())); + exitGuard = enterSessionScope(session); + } + try { + return versionRangeResolver.resolveVersionRange(session, request); + } finally { + if (exitGuard != null) { + exitGuard.run(); + } } - return versionRangeResolver.resolveVersionRange(session, request); } @Override public ArtifactDescriptorResult readArtifactDescriptor( RepositorySystemSession session, ArtifactDescriptorRequest request) throws ArtifactDescriptorException { requireNonNull(request, "request cannot be null"); - boolean outermost = !isReentrant(request.getTrace()); + boolean outermost = !isReentrant(request.getTrace(), session); + Runnable exitGuard = null; if (outermost) { validateSession(session); repositorySystemValidator.validateArtifactDescriptorRequest(session, request); request.setTrace(stampReentrancyMarker(request.getTrace())); + exitGuard = enterSessionScope(session); } - ArtifactDescriptorResult descriptorResult = artifactDescriptorReader.readArtifactDescriptor(session, request); - if (outermost) { - for (ArtifactDecorator decorator : Utils.getArtifactDecorators(session, artifactDecoratorFactories)) { - descriptorResult.setArtifact(decorator.decorateArtifact(descriptorResult)); + try { + ArtifactDescriptorResult descriptorResult = + artifactDescriptorReader.readArtifactDescriptor(session, request); + if (outermost) { + for (ArtifactDecorator decorator : Utils.getArtifactDecorators(session, artifactDecoratorFactories)) { + descriptorResult.setArtifact(decorator.decorateArtifact(descriptorResult)); + } + } + return descriptorResult; + } finally { + if (exitGuard != null) { + exitGuard.run(); } } - return descriptorResult; } @Override public ArtifactResult resolveArtifact(RepositorySystemSession session, ArtifactRequest request) throws ArtifactResolutionException { requireNonNull(request, "request cannot be null"); - if (!isReentrant(request.getTrace())) { + Runnable exitGuard = null; + if (!isReentrant(request.getTrace(), session)) { validateSession(session); repositorySystemValidator.validateArtifactRequests(session, Collections.singleton(request)); request.setTrace(stampReentrancyMarker(request.getTrace())); + exitGuard = enterSessionScope(session); + } + try { + return artifactResolver.resolveArtifact(session, request); + } finally { + if (exitGuard != null) { + exitGuard.run(); + } } - return artifactResolver.resolveArtifact(session, request); } @Override @@ -282,14 +315,22 @@ public List resolveArtifacts( .filter(Objects::nonNull) .findFirst() .orElse(null); - if (!isReentrant(firstTrace)) { + Runnable exitGuard = null; + if (!isReentrant(firstTrace, session)) { validateSession(session); repositorySystemValidator.validateArtifactRequests(session, requests); for (ArtifactRequest request : requests) { request.setTrace(stampReentrancyMarker(request.getTrace())); } + exitGuard = enterSessionScope(session); + } + try { + return artifactResolver.resolveArtifacts(session, requests); + } finally { + if (exitGuard != null) { + exitGuard.run(); + } } - return artifactResolver.resolveArtifacts(session, requests); } @Override @@ -302,96 +343,120 @@ public List resolveMetadata( .filter(Objects::nonNull) .findFirst() .orElse(null); - if (!isReentrant(firstTrace)) { + Runnable exitGuard = null; + if (!isReentrant(firstTrace, session)) { validateSession(session); repositorySystemValidator.validateMetadataRequests(session, requests); for (MetadataRequest request : requests) { request.setTrace(stampReentrancyMarker(request.getTrace())); } + exitGuard = enterSessionScope(session); + } + try { + return metadataResolver.resolveMetadata(session, requests); + } finally { + if (exitGuard != null) { + exitGuard.run(); + } } - return metadataResolver.resolveMetadata(session, requests); } @Override public CollectResult collectDependencies(RepositorySystemSession session, CollectRequest request) throws DependencyCollectionException { requireNonNull(request, "request cannot be null"); - if (!isReentrant(request.getTrace())) { + Runnable exitGuard = null; + if (!isReentrant(request.getTrace(), session)) { validateSession(session); repositorySystemValidator.validateCollectRequest(session, request); request.setTrace(stampReentrancyMarker(request.getTrace())); + exitGuard = enterSessionScope(session); + } + try { + return dependencyCollector.collectDependencies(session, request); + } finally { + if (exitGuard != null) { + exitGuard.run(); + } } - return dependencyCollector.collectDependencies(session, request); } @Override public DependencyResult resolveDependencies(RepositorySystemSession session, DependencyRequest request) throws DependencyResolutionException { requireNonNull(request, "request cannot be null"); - if (!isReentrant(request.getTrace())) { + Runnable exitGuard = null; + if (!isReentrant(request.getTrace(), session)) { validateSession(session); repositorySystemValidator.validateDependencyRequest(session, request); request.setTrace(stampReentrancyMarker(request.getTrace())); + exitGuard = enterSessionScope(session); } - RequestTrace trace = RequestTrace.newChild(request.getTrace(), request); - - DependencyResult result = new DependencyResult(request); - - DependencyCollectionException dce = null; - ArtifactResolutionException are = null; + try { + RequestTrace trace = RequestTrace.newChild(request.getTrace(), request); + + DependencyResult result = new DependencyResult(request); + + DependencyCollectionException dce = null; + ArtifactResolutionException are = null; + + if (request.getRoot() != null) { + result.setRoot(request.getRoot()); + } else if (request.getCollectRequest() != null) { + CollectResult collectResult; + try { + request.getCollectRequest().setTrace(trace); + collectResult = dependencyCollector.collectDependencies(session, request.getCollectRequest()); + } catch (DependencyCollectionException e) { + dce = e; + collectResult = e.getResult(); + } + result.setRoot(collectResult.getRoot()); + result.setCycles(collectResult.getCycles()); + result.setCollectExceptions(collectResult.getExceptions()); + } else { + throw new NullPointerException("dependency node and collect request cannot be null"); + } - if (request.getRoot() != null) { - result.setRoot(request.getRoot()); - } else if (request.getCollectRequest() != null) { - CollectResult collectResult; + final List dependencyNodes = + doFlattenDependencyNodes(session, result.getRoot(), request.getFilter()); + + final List requests = dependencyNodes.stream() + .map(n -> { + if (n.getDependency() != null) { + ArtifactRequest artifactRequest = new ArtifactRequest(n); + artifactRequest.setTrace(trace); + return artifactRequest; + } else { + return null; + } + }) + .filter(Objects::nonNull) + .collect(Collectors.toList()); + List results; try { - request.getCollectRequest().setTrace(trace); - collectResult = dependencyCollector.collectDependencies(session, request.getCollectRequest()); - } catch (DependencyCollectionException e) { - dce = e; - collectResult = e.getResult(); + results = artifactResolver.resolveArtifacts(session, requests); + } catch (ArtifactResolutionException e) { + are = e; + results = e.getResults(); } - result.setRoot(collectResult.getRoot()); - result.setCycles(collectResult.getCycles()); - result.setCollectExceptions(collectResult.getExceptions()); - } else { - throw new NullPointerException("dependency node and collect request cannot be null"); - } + result.setDependencyNodeResults(dependencyNodes); + result.setArtifactResults(results); - final List dependencyNodes = - doFlattenDependencyNodes(session, result.getRoot(), request.getFilter()); - - final List requests = dependencyNodes.stream() - .map(n -> { - if (n.getDependency() != null) { - ArtifactRequest artifactRequest = new ArtifactRequest(n); - artifactRequest.setTrace(trace); - return artifactRequest; - } else { - return null; - } - }) - .filter(Objects::nonNull) - .collect(Collectors.toList()); - List results; - try { - results = artifactResolver.resolveArtifacts(session, requests); - } catch (ArtifactResolutionException e) { - are = e; - results = e.getResults(); - } - result.setDependencyNodeResults(dependencyNodes); - result.setArtifactResults(results); + updateNodesWithResolvedArtifacts(results); - updateNodesWithResolvedArtifacts(results); + if (dce != null) { + throw new DependencyResolutionException(result, dce); + } else if (are != null) { + throw new DependencyResolutionException(result, are); + } - if (dce != null) { - throw new DependencyResolutionException(result, dce); - } else if (are != null) { - throw new DependencyResolutionException(result, are); + return result; + } finally { + if (exitGuard != null) { + exitGuard.run(); + } } - - return result; } @Override @@ -557,6 +622,19 @@ public void shutdown() { } } + /** + * Thread-scoped re-entrancy depth counter. This supplements the {@link RequestTrace}-based + * detection for consumers that rebuild the trace chain from a different tracing system + * (e.g. Maven 4's {@code RequestTraceHelper} converts between Maven API traces and + * resolver traces, losing the {@link #REPOSITORY_SYSTEM_CALL} marker). + *

+ * Re-entrancy is inherently per-call-stack (per-thread), so a {@code ThreadLocal} is the + * correct semantic. A value > 0 on entry means the current thread is already inside + * a {@code RepositorySystem} public method. Unlike a session-scoped counter, this avoids + * false positives in parallel builds where multiple threads share a single session. + */ + private static final ThreadLocal REENTRY_DEPTH = ThreadLocal.withInitial(() -> new int[] {0}); + /** * Stamps the {@link #REPOSITORY_SYSTEM_CALL} re-entrancy marker into the trace chain * while preserving the original trace tip data. The marker is inserted below @@ -587,6 +665,37 @@ private static boolean isReentrant(RequestTrace trace) { return false; } + /** + * Combined re-entrancy check using both {@link RequestTrace} ancestry and thread-scoped + * depth tracking. Either mechanism detecting re-entrancy is sufficient to skip validation. + *

+ * The trace-based check is the primary mechanism and works when callers properly propagate + * traces. The thread-scoped check is a fallback for callers that rebuild the trace chain + * from a different tracing system (e.g. Maven 4's trace conversion loses the resolver's + * re-entrancy marker). + * + * @param trace the current request trace (may be {@code null}) + * @param session the current repository system session (unused, kept for signature consistency) + * @return {@code true} if this is a re-entrant call, {@code false} if it is the outermost call + */ + private static boolean isReentrant(RequestTrace trace, RepositorySystemSession session) { + return isReentrant(trace) || REENTRY_DEPTH.get()[0] > 0; + } + + /** + * Increments the thread-scoped re-entrancy depth counter. Must be called on every outermost + * entry into a public {@code RepositorySystem} method, and the returned {@link Runnable} must + * be invoked in a {@code finally} block to decrement the counter on exit. + * + * @param session the current repository system session (unused, kept for signature consistency) + * @return a {@link Runnable} that decrements the depth counter when invoked + */ + private static Runnable enterSessionScope(RepositorySystemSession session) { + int[] depth = REENTRY_DEPTH.get(); + depth[0]++; + return () -> depth[0]--; + } + private void validateSession(RepositorySystemSession session) { requireNonNull(session, "repository system session cannot be null"); invalidSession(session.getLocalRepositoryManager(), "local repository manager"); diff --git a/maven-resolver-impl/src/test/java/org/eclipse/aether/internal/impl/DefaultRepositorySystemReentrancyTest.java b/maven-resolver-impl/src/test/java/org/eclipse/aether/internal/impl/DefaultRepositorySystemReentrancyTest.java index 6aa9e10de..8e39d7465 100644 --- a/maven-resolver-impl/src/test/java/org/eclipse/aether/internal/impl/DefaultRepositorySystemReentrancyTest.java +++ b/maven-resolver-impl/src/test/java/org/eclipse/aether/internal/impl/DefaultRepositorySystemReentrancyTest.java @@ -21,11 +21,15 @@ import java.util.Collections; import java.util.concurrent.atomic.AtomicBoolean; import java.util.concurrent.atomic.AtomicInteger; +import java.util.concurrent.atomic.AtomicReference; import org.eclipse.aether.DefaultRepositorySystemSession; import org.eclipse.aether.RepositorySystemSession; import org.eclipse.aether.RequestTrace; import org.eclipse.aether.artifact.DefaultArtifact; +import org.eclipse.aether.collection.CollectRequest; +import org.eclipse.aether.collection.CollectResult; +import org.eclipse.aether.graph.Dependency; import org.eclipse.aether.impl.ArtifactResolver; import org.eclipse.aether.impl.DependencyCollector; import org.eclipse.aether.impl.Deployer; @@ -57,7 +61,7 @@ public class DefaultRepositorySystemReentrancyTest { /** - * A validator that rejects any artifact whose version contains "${". + * A validator that rejects any artifact or dependency whose version contains "${". * This simulates Maven's MavenValidator rejecting uninterpolated expressions. */ private static final ValidatorFactory EXPRESSION_REJECTING_VALIDATOR_FACTORY = session -> new Validator() { @@ -67,6 +71,11 @@ public void validateArtifact(org.eclipse.aether.artifact.Artifact artifact) { throw new IllegalArgumentException("Uninterpolated expression in version: " + artifact.getVersion()); } } + + @Override + public void validateDependency(Dependency dependency) { + validateArtifact(dependency.getArtifact()); + } }; private DefaultRepositorySystem system; @@ -303,4 +312,164 @@ void outerCallWithNullTraceStillStampsMarker() throws Exception { system.resolveVersionRange(session, innerRequest); assertEquals(countBefore, validationCount.get(), "Re-entrant call should skip validation"); } + + @Test + void threadScopedDetectionSkipsValidationWhenTraceChainIsBroken() throws Exception { + // Simulates Maven 4's flow where the model builder converts between Maven API traces + // and resolver traces via RequestTraceHelper, losing the REPOSITORY_SYSTEM_CALL marker. + // + // The DependencyCollector, called from within collectDependencies, re-enters + // RepositorySystem.resolveVersionRange with a FRESH trace (no marker in ancestry) + // and an uninterpolated expression like ${project.version}. Without thread-scoped + // detection, the validator would reject the expression; with it, the call is + // detected as re-entrant and validation is skipped. + AtomicReference systemRef = new AtomicReference<>(); + + DependencyCollector reentrantCollector = (s, request) -> { + // Inside collectDependencies, simulate a re-entrant call with a FRESH trace + // (no marker in ancestry — this is the broken path) and an uninterpolated + // expression. If thread-scoped detection fails, the validator rejects this. + VersionRangeRequest innerRequest = new VersionRangeRequest( + new DefaultArtifact("g:inner:${project.version}"), Collections.emptyList(), null); + // Fresh trace — no REPOSITORY_SYSTEM_CALL marker (simulates Maven's trace conversion) + innerRequest.setTrace(RequestTrace.newChild(null, "MavenModelResolver")); + try { + systemRef.get().resolveVersionRange(s, innerRequest); + } catch (Exception e) { + fail("Re-entrant call with broken trace chain should succeed via thread-scoped detection", e); + } + return new CollectResult(request); + }; + + DefaultRepositorySystem strictSystem = new DefaultRepositorySystem( + new StubVersionResolver(), + new StubVersionRangeResolver(), + mock(ArtifactResolver.class), + mock(MetadataResolver.class), + new StubArtifactDescriptorReader(), + reentrantCollector, + mock(Installer.class), + mock(Deployer.class), + mock(LocalRepositoryProvider.class), + new StubSyncContextFactory(), + new DefaultRemoteRepositoryManager( + new DefaultUpdatePolicyAnalyzer(), + new DefaultChecksumPolicyProvider(), + new DefaultRepositoryKeyFunctionFactory()), + new DefaultRepositorySystemLifecycle(), + Collections.emptyMap(), + new DefaultRepositorySystemValidator( + Collections.singletonList(EXPRESSION_REJECTING_VALIDATOR_FACTORY))); + systemRef.set(strictSystem); + + // The outermost collectDependencies call should succeed, and the inner + // resolveVersionRange call (with broken trace and uninterpolated expression) + // should be detected as re-entrant via the thread-scoped depth counter. + CollectRequest collectRequest = new CollectRequest(); + collectRequest.setRootArtifact(new DefaultArtifact("g:root:1.0")); + + assertDoesNotThrow( + () -> strictSystem.collectDependencies(session, collectRequest), + "Inner call with broken trace chain should succeed via thread-scoped detection"); + } + + @Test + void collectDependenciesStillRejectsInvalidDirectDependencies() throws Exception { + // Verify that direct dependencies (not managed) ARE still validated + DependencyCollector passThroughCollector = (s, request) -> new CollectResult(request); + + DefaultRepositorySystem strictSystem = new DefaultRepositorySystem( + new StubVersionResolver(), + new StubVersionRangeResolver(), + mock(ArtifactResolver.class), + mock(MetadataResolver.class), + new StubArtifactDescriptorReader(), + passThroughCollector, + mock(Installer.class), + mock(Deployer.class), + mock(LocalRepositoryProvider.class), + new StubSyncContextFactory(), + new DefaultRemoteRepositoryManager( + new DefaultUpdatePolicyAnalyzer(), + new DefaultChecksumPolicyProvider(), + new DefaultRepositoryKeyFunctionFactory()), + new DefaultRepositorySystemLifecycle(), + Collections.emptyMap(), + new DefaultRepositorySystemValidator( + Collections.singletonList(EXPRESSION_REJECTING_VALIDATOR_FACTORY))); + + // Direct dependency with uninterpolated expression should still be rejected + CollectRequest collectRequest = new CollectRequest(); + collectRequest.setRootArtifact(new DefaultArtifact("g:project:1.0")); + collectRequest.addDependency( + new Dependency(new DefaultArtifact("org.example:lib:${undefined.version}"), "compile")); + + assertThrows( + IllegalArgumentException.class, + () -> strictSystem.collectDependencies(session, collectRequest), + "Direct dependencies with uninterpolated expressions should still be rejected"); + } + + @Test + void threadScopedDepthIsProperlyDecrementedOnExit() throws Exception { + // Verify that the thread-scoped depth counter is properly decremented after + // a RepositorySystem call completes, so independent calls are still validated + VersionRangeRequest request1 = + new VersionRangeRequest(new DefaultArtifact("g:a:1.0"), Collections.emptyList(), null); + VersionRangeRequest request2 = + new VersionRangeRequest(new DefaultArtifact("g:b:2.0"), Collections.emptyList(), null); + + system.resolveVersionRange(session, request1); + int countAfterFirst = validationCount.get(); + assertEquals(1, countAfterFirst, "First call should validate"); + + // Second independent call (no shared trace) should also validate + // because the depth counter should have been decremented on exit + system.resolveVersionRange(session, request2); + assertEquals(2, validationCount.get(), "Second independent call should also validate"); + } + + @Test + void threadScopedDepthIsDecrementedWhenDelegateThrows() throws Exception { + // Verify that the depth counter is properly cleaned up even when the delegate + // throws an exception, ensuring subsequent outermost calls are still validated. + DependencyCollector throwingCollector = (s, request) -> { + throw new RuntimeException("Simulated delegate failure"); + }; + + DefaultRepositorySystem throwingSystem = new DefaultRepositorySystem( + new StubVersionResolver(), + new StubVersionRangeResolver(), + mock(ArtifactResolver.class), + mock(MetadataResolver.class), + new StubArtifactDescriptorReader(), + throwingCollector, + mock(Installer.class), + mock(Deployer.class), + mock(LocalRepositoryProvider.class), + new StubSyncContextFactory(), + new DefaultRemoteRepositoryManager( + new DefaultUpdatePolicyAnalyzer(), + new DefaultChecksumPolicyProvider(), + new DefaultRepositoryKeyFunctionFactory()), + new DefaultRepositorySystemLifecycle(), + Collections.emptyMap(), + new DefaultRepositorySystemValidator( + Collections.singletonList(EXPRESSION_REJECTING_VALIDATOR_FACTORY))); + + // First call: collectDependencies should throw because the collector throws + CollectRequest collectRequest = new CollectRequest(); + collectRequest.setRootArtifact(new DefaultArtifact("g:root:1.0")); + assertThrows(RuntimeException.class, () -> throwingSystem.collectDependencies(session, collectRequest)); + + // Second call: resolveVersionRange should still validate (depth counter was reset + // by the try-finally guard despite the exception). If the depth counter leaked, + // this call would skip validation and accept the uninterpolated expression. + VersionRangeRequest request = + new VersionRangeRequest(new DefaultArtifact("g:bad:${unresolved}"), Collections.emptyList(), null); + assertThrows( + IllegalArgumentException.class, + () -> throwingSystem.resolveVersionRange(session, request), + "Depth counter should be reset after exception — validation must still run"); + } }