[
https://issues.apache.org/jira/browse/GROOVY-12558?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18140471#comment-18140471
]
ASF GitHub Bot commented on GROOVY-12558:
-----------------------------------------
jdaugherty commented on PR #3086:
URL: https://github.com/apache/groovy/pull/3086#issuecomment-6716162143
@paulk-asert Thanks for the thorough probe. Pushed 08b2089eec:
1. Cold tier: `ColdReflectiveMethodHandleWrapper.tryBuild` now also keeps
`selector.metaClassSwitchPoint`, and `isValidFor` rejects the wrapper once it
is invalidated.
2. Tests: `IndyForeignMetaClassSwitchPointTest` now runs each scenario for a
subclass given the type's EMC (Spock `Mock()`), a subclass answering the type's
registry metaclass (GORM proxy), and a `DelegatingMetaClass` over the EMC as a
control. The scenarios are the method replaced twice, and the real method
called first, then replaced, each for a method call and for `+=`. The cold tier
runs in a fresh JVM with `-Dgroovy.indy.cold.reflection=true`
(`IndyForeignMetaClassColdTierProbe`, the same pattern as `IndyLoggingProbe`),
and `IndyScopedSwitchPointTest` has a unit test for the new `isValidFor` check.
Without the `isValidFor` check, only the cold-tier test fails
(`SHARED_METACLASS`: expected `replaced`, was `real`). On the PR's base, every
proxy-shape case fails and the `DelegatingMetaClass` controls pass.
3. Filled in the missing cells in the table above. I ran the GORM-proxy and
`DelegatingMetaClass` rows against the released 5.1.3 and 6.0.0 jars.
The `DelegatingMetaClass` control covers method calls only. While adding it
I found that a compound-assignment site does not guard on the receiver's
metaclass instance. `IndyCompoundAssign` guards on the receiver and argument
classes and the SwitchPoints, while a method call also gets `Selector`'s
metaclass-identity guard. Two instances of one Groovy class with different
metaclasses therefore share whichever `plus` was linked first:
```groovy
class Plain { def plus(int n) { 'real' } }
def addOne(v) { def r = v; r += 1; r }
def a1 = new Plain(); a1.metaClass.plus = { int n -> 'x' }
assert addOne(a1) == 'x'
assert addOne(new Plain()) == 'real' // 'x' on 6.0.0 and master, with or
without this PR; 'real' on 5.1.3
```
It is independent of this change, so I left it out of this PR; it should get
its own issue.
The 6.0.x backport branch
[`GROOVY-12558-6_0_X`](https://github.com/jdaugherty/groovy/tree/GROOVY-12558-6_0_X)
has the same commit (f03c0d2e1a). The indy, `ClassInfo` and
`ColdReflectionParityTest` suites pass on both branches.
> Indy: call site keeps a method replaced on a metaclass the receiver shares
> with another class
> ---------------------------------------------------------------------------------------------
>
> Key: GROOVY-12558
> URL: https://issues.apache.org/jira/browse/GROOVY-12558
> Project: Groovy
> Issue Type: Bug
> Affects Versions: 6.0.0
> Reporter: James Daugherty
> Priority: Major
>
> On Groovy 6 with invokedynamic, a call site whose receiver uses the metaclass
> of another class keeps invoking a method after that metaclass replaces it.
> Groovy 5, and Groovy 6 with classic call sites, invoke the replacement. This
> is a regression from GROOVY-12191 (6.0.0-beta-1).
> Such receivers are common in tests: Spock's {{Mock()}} generates a subclass
> of the mocked type whose {{getMetaClass()}} answers the mocked type's
> metaclass, so a feature that replaces a method on the mock's metaclass
> changes the {{ExpandoMetaClass}} of the mocked type, and many proxy libraries
> behave the same way. A specification that replaces such a method in one
> feature and runs the same code under test in a later feature gets the earlier
> feature's closure: a {{0 * collaborator.update(*_)}} interaction fails with
> "Too many invocations", or an expected exception is never thrown.
> *Reproducer:*
> https://github.com/jdaugherty/groovy-indy-shared-metaclass-switchpoint-reproducer
> ({{./run.sh 5.1.3 6.0.0}}). {{GreeterProxy extends Greeter}} and is given
> the {{ExpandoMetaClass}} of {{Greeter}}, as a mock or proxy subclass is. One
> call site invokes {{greet()}} and one {{+=}} site invokes {{plus()}}; each
> method is replaced on the metaclass between two calls of the same site:
> {noformat}
> == Groovy 5.1.3
> indy=true: method call after replacing the method: second (OK)
> indy=true: compound assignment after replacing plus: second (OK)
> == Groovy 6.0.0
> indy=true: method call after replacing the method: first (STALE: the
> call site still invokes the replaced method)
> indy=true: compound assignment after replacing plus: first (STALE: the
> call site still invokes the replaced method)
> indy=false: method call after replacing the method: second (OK)
> indy=false: compound assignment after replacing plus: second (OK)
> {noformat}
> *Cause:* GROOVY-12191 replaced the global indy SwitchPoint with per-class
> domains. A call site is guarded by the SwitchPoint of the receiver's class
> ({{Selector.MethodSelector.captureTokenAndGetMetaClass()}},
> {{IndyInvalidation.classSwitchPointFor(args[0])}}; likewise in
> {{IndyCompoundAssign.resolve}}). A change to a metaclass retires the domain
> of the class the metaclass belongs to, {{MetaClass.getTheClass()}}
> ({{IndyInvalidation.switchPointForMetaClass}} / {{invalidateMetaClass}}).
> When the receiver's class is not the metaclass's class, as for
> {{GreeterProxy}} and the {{Greeter}} metaclass it uses, the change retires
> {{Greeter}}'s domain while the site waits on {{GreeterProxy}}'s. The receiver
> metaclass guard ({{SAME_MC}}) does not catch it either, because replacing an
> {{ExpandoMetaClass}} method keeps the same metaclass instance.
> *Proposed fix:* when the selected metaclass belongs to a class other than the
> receiver's, also capture that class's SwitchPoint before selection and guard
> the site with both. That applies to {{Selector}} and to
> {{IndyCompoundAssign}}, which captures its token the same way. A patch with
> tests follows as a pull request against master, with a backport for the 6.0.x
> line.
--
This message was sent by Atlassian Jira
(v10.3.26#10030026)
