[
https://issues.apache.org/jira/browse/GROOVY-12725?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18140229#comment-18140229
]
ASF GitHub Bot commented on GROOVY-12725:
-----------------------------------------
daniellansun opened a new pull request, #3260:
URL: https://github.com/apache/groovy/pull/3260
https://issues.apache.org/jira/browse/GROOVY-12725
> Preserve default-import package precedence and thread-safety in ResolveVisitor
> ------------------------------------------------------------------------------
>
> Key: GROOVY-12725
> URL: https://issues.apache.org/jira/browse/GROOVY-12725
> Project: Groovy
> Issue Type: Bug
> Reporter: Daniel Sun
> Assignee: Daniel Sun
> Priority: Major
>
> h2. Overview
> {{ResolveVisitor}} maintains a process-wide cache
> ({{DEFAULT_IMPORT_CLASS_AND_PACKAGES_CACHE}}) mapping simple class names to
> the set of default-imported package prefixes ({{java.lang.}}, {{java.io.}},
> {{java.net.}}, {{java.util.}}, {{groovy.lang.}}, {{groovy.util.}}) that
> contain a class with that simple name. Three issues in how this cache is
> initialized and updated can cause package precedence inversion, lost package
> mappings for colliding class names, or data races during concurrent
> compilation.
> h2. Problem Description
> h3. 1. Overwritten package mappings in {{Java9.getDefaultImportClasses}}
> {{Java9.getDefaultImportClasses(String[] packageNames)}} previously populated
> its result map via {{result.putAll(doFindClasses(...))}} across
> {{groovy-core}} locations and {{jrt:/modules/java.base/}}. When the same
> simple class name exists in both a Groovy default-imported package and a Java
> default-imported package (for example, {{Proxy}} in both {{java.net.Proxy}}
> and {{groovy.util.Proxy}}), the later {{putAll}} call overwrote the earlier
> entry instead of merging both package prefixes. In addition, the stored value
> sets were mutable, unordered {{HashSet}} instances rather than unmodifiable
> sets ordered by {{ResolveVisitor.DEFAULT_IMPORTS}}.
> {code:java}
> URI gcjLocation =
> DefaultGroovyMethods.getLocation(gcl.loadClass("groovy.lang.GroovySystem")).toURI();
> result.putAll(doFindClasses(gcjLocation, "groovy", groovyPackages));
> ...
> result.putAll(doFindClasses(URI.create("jrt:/modules/java.base/"), "java",
> javaPackages));
> {code}
> h3. 2. Non-atomic {{Map}} default methods in {{UnlimitedConcurrentCache}}
> {{UnlimitedConcurrentCache}} wraps a {{ConcurrentHashMap}} and implements
> {{EvictableCache}} (which extends {{java.util.Map}}), but did not override
> the atomic {{Map}} methods introduced in Java 8 ({{compute}},
> {{computeIfAbsent}}, {{computeIfPresent}}, {{merge}}, {{putIfAbsent}},
> {{remove(Object, Object)}}, and {{replace}}). Consequently, calling
> {{DEFAULT_IMPORT_CLASS_AND_PACKAGES_CACHE.computeIfAbsent(...)}} invoked the
> non-atomic {{Map}} default method rather than
> {{ConcurrentHashMap.computeIfAbsent}}.
> h3. 3. In-place {{HashSet}} mutation and lost {{DEFAULT_IMPORTS}} ordering in
> {{ResolveVisitor.resolveFromDefaultImports}}
> On a cache miss during {{resolveFromDefaultImports}}, {{ResolveVisitor}}
> obtained a mutable {{HashSet}} from
> {{DEFAULT_IMPORT_CLASS_AND_PACKAGES_CACHE}} and mutated it in place:
> {code:java}
> Set<String> packagePrefixSet =
> DEFAULT_IMPORT_CLASS_AND_PACKAGES_CACHE.computeIfAbsent(typeName, key -> new
> HashSet<>(2));
> packagePrefixSet.add(packagePrefix);
> {code}
> * Concurrent compiler threads iterating over the cached {{Set<String>}} could
> race with in-place {{HashSet.add}} mutations.
> * Because {{HashSet}} iteration order is arbitrary and does not follow
> {{ResolveVisitor.DEFAULT_IMPORTS}}, discovering a class in a lower-precedence
> package (e.g. {{groovy.util.}}) before a higher-precedence package (e.g.
> {{groovy.lang.}}) could cause subsequent lookups to resolve the
> lower-precedence class even when both are available on the classpath.
> h2. Solution
> * Merge package sets across all scanned locations in
> {{Java9.getDefaultImportClasses}} instead of overwriting entries with
> {{Map.putAll}}, and store unmodifiable sets ordered according to
> {{packageNames}} ({{ResolveVisitor.DEFAULT_IMPORTS}}).
> * Delegate atomic {{Map}} operations ({{compute}}, {{computeIfAbsent}},
> {{computeIfPresent}}, {{merge}}, {{putIfAbsent}}, {{remove(Object, Object)}},
> {{replace}}, {{replaceAll}}, {{getOrDefault}}, and {{forEach}}) in
> {{UnlimitedConcurrentCache}} directly to the underlying {{ConcurrentHashMap}}.
> * Update {{ResolveVisitor.DEFAULT_IMPORT_CLASS_AND_PACKAGES_CACHE}}
> atomically via {{compute(...)}} using unmodifiable sets ordered according to
> {{DEFAULT_IMPORTS}}, preventing concurrent mutation races and preserving
> default-import package precedence.
--
This message was sent by Atlassian Jira
(v10.3.26#10030026)
