[
https://issues.apache.org/jira/browse/GROOVY-12725?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18142622#comment-18142622
]
ASF GitHub Bot commented on GROOVY-12725:
-----------------------------------------
daniellansun commented on code in PR #3260:
URL: https://github.com/apache/groovy/pull/3260#discussion_r4531493441
##########
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:
The `new HashSet(2)` line is the previous code. On the current tip the
constructor is `new HashSet<>(existing == null ? 1 : existing.size() + 1)`, so
a null `existing` really does allocate capacity 1. I agree one versus two is
not important here; I changed it only because you asked why 2 was chosen, and 1
matches the single element that is added.
##########
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:
You are right on both counts. `DefaultImportPackagesTest` (and the `add`
throws checks in `Groovy12725`) are API / regression tests: they lock the
iteration order and the fact that a caller cannot `add` to a returned set. They
do not establish concurrent behaviour, and I should not have pointed at them as
if they did.
The concurrency design is not the wrapper itself. It is that an update
replaces the `ConcurrentHashMap` entry instead of mutating the set a reader may
already be iterating. `compute` in `ResolveVisitor.resolveOneDefaultImport`
calls `including()`, which returns either the same instance or a new set; it
never adds to a set that has already been published. The map provides the safe
publication of that reference. `unmodifiableSet` does not invent that race
freedom, and it is also not the same as an immutable set: if the backing
`LinkedHashSet` were still written after the wrap, the view would change and
you could have a publication problem.
What the wrapper is for: the same set instance is shared. Until a later hit
replaces a cache entry, the cache holds the startup index sets (`assertSame` in
`Groovy12725`). A caller who could mutate a cache value would also mutate that
snapshot. The wrapper is a defensive contract against that, and the `add` tests
are there to catch a later change that starts returning a mutable set or
updating in place. They are not a thread-safety proof.
I used `Collections.unmodifiableSet` rather than `Set.copyOf` / `Set.of`
because those do not keep the `LinkedHashSet` insertion order, which is the
precedence order. The mutable set is a local in `ordered()`; it is not retained
or written after the wrap, so the published view does not change. Empty and
single-element results are already `Collections.emptySet()` and
`Collections.singleton`.
> 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)
