[
https://issues.apache.org/jira/browse/GROOVY-12344?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18110933#comment-18110933
]
ASF GitHub Bot commented on GROOVY-12344:
-----------------------------------------
codecov-commenter commented on PR #2868:
URL: https://github.com/apache/groovy/pull/2868#issuecomment-5519523923
##
[Codecov](https://app.codecov.io/gh/apache/groovy/pull/2868?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 70.9426%. Comparing base
([`2ca5668`](https://app.codecov.io/gh/apache/groovy/commit/2ca56681c42e8b0599a878de848098df2155d25e?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache))
to head
([`38f658d`](https://app.codecov.io/gh/apache/groovy/commit/38f658d078cb97ae4ef242cc104b2f5542125600?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 2 commits behind head on master.
<details><summary>Additional details and impacted files</summary>
[](https://app.codecov.io/gh/apache/groovy/pull/2868?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
```diff
@@ Coverage Diff @@
## master #2868 +/- ##
==================================================
+ Coverage 70.9418% 70.9426% +0.0007%
- Complexity 37054 37055 +1
==================================================
Files 1576 1576
Lines 134857 134857
Branches 24973 24973
==================================================
+ Hits 95670 95671 +1
+ Misses 30530 30526 -4
- Partials 8657 8660 +3
```
| [Files with missing
lines](https://app.codecov.io/gh/apache/groovy/pull/2868?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
| Coverage Δ | |
|---|---|---|
|
[.../src/main/java/groovy/servlet/TemplateServlet.java](https://app.codecov.io/gh/apache/groovy/pull/2868?src=pr&el=tree&filepath=subprojects%2Fgroovy-servlet%2Fsrc%2Fmain%2Fjava%2Fgroovy%2Fservlet%2FTemplateServlet.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VicHJvamVjdHMvZ3Jvb3Z5LXNlcnZsZXQvc3JjL21haW4vamF2YS9ncm9vdnkvc2VydmxldC9UZW1wbGF0ZVNlcnZsZXQuamF2YQ==)
| `42.9578% <100.0000%> (+3.5211%)` | :arrow_up: |
... and [4 files with indirect coverage
changes](https://app.codecov.io/gh/apache/groovy/pull/2868/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>
> 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)
