[
https://issues.apache.org/jira/browse/GROOVY-12558?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18140039#comment-18140039
]
ASF GitHub Bot commented on GROOVY-12558:
-----------------------------------------
paulk-asert commented on PR #3086:
URL: https://github.com/apache/groovy/pull/3086#issuecomment-6700400031
Thanks @jdaugherty, the explanation of how this differs from GROOVY-12046
and the framework examples settle the "why" for me. I probed the PR (rebased
onto current master) against master. I ran four receiver shapes: a subclass
given the type's EMC (Spock `Mock()`), a subclass answering the registry
metaclass (GORM proxy), a `DelegatingMetaClass` over the EMC, and a plain
instance. Each ran six operations: a method replaced twice, the real method
first then replaced, getter, setter, a property added later, and `+=`. Every
case ran with the cold reflective tier off and on.
**Results**
- The PR fixes all the stale method and `+=` cases for both proxy shapes,
including the "real method first, then replaced" case. The new test doesn't
cover that one, because it installs the replacement before the first call.
- Getters, setters and properties added later were already correct on
master. The `DelegatingMetaClass` and plain controls are correct before and
after.
- For ordinary receivers, `foreignMetaClassSwitchPointFor` returns `null`,
so they get no extra guard. I checked null, `Class`, closures,
`String`/`GString`, collections, arrays, lambdas, scripts, Groovy objects, `as`
proxies, `withTraits`, per-instance EMCs, enums and `BigDecimal`. The extra
guard is confined to the proxy shapes, which makes the change look safe to
backport to 6.0.x.
**One gap: the cold reflective tier**
With `-Dgroovy.indy.cold.reflection=true` (also the default for AOT-linked
sites), "call the real method, then replace it on the EMC" stays stale with the
PR, for both proxy shapes. `ColdReflectiveMethodHandleWrapper.tryBuild` keeps
only `selector.mopSwitchPoint`, so `isValidFor` never sees the new
`metaClassSwitchPoint`. The wrapper should carry and check that switch point
too.
**Suggested additions**
1. The cold-tier fix above.
2. Tests for:
- the real method called first, then replaced, with the cold tier off and
on (the on case needs a forked JVM or a reset of the flag, since it's read once
at class init);
- the registry-answering proxy (the GORM shape) alongside the shared-EMC
one;
- `DelegatingMetaClass` as an unaffected control, as you offered.
3. A small one: in your table comment, the GORM-proxy and
`DelegatingMetaClass` rows are missing cells.
The 6.0.x backport commit is the same patch, and the three files it touches
are identical on both branches, so the cold-tier fix should carry over
unchanged.
On @blackdrag's design point: I agree a metaclass standing in for another
class isn't what `MetaClassImpl` was designed for. But Groovy 5 and classic
call sites both handle these receivers, and the cost here is a second
`SwitchPoint`, only on the shapes that need it. I looked at the alternatives:
- guarding with only the metaclass owner's switch point would be unsafe for
per-instance metaclass changes;
- making these sites uncacheable would slow GORM proxies in production;
- a hierarchy fan-out is what GROOVY-12191 removed.
So this approach looks like the right one to me.
> 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)
