[
https://issues.apache.org/jira/browse/GROOVY-12344?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18110931#comment-18110931
]
ASF GitHub Bot commented on GROOVY-12344:
-----------------------------------------
testlens-app[bot] commented on PR #2868:
URL: https://github.com/apache/groovy/pull/2868#issuecomment-5519468773
## 🚨 TestLens detected 3 failed tests 🚨
Here is what you can do:
1) Inspect the test failures carefully.
2) If you are convinced that some of the tests are flaky, you can mute them
below.
3) Finally, trigger a rerun by checking the rerun checkbox.
### Test Summary
#### [Build and test / lts \(17, windows-latest,
1\)](https://github.com/apache/groovy/actions/runs/33706324746/job/100496088822?pr=2868)
> :groovy-groovysh:test
| Test | Runs | Flakiness |
|---|---|--:|
| GroovyPosixCommandsTest >
headAndGrepAndWcStripControlCharactersFromNames\(\) | ❌ | 23% 🔴 |
| GroovyPosixCommandsTest > lsStripsControlCharactersFromFileNames\(\) | ❌ |
23% 🔴 |
| GroovyPosixCommandsTest > lsStripsControlCharactersFromSymlinkTargets\(\)
| ❌ | 23% 🔴 |
🏷️ Commit: 38f658d078cb97ae4ef242cc104b2f5542125600
▶️ Tests: 84130 executed
🟡 Checks: 19/29 completed
### Test Failures
<details><summary><strong>GroovyPosixCommandsTest >
headAndGrepAndWcStripControlCharactersFromNames()</strong>
(:groovy-groovysh:test in <a
href="https://github.com/apache/groovy/actions/runs/33706324746/job/100496088822?pr=2868";>Build
and test / lts (17, windows-latest, 1)</a>)</summary>
```
java.nio.file.InvalidPathException: Illegal char <> at index 2:
ok[2Ktrap.txt
at
java.base/sun.nio.fs.WindowsPathParser.normalize(WindowsPathParser.java:182)
at
java.base/sun.nio.fs.WindowsPathParser.parse(WindowsPathParser.java:153)
at
java.base/sun.nio.fs.WindowsPathParser.parse(WindowsPathParser.java:77)
at java.base/sun.nio.fs.WindowsPath.parse(WindowsPath.java:92)
at
java.base/sun.nio.fs.WindowsFileSystem.getPath(WindowsFileSystem.java:232)
at java.base/java.nio.file.Path.resolve(Path.java:515)
at
java.base/jdk.internal.reflect.GeneratedMethodAccessor31.invoke(Unknown Source)
at
java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
at java.base/java.lang.reflect.Method.invoke(Method.java:569)
at
org.codehaus.groovy.reflection.CachedMethod.invoke(CachedMethod.java:391)
at groovy.lang.MetaMethod.doMethodInvoke(MetaMethod.java:298)
at
org.codehaus.groovy.vmplugin.v8.IndyInterface.invokeColdReflective(IndyInterface.java:648)
at
org.apache.groovy.groovysh.jline.GroovyPosixCommandsTest.headAndGrepAndWcStripControlCharactersFromNames(GroovyPosixCommandsTest.groovy:123)
```
</details>
<details><summary><strong>GroovyPosixCommandsTest >
lsStripsControlCharactersFromFileNames()</strong> (:groovy-groovysh:test in <a
href="https://github.com/apache/groovy/actions/runs/33706324746/job/100496088822?pr=2868";>Build
and test / lts (17, windows-latest, 1)</a>)</summary>
```
java.nio.file.InvalidPathException: Illegal char <> at index 2:
ok[2Ktrap.txt
at
java.base/sun.nio.fs.WindowsPathParser.normalize(WindowsPathParser.java:182)
at
java.base/sun.nio.fs.WindowsPathParser.parse(WindowsPathParser.java:153)
at
java.base/sun.nio.fs.WindowsPathParser.parse(WindowsPathParser.java:77)
at java.base/sun.nio.fs.WindowsPath.parse(WindowsPath.java:92)
at
java.base/sun.nio.fs.WindowsFileSystem.getPath(WindowsFileSystem.java:232)
at java.base/java.nio.file.Path.resolve(Path.java:515)
at
java.base/jdk.internal.reflect.GeneratedMethodAccessor31.invoke(Unknown Source)
at
java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
at java.base/java.lang.reflect.Method.invoke(Method.java:569)
at
org.codehaus.groovy.reflection.CachedMethod.invoke(CachedMethod.java:391)
at groovy.lang.MetaMethod.doMethodInvoke(MetaMethod.java:298)
at
org.codehaus.groovy.vmplugin.v8.IndyInterface.invokeColdReflective(IndyInterface.java:648)
at
org.apache.groovy.groovysh.jline.GroovyPosixCommandsTest.lsStripsControlCharactersFromFileNames(GroovyPosixCommandsTest.groovy:97)
```
</details>
<details><summary><strong>GroovyPosixCommandsTest >
lsStripsControlCharactersFromSymlinkTargets()</strong> (:groovy-groovysh:test
in <a
href="https://github.com/apache/groovy/actions/runs/33706324746/job/100496088822?pr=2868";>Build
and test / lts (17, windows-latest, 1)</a>)</summary>
```
java.nio.file.InvalidPathException: Illegal char <> at index 2:
ok[2Ktrap.txt
at
java.base/sun.nio.fs.WindowsPathParser.normalize(WindowsPathParser.java:182)
at
java.base/sun.nio.fs.WindowsPathParser.parse(WindowsPathParser.java:153)
at
java.base/sun.nio.fs.WindowsPathParser.parse(WindowsPathParser.java:77)
at java.base/sun.nio.fs.WindowsPath.parse(WindowsPath.java:92)
at
java.base/sun.nio.fs.WindowsFileSystem.getPath(WindowsFileSystem.java:232)
at java.base/java.nio.file.Path.resolve(Path.java:515)
at
java.base/jdk.internal.reflect.GeneratedMethodAccessor31.invoke(Unknown Source)
at
java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
at java.base/java.lang.reflect.Method.invoke(Method.java:569)
at
org.codehaus.groovy.reflection.CachedMethod.invoke(CachedMethod.java:391)
at groovy.lang.MetaMethod.doMethodInvoke(MetaMethod.java:298)
at
org.codehaus.groovy.vmplugin.v8.IndyInterface.invokeColdReflective(IndyInterface.java:648)
at
org.apache.groovy.groovysh.jline.GroovyPosixCommandsTest.lsStripsControlCharactersFromSymlinkTargets(GroovyPosixCommandsTest.groovy:108)
```
</details>
### Rerun Controls
> [!NOTE]
> Checks are currently running using the configuration below.
Select tests to mute in this pull request:
🔲 GroovyPosixCommandsTest >
headAndGrepAndWcStripControlCharactersFromNames\(\) <!
> Replace TemplateServlet backend cache class
> -------------------------------------------
>
> Key: GROOVY-12344
> URL: https://issues.apache.org/jira/browse/GROOVY-12344
> Project: Groovy
> Issue Type: Improvement
> Reporter: Paul King
> Assignee: Paul King
> Priority: Major
>
> {{TemplateServlet}} caches compiled templates in a {{WeakHashMap}} that is
> read and written from {{service()}} with no synchronization:
> {code:java}
> private final Map<String, TemplateCacheEntry> cache;
> ...
> this.cache = new WeakHashMap<String, TemplateCacheEntry>();
> {code}
> There are two independent defects here. The caching one is the more
> consequential.
> h3. 1. The cache almost never hits
> The key is {{file.getAbsolutePath()}} (or {{url.toString()}}), a {{String}}
> local to {{getTemplate}}. {{WeakHashMap}} holds its _keys_ weakly, so an
> entry survives only while something outside the map strongly references that
> key. Nothing does: {{TemplateCacheEntry}} stores only {{lastModified}},
> {{length}}, a {{Date}} and the {{Template}} — deliberately not the {{File}} —
> so the key becomes unreachable the moment the request returns.
> Every entry is therefore collectible as soon as it is created, and the cache
> empties at the next GC regardless of how hot a template is:
> {noformat}
> key identity reused across requests? false (equal: true)
> cache size right after request: 1
> cache size after GC: 0
> next request hits cache? false
> {noformat}
> Note when reproducing this: build the path at runtime. Deriving it from a
> source literal makes the entry appear to survive, because the literal is
> interned and permanently reachable from the constant pool. A servlet's path
> always comes from the request URI.
> h3. 2. Concurrent requests can corrupt the map
> {{HashMap}} received the JDK 8 rewrite that ended the well-known resize
> infinite-loop. {{WeakHashMap}} did not — as of JDK 17 its {{transfer()}}
> still head-inserts:
> {code:java}
> int i = indexFor(e.hash, dest.length);
> e.next = dest[i];
> dest[i] = e;
> {code}
> That is the list reversal which lets two concurrent resizers build a cycle,
> after which {{get()}} spins forever. The mechanism is present in the shipped
> JDK; a spin has not been forced under test, as it is timing dependent.
> {{TemplateCacheEntry.hit}} is likewise a non-atomic {{long}} incremented from
> concurrent requests, so hit counts in verbose mode can be wrong.
> h3. Proposed fix
> Use {{ConcurrentHashMap}}. Keep the plain {{get}}/{{put}} pair rather than
> {{computeIfAbsent}}: template compilation is slow and would hold a bin lock
> for its duration, and two threads racing to compile the same template is
> harmless and rare.
> Strong references mean templates are now retained for the servlet's lifetime,
> which is the point — the key space is bounded by templates that actually
> exist, since {{service()}} sends a 404 before {{getTemplate}} is reached in
> both the {{File}} branch ({{exists()}}/{{canRead()}}) and the {{URL}} branch
> ({{getResource(name) == null}}). {{getScriptUri}} is overridable, but an
> override still has to name a resolvable resource, so a subclass cannot make
> the key space unbounded.
> Staleness handling is unaffected: {{TemplateCacheEntry.validate()}} continues
> to check {{lastModified}} and {{length}}, so an edited template is still
> picked up.
> h3. Throughput
> Read throughput, 40 cached templates, JDK 17:
> ||Map||1 thread||4 threads||8 threads||
> |{{WeakHashMap}} (as shipped)|53.7 M gets/s|corrupts|corrupts|
> |{{synchronizedMap(WeakHashMap)}}|55.0 M gets/s|16.4 M gets/s|18.2 M gets/s|
> |{{ConcurrentHashMap}}|70.9 M gets/s|284.0 M gets/s|584.8 M gets/s|
> {{ConcurrentHashMap}} is ahead even single-threaded, because
> {{WeakHashMap.get}} polls a {{ReferenceQueue}} on every read and dereferences
> a {{WeakReference}} per probe. Wrapping the existing map in
> {{synchronizedMap}} would fix the corruption but leaves a cache that does not
> cache, and goes backwards under contention.
> h3. Compatibility
> No API change. {{cache}} is a {{private final}} field whose declared type
> stays {{Map}}, {{TemplateCacheEntry}} is a {{private static}} class, and the
> two helpers are {{private}} — only the constructor body changes, so the
> change is source and binary compatible and existing compiled subclasses link
> unchanged. The only operations on the map are {{get}} and {{put}}, so no
> iteration or view semantics are observable.
> {{ConcurrentHashMap}} rejects null keys and values, which cannot arise here:
> {{service()}} validates the file and the URL before {{getTemplate}} is
> called, and {{TemplateCacheEntry}}'s constructor throws on a null template.
> One residual difference: {{HttpServlet}} is {{Serializable}} and {{cache}} is
> not transient, while {{TemplateCacheEntry}} and {{Template}} are not
> serializable. Today the cache is usually empty, so serializing a
> {{TemplateServlet}} can succeed by chance; with a populated cache it would
> consistently throw {{NotSerializableException}}. Containers passivate
> sessions rather than servlet instances, so this is theoretical, and
> {{transient}} is not a free fix given the field is {{final}} and
> deserialization skips the constructor.
> The cache was introduced as a {{WeakHashMap}} in 2005 (GROOVY-814), in a
> revision still carrying {{// Java5}} comments beside the raw-typed field. No
> rationale was recorded.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
