gnodet-bot commented on code in PR #13570:
URL: https://github.com/apache/maven/pull/13570#discussion_r4322865567
##########
impl/maven-core/src/main/java/org/apache/maven/internal/impl/DefaultTypeRegistry.java:
##########
@@ -79,7 +80,9 @@ public Optional<Type> lookup(String id) {
public Type require(String id) {
requireNonNull(id, "id cannot be null");
return usedTypes.computeIfAbsent(id, i -> {
+ ClassLoader tccl = Thread.currentThread().getContextClassLoader();
Type type = lookup.lookupList(TypeProvider.class).stream()
+ .filter(p -> ExtensibleEnumRegistries.isVisibleFrom(p,
tccl))
Review Comment:
⚠️ **Caching defeats the classloader filter.** `computeIfAbsent(id, i -> {
... })` runs the lambda — including the TCCL capture and `isVisibleFrom` filter
— only on the *first* lookup of a given `id`. All subsequent calls return the
cached `Type` without re-checking the calling thread's classloader. In a
parallel reactor build, module A's first lookup caches the result, and module
B's lookup of the same id reuses that cached value even if the provider is not
visible from module B's classloader.
The filter must be moved outside `computeIfAbsent`, to the call site that
needs per-call TCCL semantics. The `usedTypes` cache was designed to avoid
repeated SPI enumeration — but it conflates two things: caching the *provider
lookup* and caching the *visibility decision*. The latter must be per-call.
##########
impl/maven-impl/src/main/java/org/apache/maven/impl/ExtensibleEnumRegistries.java:
##########
@@ -89,15 +152,57 @@ public abstract static class DefaultExtensibleEnumRegistry<
implements ExtensibleEnumRegistry<T> {
protected final Map<String, T> values;
+ private final Lookup lookup;
+ private final Class<P> providerType;
+ @SafeVarargs
public DefaultExtensibleEnumRegistry(List<P> providers, T...
builtinValues) {
+ this(null, null, providers, builtinValues);
+ }
+
+ @SafeVarargs
+ public DefaultExtensibleEnumRegistry(
+ Lookup lookup, Class<P> providerType, List<P> providers, T...
builtinValues) {
+ this.lookup = lookup;
+ this.providerType = providerType;
Review Comment:
⚠️ **4-arg constructor is unused in production.** The three concrete inner
subclasses (`DefaultPathScopeRegistry`, `DefaultProjectScopeRegistry`,
`DefaultLanguageRegistry`) all delegate to the 2-arg constructor
(`super(providers, builtinValues)`), which sets `lookup = null` and
`providerType = null`. The `if (lookup != null && providerType != null)` branch
in `lookup()` therefore never executes in production — only the test uses the
4-arg path.
Either wire the `Lookup` through DI for these subclasses (so the fix
actually takes effect), or remove the dead code path and document explicitly
that the reactor-leak fix is limited to `DefaultTypeRegistry.require()` only.
--
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]
