[
https://issues.apache.org/jira/browse/GROOVY-12557?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18135395#comment-18135395
]
ASF GitHub Bot commented on GROOVY-12557:
-----------------------------------------
paulk-asert commented on PR #3085:
URL: https://github.com/apache/groovy/pull/3085#issuecomment-6532734438
Verdict: ready to merge. I found no correctness or metaprogramming blocker.
James has already answered Daniel's asType question in the PR thread: a cast
site never selects asType, while the as operator goes through an ordinary
method call site whose metaclass guard the PR leaves alone. The PR has
blackdrag's approval and green CI.
To test it I built the PR head and its parent and ran probes against both.
The jars and probe scripts are in
/private/tmp/claude-501/-Users-paulk-Projects-groovy/deaa34fd-22fc-4390-b220-52371f39f6b5/scratchpad/p3085/;
I've removed the scratch worktree.
Other ways it could break, and what I found:
1. Is anything else in the cast metaclass-dependent? No. Besides asBoolean,
a cast links one of four handles:
- identity;
- the HashSet/ArrayList constructors;
- closure-to-SAM conversion;
- castToType.
The first three depend only on the value's class. castToType runs fresh
on every call, so any metaclass lookup inside it (such as a constructor through
invokeConstructorOf) never gets baked into the cached handle. The Boolean and
boolean targets are caught by handleBoolean first, which keeps the guard.
2. The metaclass guard was also quietly a null guard. isSameMetaClass(mc,
null) returns false, so a null value forced a relink. With the PR, a cast from
a final static type gets only the switch-point guard; I confirmed in the indy
log that nothing else is added. A null-unsafe handle cached for a non-null
value could therefore be reused for null. In practice that can't happen:
- Identity, SAM and castToType handle null fine.
- The collection-constructor fast path is effectively unreachable,
because isAbstractClassOf(HashSet, Set) tests the subtype relationship the
wrong way round. Those casts fall through to castToType instead. That's an old
oddity, not something this PR introduced.
I warmed a final class Bag extends ArrayList → Set cast past the optimize
threshold, then passed null. It returned null on both builds, and the final
Number → int case still throws GroovyCastException on both.
3. Per-instance asBoolean still relinks. The flag is only set when
handleBoolean selects through the metaclass, and a new selector is created on
every relink, so stale state can't carry over. The PR's alternating
truthy/falsy test covers this.
4. Proxy truthiness is still not fixed, but it matches classic call sites. I
used a Groovy subclass whose getMetaClass() throws, standing in for a scoped
proxy:
| Shape | Before (indy) | PR (indy) | PR
(classic) |
|----------------------------------|---------------|-----------|--------------|
| Report r = p, (Report) p | throws | works | works
|
| boolean b = p, if (p), p ? :, !p | throws | throws | throws
|
| p == p | works | works | works
|
It's worth one sentence on the JIRA so Grails users don't expect if
(scopedProxy) to work outside the scope.
5. Small performance gain. Casting a GroovyObject no longer calls
getMetaClass() on every invocation. Values with per-instance metaclasses no
longer fail the guard and relink the cast site each time.
Backports:
- The PR links 6.0.x and 5.1.x commits, and Daniel agrees backporting is
warranted. I didn't review James's hand-applied 5.1.x version; it's worth
running the new test on that branch before you pick it.
- The PR says the bug reproduces on 4.0.33, and GROOVY_4_0_X has the same
unconditional receiver instanceof GroovyObject guard in setGuards. Grails 7
runs on Groovy 4.0.x. If the scoped-proxy report comes from a Grails 7 app,
4.0.x may be the backport that actually matters, so ask James whether he wants
it there too.
Nits (none blocking):
- The test uses static counters, which is fine as long as tests run
sequentially in one JVM.
- No test uses a real proxy subclass, but Watched covers the same path.
- The existing "call site stays uncached" log line in doCallSiteTargetSet is
misleading: the target is in fact set later.
> Indy: casting a GroovyObject calls getMetaClass() on the value being cast
> -------------------------------------------------------------------------
>
> Key: GROOVY-12557
> URL: https://issues.apache.org/jira/browse/GROOVY-12557
> Project: Groovy
> Issue Type: Bug
> Affects Versions: 4.0.33, 5.1.3, 6.0.0
> Reporter: James Daugherty
> Priority: Major
>
> With invokedynamic, a cast of a {{GroovyObject}} that already has the target
> type, such as {{Widget widget = value}}, calls {{value.getMetaClass()}}: once
> to build the call site's guard and again on each call through it. With
> classic call sites the same cast calls nothing on the value.
> That is observable whenever {{getMetaClass()}} is not a plain field read. A
> Spring class-based (CGLIB) proxy of a Groovy class forwards every public
> method, {{getMetaClass()}} included, to its target. For a step-, job-,
> request- or session-scoped bean the target exists only while the scope is
> active, so merely assigning the proxy to a typed variable outside the scope
> fails:
> {noformat}
> org.springframework.beans.factory.support.ScopeNotActiveException: Error
> creating bean with name 'scopedTarget.widget': Scope 'step' is not active for
> the current thread
> at
> org.springframework.aop.target.SimpleBeanTargetSource.getTarget(SimpleBeanTargetSource.java:35)
> at
> org.springframework.aop.framework.CglibAopProxy$DynamicAdvisedInterceptor.intercept(CglibAopProxy.java:698)
> at Widget$$SpringCGLIB$$0.getMetaClass
> at org.codehaus.groovy.vmplugin.v8.Selector$MethodSelector.setGuards
> at org.codehaus.groovy.vmplugin.v8.Selector$CastSelector.castAndSetGuards
> at org.codehaus.groovy.vmplugin.v8.Selector$CastSelector.setCallSiteTarget
> at org.codehaus.groovy.vmplugin.v8.IndyInterface.fallback
> {noformat}
> A Grails application moving from classic call sites to invokedynamic (the
> Grails 9 default) meets this in code that handles scoped proxies dynamically.
> *Reproducer:*
> https://github.com/jdaugherty/groovy-indy-cast-getmetaclass-reproducer
> ({{./run.sh 4.0.33 5.1.3 6.0.0}}). With indy the cast {{Watched typed =
> value}} makes 2 {{getMetaClass()}} calls on the value and fails when
> {{getMetaClass()}} throws; with classic call sites it makes none. Reproduced
> on 4.0.33, 5.1.3 and 6.0.0.
> *Cause:* {{Selector.CastSelector.castAndSetGuards()}} reuses
> {{MethodSelector.setGuards(args[0])}}, which calls {{getMetaClass()}} on a
> {{GroovyObject}} receiver to bind a {{SAME_MC}} guard. Only a cast to boolean
> selects a method ({{asBoolean}}) through the metaclass; every other cast is
> decided by the class of the value, which the argument guards and the class
> SwitchPoint already cover.
> *Proposed fix:* keep the receiver metaclass guard for the {{asBoolean}} path
> only, so a cast decided by the value's class never calls into the value, as
> with classic call sites. A patch with tests follows as a pull request against
> master, with backports for the 6.0.x and 5.1.x lines.
--
This message was sent by Atlassian Jira
(v10.3.26#10030026)
