gnodet commented on code in PR #2132:
URL: https://github.com/apache/maven-resolver/pull/2132#discussion_r3961179907
##########
maven-resolver-util/src/main/java/org/eclipse/aether/util/graph/transformer/ConflictResolver.java:
##########
@@ -254,18 +255,76 @@ public DependencyNode transformGraph(DependencyNode node,
DependencyGraphTransfo
throws RepositoryException {
String cf = ConfigUtils.getString(
context.getSession(), DEFAULT_CONFLICT_RESOLVER_IMPL,
CONFIG_PROP_CONFLICT_RESOLVER_IMPL);
+
ConflictResolver delegate;
- if (AUTO_CONFLICT_RESOLVER.equals(cf) ||
CLASSIC_CONFLICT_RESOLVER.equals(cf)) {
+
+ if (AUTO_CONFLICT_RESOLVER.equals(cf)) {
+ delegate = selectConflictResolver(node, context);
+ } else if (CLASSIC_CONFLICT_RESOLVER.equals(cf)) {
delegate = new ClassicConflictResolver(versionSelector,
scopeSelector, optionalitySelector, scopeDeriver);
} else if (PATH_CONFLICT_RESOLVER.equals(cf)) {
delegate = new PathConflictResolver(versionSelector,
scopeSelector, optionalitySelector, scopeDeriver);
} else {
throw new IllegalArgumentException("Unknown conflict resolver: " +
cf + "; known are "
+ Arrays.asList(AUTO_CONFLICT_RESOLVER,
PATH_CONFLICT_RESOLVER, CLASSIC_CONFLICT_RESOLVER));
}
+
return delegate.transformGraph(node, context);
}
+ /**
+ * Automatically selects the conflict resolver based on the estimated
memory requirements.
+ * PathConflictResolver is used for dependency trees that fit within the
memory threshold,
+ * while ClassicConflictResolver is used for larger trees.
+ */
+ private ConflictResolver selectConflictResolver(DependencyNode node,
DependencyGraphTransformationContext context)
+ throws RepositoryException {
+
+ if (context.get(TransformationContextKeys.CONFLICT_IDS) == null) {
+ new ConflictIdSorter().transformGraph(node, context);
+ }
Review Comment:
🔴 **Bug: wrong context key checked.** This checks `CONFLICT_IDS` (set by
`ConflictMarker`) but should check `SORTED_CONFLICT_IDS` (set by
`ConflictIdSorter`). The original code in PR #1938 correctly checked
`SORTED_CONFLICT_IDS`.
As written, if `ConflictMarker` has already run (populating `CONFLICT_IDS`)
but `ConflictIdSorter` hasn't, this guard passes and the sorter never runs. In
practice both delegate resolvers (`ClassicConflictResolver` line 119-124 and
`PathConflictResolver` line 141-146) independently check for
`SORTED_CONFLICT_IDS` and call `ConflictIdSorter` themselves, so this doesn't
cause a runtime failure — but it makes this block dead code that never triggers
when it should.
```suggestion
if (context.get(TransformationContextKeys.SORTED_CONFLICT_IDS) ==
null) {
new ConflictIdSorter().transformGraph(node, context);
}
```
##########
maven-resolver-util/src/main/java/org/eclipse/aether/util/graph/transformer/ConflictResolver.java:
##########
@@ -119,8 +120,8 @@ public class ConflictResolver implements
DependencyGraphTransformer {
/**
* The name of the conflict resolver implementation to use: "auto"
(default), "path", or "classic" (same as Maven 3).
* <p>
Review Comment:
💡 **Javadoc update is premature.** The class-level Javadoc (lines 46-53)
still says PathConflictResolver is "Not yet recommended for production" and
advises "All projects: Use ClassicConflictResolver for optimal correctness."
This Javadoc update claims auto mode intelligently selects the right one, but
the class-level docs discourage using PCR at all. Either the class-level docs
should be updated to match, or this Javadoc should stay conservative.
##########
maven-resolver-util/src/main/java/org/eclipse/aether/util/graph/transformer/ConflictResolver.java:
##########
@@ -254,18 +255,76 @@ public DependencyNode transformGraph(DependencyNode node,
DependencyGraphTransfo
throws RepositoryException {
String cf = ConfigUtils.getString(
context.getSession(), DEFAULT_CONFLICT_RESOLVER_IMPL,
CONFIG_PROP_CONFLICT_RESOLVER_IMPL);
+
ConflictResolver delegate;
- if (AUTO_CONFLICT_RESOLVER.equals(cf) ||
CLASSIC_CONFLICT_RESOLVER.equals(cf)) {
+
+ if (AUTO_CONFLICT_RESOLVER.equals(cf)) {
+ delegate = selectConflictResolver(node, context);
+ } else if (CLASSIC_CONFLICT_RESOLVER.equals(cf)) {
delegate = new ClassicConflictResolver(versionSelector,
scopeSelector, optionalitySelector, scopeDeriver);
} else if (PATH_CONFLICT_RESOLVER.equals(cf)) {
delegate = new PathConflictResolver(versionSelector,
scopeSelector, optionalitySelector, scopeDeriver);
} else {
throw new IllegalArgumentException("Unknown conflict resolver: " +
cf + "; known are "
+ Arrays.asList(AUTO_CONFLICT_RESOLVER,
PATH_CONFLICT_RESOLVER, CLASSIC_CONFLICT_RESOLVER));
}
+
return delegate.transformGraph(node, context);
}
+ /**
+ * Automatically selects the conflict resolver based on the estimated
memory requirements.
+ * PathConflictResolver is used for dependency trees that fit within the
memory threshold,
+ * while ClassicConflictResolver is used for larger trees.
+ */
+ private ConflictResolver selectConflictResolver(DependencyNode node,
DependencyGraphTransformationContext context)
+ throws RepositoryException {
+
+ if (context.get(TransformationContextKeys.CONFLICT_IDS) == null) {
+ new ConflictIdSorter().transformGraph(node, context);
+ }
+
+ Runtime rt = Runtime.getRuntime();
+ long available = rt.maxMemory() - (rt.totalMemory() - rt.freeMemory());
Review Comment:
⚠️ **Unreliable heap measurement.** `Runtime.freeMemory()` reflects the free
space in the *currently allocated* heap, not total available memory. Its value
fluctuates wildly depending on whether GC has recently run, allocation pressure
from other threads, and JVM ergonomics.
This means the same dependency graph can get `PathConflictResolver` on one
build and `ClassicConflictResolver` on the next, producing non-deterministic
behavior — a property Maven users don't expect from dependency resolution. The
original PR #1938 had this same issue but was accepted as a pragmatic trade-off
alongside extensive benchmarking and the PathConflictResolver memory
optimizations. Without those optimizations, the risk is higher.
Consider requesting a GC before measurement (`System.gc()` — advisory but
common for memory heuristics), or using a fixed configurable threshold instead
of runtime measurement.
##########
maven-resolver-util/src/main/java/org/eclipse/aether/util/graph/transformer/ConflictResolver.java:
##########
@@ -254,18 +255,76 @@ public DependencyNode transformGraph(DependencyNode node,
DependencyGraphTransfo
throws RepositoryException {
String cf = ConfigUtils.getString(
context.getSession(), DEFAULT_CONFLICT_RESOLVER_IMPL,
CONFIG_PROP_CONFLICT_RESOLVER_IMPL);
+
ConflictResolver delegate;
- if (AUTO_CONFLICT_RESOLVER.equals(cf) ||
CLASSIC_CONFLICT_RESOLVER.equals(cf)) {
+
+ if (AUTO_CONFLICT_RESOLVER.equals(cf)) {
+ delegate = selectConflictResolver(node, context);
+ } else if (CLASSIC_CONFLICT_RESOLVER.equals(cf)) {
delegate = new ClassicConflictResolver(versionSelector,
scopeSelector, optionalitySelector, scopeDeriver);
} else if (PATH_CONFLICT_RESOLVER.equals(cf)) {
delegate = new PathConflictResolver(versionSelector,
scopeSelector, optionalitySelector, scopeDeriver);
} else {
throw new IllegalArgumentException("Unknown conflict resolver: " +
cf + "; known are "
+ Arrays.asList(AUTO_CONFLICT_RESOLVER,
PATH_CONFLICT_RESOLVER, CLASSIC_CONFLICT_RESOLVER));
}
+
return delegate.transformGraph(node, context);
}
+ /**
+ * Automatically selects the conflict resolver based on the estimated
memory requirements.
+ * PathConflictResolver is used for dependency trees that fit within the
memory threshold,
+ * while ClassicConflictResolver is used for larger trees.
+ */
+ private ConflictResolver selectConflictResolver(DependencyNode node,
DependencyGraphTransformationContext context)
+ throws RepositoryException {
+
+ if (context.get(TransformationContextKeys.CONFLICT_IDS) == null) {
+ new ConflictIdSorter().transformGraph(node, context);
+ }
+
+ Runtime rt = Runtime.getRuntime();
+ long available = rt.maxMemory() - (rt.totalMemory() - rt.freeMemory());
+
+ // Estimate the maximum number of Path tree nodes that would fit in
25% of available heap.
+ // Each Path object costs ~200 bytes (object header + fields +
children list entry).
+ int maxPathNodes = (int) Math.min(available / (4L * 200),
Integer.MAX_VALUE);
+
+ // Walk the dependency tree to count total nodes (including
diamond-expanded duplicates).
+ // The Path tree mirrors this structure, so the count directly
reflects Path tree size.
+ // Use early-exit: stop counting once we exceed the threshold.
+ if (treeExceedsThreshold(node, maxPathNodes)) {
+ return new ClassicConflictResolver(versionSelector, scopeSelector,
optionalitySelector, scopeDeriver);
+ } else {
+ return new PathConflictResolver(versionSelector, scopeSelector,
optionalitySelector, scopeDeriver);
+ }
+ }
Review Comment:
⚠️ **Missing tests.** The original PR #1938 that introduced this logic
included 5 test cases covering: auto mode selection, explicit path/classic
config dispatch, unknown config rejection, and default config behavior. This PR
re-introduces the production code without any tests.
Given this code was removed for stability reasons, re-introducing it without
tests — especially for the memory threshold heuristic and the fallback behavior
— is risky. At minimum, tests should verify:
1. `auto` selects `PathConflictResolver` for small graphs
2. `auto` selects `ClassicConflictResolver` for large graphs
3. Explicit `path` and `classic` configs are unaffected
4. The tree-walking threshold check terminates early
--
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]
