[
https://issues.apache.org/jira/browse/GROOVY-12725?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18140233#comment-18140233
]
ASF GitHub Bot commented on GROOVY-12725:
-----------------------------------------
codecov-commenter commented on PR #3260:
URL: https://github.com/apache/groovy/pull/3260#issuecomment-6706512672
##
[Codecov](https://app.codecov.io/gh/apache/groovy/pull/3260?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
Report
:x: Patch coverage is `78.04878%` with `9 lines` in your changes missing
coverage. Please review.
:white_check_mark: Project coverage is 74.4018%. Comparing base
([`d020012`](https://app.codecov.io/gh/apache/groovy/commit/d02001286af335c8777fb9f5770e48ec4d07119f?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache))
to head
([`bae9327`](https://app.codecov.io/gh/apache/groovy/commit/bae9327b3f1288595ad533f7b9248b4b6199608c?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
| [Files with missing
lines](https://app.codecov.io/gh/apache/groovy/pull/3260?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
| Patch % | Lines |
|---|---|---|
|
[...oovy/runtime/memoize/UnlimitedConcurrentCache.java](https://app.codecov.io/gh/apache/groovy/pull/3260?src=pr&el=tree&filepath=src%2Fmain%2Fjava%2Forg%2Fcodehaus%2Fgroovy%2Fruntime%2Fmemoize%2FUnlimitedConcurrentCache.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3JjL21haW4vamF2YS9vcmcvY29kZWhhdXMvZ3Jvb3Z5L3J1bnRpbWUvbWVtb2l6ZS9VbmxpbWl0ZWRDb25jdXJyZW50Q2FjaGUuamF2YQ==)
| 61.5385% | [5 Missing :warning:
](https://app.codecov.io/gh/apache/groovy/pull/3260?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
|
|
[...va/org/codehaus/groovy/control/ResolveVisitor.java](https://app.codecov.io/gh/apache/groovy/pull/3260?src=pr&el=tree&filepath=src%2Fmain%2Fjava%2Forg%2Fcodehaus%2Fgroovy%2Fcontrol%2FResolveVisitor.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3JjL21haW4vamF2YS9vcmcvY29kZWhhdXMvZ3Jvb3Z5L2NvbnRyb2wvUmVzb2x2ZVZpc2l0b3IuamF2YQ==)
| 70.0000% | [1 Missing and 2 partials :warning:
](https://app.codecov.io/gh/apache/groovy/pull/3260?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
|
|
[...in/java/org/codehaus/groovy/vmplugin/v9/Java9.java](https://app.codecov.io/gh/apache/groovy/pull/3260?src=pr&el=tree&filepath=src%2Fmain%2Fjava%2Forg%2Fcodehaus%2Fgroovy%2Fvmplugin%2Fv9%2FJava9.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3JjL21haW4vamF2YS9vcmcvY29kZWhhdXMvZ3Jvb3Z5L3ZtcGx1Z2luL3Y5L0phdmE5LmphdmE=)
| 94.4444% | [1 Missing :warning:
](https://app.codecov.io/gh/apache/groovy/pull/3260?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
|
<details><summary>Additional details and impacted files</summary>
[](https://app.codecov.io/gh/apache/groovy/pull/3260?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
```diff
@@ Coverage Diff @@
## master #3260 +/- ##
==================================================
+ Coverage 74.4010% 74.4018% +0.0008%
- Complexity 44345 44365 +20
==================================================
Files 1699 1699
Lines 150998 151034 +36
Branches 28770 28775 +5
==================================================
+ Hits 112344 112372 +28
- Misses 28380 28385 +5
- Partials 10274 10277 +3
```
| [Files with missing
lines](https://app.codecov.io/gh/apache/groovy/pull/3260?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
| Coverage Δ | |
|---|---|---|
|
[...in/java/org/codehaus/groovy/vmplugin/v9/Java9.java](https://app.codecov.io/gh/apache/groovy/pull/3260?src=pr&el=tree&filepath=src%2Fmain%2Fjava%2Forg%2Fcodehaus%2Fgroovy%2Fvmplugin%2Fv9%2FJava9.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3JjL21haW4vamF2YS9vcmcvY29kZWhhdXMvZ3Jvb3Z5L3ZtcGx1Z2luL3Y5L0phdmE5LmphdmE=)
| `63.8710% <94.4444%> (+3.8710%)` | :arrow_up: |
|
[...va/org/codehaus/groovy/control/ResolveVisitor.java](https://app.codecov.io/gh/apache/groovy/pull/3260?src=pr&el=tree&filepath=src%2Fmain%2Fjava%2Forg%2Fcodehaus%2Fgroovy%2Fcontrol%2FResolveVisitor.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3JjL21haW4vamF2YS9vcmcvY29kZWhhdXMvZ3Jvb3Z5L2NvbnRyb2wvUmVzb2x2ZVZpc2l0b3IuamF2YQ==)
| `90.9604% <70.0000%> (-0.1456%)` | :arrow_down: |
|
[...oovy/runtime/memoize/UnlimitedConcurrentCache.java](https://app.codecov.io/gh/apache/groovy/pull/3260?src=pr&el=tree&filepath=src%2Fmain%2Fjava%2Forg%2Fcodehaus%2Fgroovy%2Fruntime%2Fmemoize%2FUnlimitedConcurrentCache.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3JjL21haW4vamF2YS9vcmcvY29kZWhhdXMvZ3Jvb3Z5L3J1bnRpbWUvbWVtb2l6ZS9VbmxpbWl0ZWRDb25jdXJyZW50Q2FjaGUuamF2YQ==)
| `76.4706% <61.5385%> (-2.4768%)` | :arrow_down: |
... and [9 files with indirect coverage
changes](https://app.codecov.io/gh/apache/groovy/pull/3260/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
</details>
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test
Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests,
report on failures, and find test suite problems.
- :package: [JS Bundle
Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save
yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
> 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)
