Daniel Sun created GROOVY-12725:
-----------------------------------
Summary: 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
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)
