[
https://issues.apache.org/jira/browse/GROOVY-12725?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18141996#comment-18141996
]
ASF GitHub Bot commented on GROOVY-12725:
-----------------------------------------
daniellansun commented on code in PR #3260:
URL: https://github.com/apache/groovy/pull/3260#discussion_r4525497465
##########
src/main/java/org/apache/groovy/vmplugin/internal/DefaultImportPackages.java:
##########
@@ -0,0 +1,127 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.groovy.vmplugin.internal;
+
+import groovy.transform.Internal;
+
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.Collections;
+import java.util.HashSet;
+import java.util.LinkedHashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+
+/**
+ * Orders default-import package prefixes by a caller-supplied precedence
+ * array. Iteration order of the returned sets is that array's order.
+ * Packages that are not in the array are dropped, so a {@code HashSet}
+ * walk cannot put a later package ahead of an earlier one.
+ *
+ * @since 7.0.0
+ */
+@Internal
+public final class DefaultImportPackages {
+
+ private DefaultImportPackages() {
+ }
+
+ /**
+ * Returns the members of {@code packages} that occur in {@code
precedence},
+ * iterated in {@code precedence} order. The result is unmodifiable.
+ * An empty result is {@link Collections#emptySet()}. A single match is
+ * {@link Collections#singleton(Object)}.
+ *
+ * @param packages packages discovered for one simple name; {@code null}
is empty
+ * @param precedence package prefixes in the order they must be tried;
{@code null} yields an empty set
+ * @return an immutable set in precedence order
+ */
+ public static Set<String> ordered(final Collection<String> packages, final
String[] precedence) {
+ if (packages == null || packages.isEmpty() || precedence == null ||
precedence.length == 0) {
+ return Collections.emptySet();
+ }
+ List<String> matches = new ArrayList<>(Math.min(packages.size(),
precedence.length));
+ for (String candidate : precedence) {
+ if (packages.contains(candidate)) {
+ matches.add(candidate);
+ }
+ }
+ if (matches.isEmpty()) {
+ return Collections.emptySet();
+ }
+ if (matches.size() == 1) {
+ return Collections.singleton(matches.get(0));
+ }
+ return Collections.unmodifiableSet(new LinkedHashSet<>(matches));
Review Comment:
`ordered()` now inserts into a `LinkedHashSet` while it walks `precedence`.
Match order is insertion order, and there is no `ArrayList` to copy afterwards.
The unmodifiable wrapper stays. `Collections.unmodifiableSet` does not copy;
it only wraps that set. Callers publish the result: `ResolveVisitor` stores it
in a `ConcurrentHashMap`, and until a later hit replaces the entry the same
instance is the startup index (`Groovy12725` checks that with `assertSame`). A
reader on another thread must not observe a set being edited, and the tests
expect `add` to throw. An update replaces the map entry. An empty result is
still `Collections.emptySet()`, and a single match is still
`Collections.singleton`, which is already immutable.
Package precedence and the replace-rather-than-mutate cache behaviour are
unchanged.
##########
src/main/java/org/apache/groovy/vmplugin/internal/DefaultImportPackages.java:
##########
@@ -0,0 +1,127 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.groovy.vmplugin.internal;
+
+import groovy.transform.Internal;
+
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.Collections;
+import java.util.HashSet;
+import java.util.LinkedHashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+
+/**
+ * Orders default-import package prefixes by a caller-supplied precedence
+ * array. Iteration order of the returned sets is that array's order.
+ * Packages that are not in the array are dropped, so a {@code HashSet}
+ * walk cannot put a later package ahead of an earlier one.
+ *
+ * @since 7.0.0
+ */
+@Internal
+public final class DefaultImportPackages {
+
+ private DefaultImportPackages() {
+ }
+
+ /**
+ * Returns the members of {@code packages} that occur in {@code
precedence},
+ * iterated in {@code precedence} order. The result is unmodifiable.
+ * An empty result is {@link Collections#emptySet()}. A single match is
+ * {@link Collections#singleton(Object)}.
+ *
+ * @param packages packages discovered for one simple name; {@code null}
is empty
+ * @param precedence package prefixes in the order they must be tried;
{@code null} yields an empty set
+ * @return an immutable set in precedence order
+ */
+ public static Set<String> ordered(final Collection<String> packages, final
String[] precedence) {
+ if (packages == null || packages.isEmpty() || precedence == null ||
precedence.length == 0) {
+ return Collections.emptySet();
+ }
+ List<String> matches = new ArrayList<>(Math.min(packages.size(),
precedence.length));
+ for (String candidate : precedence) {
+ if (packages.contains(candidate)) {
+ matches.add(candidate);
+ }
+ }
+ if (matches.isEmpty()) {
+ return Collections.emptySet();
+ }
+ if (matches.size() == 1) {
+ return Collections.singleton(matches.get(0));
+ }
+ return Collections.unmodifiableSet(new LinkedHashSet<>(matches));
+ }
+
+ /**
+ * Returns {@code existing} when it already contains {@code packagePrefix}.
+ * Otherwise returns the precedence-ordered union. {@code packagePrefix}
+ * values absent from {@code precedence} are not added.
+ *
+ * @param existing the packages already recorded, or {@code null}
+ * @param packagePrefix the package to add
+ * @param precedence package prefixes in try-order
+ * @return an immutable set in precedence order, or {@code existing} when
unchanged
+ */
+ public static Set<String> including(final Set<String> existing, final
String packagePrefix, final String[] precedence) {
+ if (existing != null && existing.contains(packagePrefix)) {
+ return existing;
+ }
+ Set<String> combined = new HashSet<>(existing == null ? 2 :
existing.size() + 1);
Review Comment:
`including()` with a null `existing` only adds `packagePrefix`, so the
temporary `HashSet` is created with capacity 1.
> 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)
