[
https://issues.apache.org/jira/browse/GROOVY-12344?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Paul King updated GROOVY-12344:
-------------------------------
Description:
{{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.
> 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
> 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)
