[
https://issues.apache.org/jira/browse/GROOVY-12725?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18142608#comment-18142608
]
ASF GitHub Bot commented on GROOVY-12725:
-----------------------------------------
blackdrag commented on code in PR #3260:
URL: https://github.com/apache/groovy/pull/3260#discussion_r4531102938
##########
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:
> 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.
I assume you mean
src/test/groovy/org/apache/groovy/vmplugin/internal/DefaultImportPackagesTest.groovy.
This test does not establish multi threading behaviour. It tests an API and
can maybe be used as regression test to ensure the ordering logic is kept. But
it does not explain why this is needed. What is the goal? To catch the mistake
during development? It most likely is not to enable multithreading behaviour.
Also unmodifiable is not the same as immutable. A unmodifiableSet may still
have publication issues based on the underlying collection. Again... I am not
saying this is wrong. Just want a more clear picture why here exactly like this
and not different. Just pointing to the test with the argument of thread safety
and then having a test that does not test that if not convincing.
> 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)
