[
https://issues.apache.org/jira/browse/GROOVY-12744?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18140814#comment-18140814
]
ASF GitHub Bot commented on GROOVY-12744:
-----------------------------------------
codeconsole opened a new pull request, #3279:
URL: https://github.com/apache/groovy/pull/3279
https://issues.apache.org/jira/browse/GROOVY-12744
## Problem
Node metadata lives in a `ListHashMap` wrapped in
`Collections.synchronizedMap` (GROOVY-11966). The wrapper creates its
`entrySet()`, `keySet()` and `values()` views on first use and keeps returning
them. With up to three entries, `ListHashMap` returned those views from a copy
of its arrays, not from the map itself. So after the first `getNodeMetaData()`
or `copyNodeMetaData(node)`, every later read or copy of that node returned the
old entries. Entries added since were missing, removed ones were still there,
and replaced values kept their old value.
In the core test suite this happens during static compilation. Copies of
map-style `ConstructorCallExpression`s and of a `PropertyExpression` took a
`DIRECT_METHOD_CALL_TARGET` from an earlier type-checking pass. In every case
found, the old and new targets have the same signature, so no wrong bytecode
was found.
## Fix
`ListHashMap.entrySet()`, `keySet()` and `values()` now return views backed
by the map, as the `Map` contract requires: `size()`, `contains` and each new
iterator see the map's current entries. Each iterator still works on a copy of
the entries while the map keeps them in its arrays (up to three by default),
and on a read-only view of the backing map otherwise, as before. The views
still can't change the map.
## Tests
- `ListHashMapTest`: the views and a `synchronizedMap` wrapper see later
puts, replacements and removals, across both storage switches.
- `ASTNodeTest`: `getNodeMetaData()` and `copyNodeMetaData` see entries
added, replaced and removed after an earlier snapshot.
Both fail without the fix. The full core `:test` suite,
`NodeMetaDataHandlerStressTest` and the `groovy-ginq`, `groovy-contracts` and
`groovy-macro` tests pass. With a temporary check that compared each copy
against the live map, the 34 stale copies seen in
`DefaultGroovyMethodsSTCTest`, `StaticCompileDGMTest`, `GenericsSTCTest` and
`GenericsStaticCompileTest` drop to none.
> Node metadata snapshots and copies miss changes made after the first one
> ------------------------------------------------------------------------
>
> Key: GROOVY-12744
> URL: https://issues.apache.org/jira/browse/GROOVY-12744
> Project: Groovy
> Issue Type: Bug
> Components: Compiler
> Affects Versions: 6.0.0
> Reporter: Scott Murphy Heiberg
> Priority: Major
>
> Since GROOVY-11966, {{NodeMetaDataHandler.newMetaDataMap()}} wraps a
> {{ListHashMap}} in {{Collections.synchronizedMap}}. The wrapper creates its
> {{entrySet()}} view on the first call and returns that same view afterwards.
> {{ListHashMap.entrySet()}} does not return a view backed by the map, as the
> {{Map}} contract requires: with up to three entries it returns the entries of
> a copy. So once a node's metadata has been read through {{getNodeMetaData()}}
> or copied with {{copyNodeMetaData}}, every later read or copy of that node
> returns the same old entries. Entries added since are missing, removed
> entries are still there, and replaced values keep their old value.
> {{keySet()}} and {{values()}} go stale in the same way.
> {code:groovy}
> import org.codehaus.groovy.ast.expr.ConstantExpression
> def a = new ConstantExpression(1)
> a.putNodeMetaData('k1', 1)
> println a.nodeMetaData // [k1:1]
> a.putNodeMetaData('k2', 2)
> println a.nodeMetaData // [k1:1], expected [k1:1, k2:2]
> def c = new ConstantExpression(3)
> c.copyNodeMetaData(a)
> println c.nodeMetaData // [k1:1], expected [k1:1, k2:2]
> a.removeNodeMetaData('k1')
> println a.nodeMetaData // [k1:1], expected [k2:2]
> {code}
> {{getNodeMetaData(key)}} is not affected, because it reads the map directly.
> Compilations run into this. In the core test suite, static compilation
> transforms of {{ConstructorCallExpression}} (map-style constructor calls) and
> {{PropertyExpression}} copy a stale {{DIRECT_METHOD_CALL_TARGET}}. In every
> case observed, the old and new values have the same signature, so no wrong
> bytecode was found. Any code that reads all of a node's metadata or copies it
> (AST transforms, tooling) can get old data.
> On 5.0.x and 4.0.x, {{getNodeMetaData()}} and {{copyNodeMetaData()}} read the
> {{ListHashMap}} directly, so each call sees the current entries. The
> {{ListHashMap}} views break the {{Map}} contract there too, though: if code
> keeps the map returned by {{getNodeMetaData()}} and reads it after the node
> changes, it gets the old entries.
--
This message was sent by Atlassian Jira
(v10.3.26#10030026)
