Copilot commented on code in PR #2009:
URL: https://github.com/apache/maven-resolver/pull/2009#discussion_r3645488199
##########
maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/DefaultRepositorySystem.java:
##########
@@ -587,6 +670,41 @@ private static boolean isReentrant(RequestTrace trace) {
return false;
}
+ /**
+ * Combined re-entrancy check using both {@link RequestTrace} ancestry and
session-scoped
+ * depth tracking. Either mechanism detecting re-entrancy is sufficient to
skip validation.
+ * <p>
+ * The trace-based check is the primary mechanism and works when callers
properly propagate
+ * traces. The session-based 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
+ * @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) || getReentryDepth(session).get() > 0;
+ }
+
+ /**
+ * Increments the session-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
+ * @return a {@link Runnable} that decrements the depth counter when
invoked
+ */
+ private static Runnable enterSessionScope(RepositorySystemSession session)
{
+ AtomicInteger depth = getReentryDepth(session);
+ depth.incrementAndGet();
+ return depth::decrementAndGet;
+ }
+
+ private static AtomicInteger getReentryDepth(RepositorySystemSession
session) {
+ return (AtomicInteger)
session.getData().computeIfAbsent(SESSION_REENTRY_DEPTH_KEY, () -> new
AtomicInteger(0));
+ }
Review Comment:
The session-scoped `AtomicInteger` depth counter is shared across all
threads using the same `RepositorySystemSession`. If the session is used
concurrently (parallel resolution), one thread can make another thread’s
*non-reentrant* call look re-entrant (`depth > 0`), causing validation to be
skipped incorrectly. Consider making the depth tracking thread-scoped (e.g.,
store a `ThreadLocal<Integer>`/`ThreadLocal<AtomicInteger>` in session data, or
store a per-thread counter keyed by `Thread.currentThread()`/thread id with
proper cleanup) so only true call-stack re-entrancy on the same thread
suppresses validation. Update the Javadoc to reflect the chosen semantics.
##########
maven-resolver-impl/src/test/java/org/eclipse/aether/internal/impl/DefaultRepositorySystemReentrancyTest.java:
##########
@@ -303,4 +312,120 @@ void outerCallWithNullTraceStillStampsMarker() throws
Exception {
system.resolveVersionRange(session, innerRequest);
assertEquals(countBefore, validationCount.get(), "Re-entrant call
should skip validation");
}
+
+ @Test
+ void sessionScopedDetectionSkipsValidationWhenTraceChainIsBroken() 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
session-scoped
+ // detection, the validator would reject the expression; with it, the
call is
+ // detected as re-entrant and validation is skipped.
+ AtomicReference<DefaultRepositorySystem> 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 session-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 session-scoped detection: " + e);
+ }
Review Comment:
Catching `Exception` and concatenating `e` into the failure message drops
the stack trace from the assertion output, which makes failures harder to
diagnose. Prefer `fail(message, e)` (or an assertion that preserves the
throwable) so the original exception is reported with full context.
--
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]
