[
https://issues.apache.org/jira/browse/GROOVY-12361?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18111962#comment-18111962
]
ASF GitHub Bot commented on GROOVY-12361:
-----------------------------------------
daniellansun commented on PR #2882:
URL: https://github.com/apache/groovy/pull/2882#issuecomment-5555679838
This is a set of notes on the commit, offered with respect — the overall
direction looks right to me.
## Overall impression
The diagnosis and the production-side structure both seem sound to me.
The trampoline encodings name the declaring class, return type, and
parameter types in the constant pool (`CHECKCAST` / `INVOKE*` / `invokeExact`).
A hidden class is still identity-visible to the defining loader, so
`canResolveInvokeTypes` / Step 1 can succeed, the hidden class defines, and the
first invoke throws `NoClassDefFoundError`. That reading of GROOVY-12361
matches the bytecode in `InvokerBytecode`.
A single sticky decline in `tryCreate` also feels like the right place to
hold the invariant. Patching Step 1 alone would not be enough: Step 2 already
refuses a hidden nest host via `HiddenClassDefiner.canAttemptPrivateLookup`,
but Step 3 (classData) and Step 4 (visible artifact) could still emit
uninvokable bytecode. Folding `isHidden()` into `loaderCanResolve` would
similarly miss Step 2/4, which do not walk the method’s types.
So the “code judo” here — four failure modes collapsed into one policy
decline, with the reflective path remaining the fallback — is, in my view, the
correct layer. I would not try to “fix” hidden types by asType-erasing
classData so they can trampoline; that would add a third encoding for a case
that has always been reflective.
The comments in `tryCreate` / `allTypesNameable` are also very helpful: they
explain *why* define succeeds and invoke fails, which is the non-obvious part.
What I would be grateful to see tightened is mainly the tests (and one small
layering point in `tryCreate`). I would not want to hold up the production
idea; I would only suggest we lock the invariant a bit more firmly before
landing.
## Suggestions
### 1. Consider treating the hidden-type check as a policy decline, next to
the existing ones
```99:121:src/main/java/org/apache/groovy/internal/runtime/invoke/InvokerFactory.java
public static DirectInvoker tryCreate(final CachedMethod method) {
if (method == null || !generationAllowed()) {
return null;
}
if (method.isCallerSensitive() ||
Modifier.isAbstract(method.getModifiers())) {
return null;
}
try {
if (!allTypesNameable(method.getCachedMethod())) {
// ...
return null;
}
return defineSteps(method);
} catch (Exception | LinkageError ignored) {
```
Caller-sensitive and abstract are already sticky policy declines *before*
the `try`. Hidden types seem to be the same kind of decision — “please do not
generate” — rather than a define/link failure. Sitting inside `catch (Exception
| LinkageError)` slightly blurs that boundary, and `getCachedMethod()` (which
may `setAccessible`) becomes a side effect of the predicate.
Would it be reasonable to lift the check next to the other declines, and
take `CachedMethod` the way `isPubliclyInvocableFromInvokerFactory` already
does (`getDeclaringClass().getTheClass()`, `getReturnType()`,
`getNativeParameterTypes()`)? That would also match the types `InvokerBytecode`
actually writes, without calling `getCachedMethod()` only to inspect names.
A small related thought: the class javadoc’s define-path list does not yet
mention this precondition. A reader of `tryStep1` still sees “loader can
resolve these types” and could reasonably think a public hidden nestmate is
legal. A sentence next to the four steps might save the next reader a trip
through GROOVY-12361.
I would *not* suggest sprinkling `isHidden()` into `tryStep1` / `tryStep3` /
`canResolveInvokeTypes` in addition to `tryCreate`. That would be the scattered
version of this fix; the single gate is the cleaner model.
### 2. `testHiddenProxyStaysInvocablePastThreshold` may pass without
exercising the bug
```502:518:src/test/groovy/org/apache/groovy/internal/runtime/invoke/InvokerFactoryTest.groovy
@Test
void testHiddenProxyStaysInvocablePastThreshold() {
String old = System.getProperty(InvokerFactory.PROPERTY_THRESHOLD)
System.setProperty(InvokerFactory.PROPERTY_THRESHOLD, '0')
try {
Object proxy = [name: { 'circle' }] as HiddenProxyBase
// ...
3.times { assertEquals('circle', name.invoke(proxy, new
Object[0])) }
} finally {
if (old == null)
System.clearProperty(InvokerFactory.PROPERTY_THRESHOLD)
else System.setProperty(InvokerFactory.PROPERTY_THRESHOLD, old)
}
}
```
This looks like the only test that walks the production hook
(`CachedMethod.invoke` after the threshold), which is exactly the coverage we
want. A few details made me slightly uneasy that it might stay green without
generation ever running:
- Every other property-mutating test in this file, and in
`CachedMethodDirectInvokerTest`, uses
`@ResourceLock(Resources.SYSTEM_PROPERTIES)`. This one does not. Under parallel
execution, a sibling could restore the default threshold while this test is
running. Then `hits > 1000` is never true, generation never runs, and three
reflective invokes would still pass.
- It never asserts `invokerAttempted` / `invoker == null`. The sticky-fail
tests in this same file already do that
(`testCachedMethodStickyFailsWhenAllDefineStepsFail`). Without those fields,
“invoke still works” is not quite proof that the trampoline was refused.
- Save/restore is inlined; `restoreProperty` is already the local helper.
`testTryCreateDeclinesPublicHiddenHost` also splits `cm(ping)` across two
instances, so the `invoke` after `tryCreate` is a fresh `CachedMethod` that
never reaches threshold. That test still proves the factory returns null, which
is valuable; it just does not prove the hook sticky-fails. The e2e test is the
one that has to carry that load.
If you agree, matching `CachedMethodDirectInvokerTest`’s `withThreshold(0L,
…)` shape would make this much harder to go green accidentally: lock, one
`CachedMethod`, invoke past threshold, assert sticky-fail, then invoke again.
### 3. The old Step 3 fall-through test may now be slightly misleading
`testDefineStepsFallsThroughToClassDataForHiddenNonPublicHost` no longer
tests fall-through. `tryCreate` returning null is a good assertion — it is the
regression guard that a Step 1-only patch would miss. The second half then
drives `tryCreateClassData` and asserts that an uninvokable trampoline still
defines.
I may be missing a reason to keep that second half; from the outside it
looks like a dead production path. `testClassDataEncodingInvokesPublicMethod`
already covers classData on types that can actually run. A “definition
succeeds; do not invoke” fixture might document a landmine rather than remove
it.
Would you consider renaming to something like
`testTryCreateDeclinesNonPublicHiddenHost`, keeping `assertNull(tryCreate(…))`,
and dropping the `tryCreateClassData` success assertion?
I realise that once hidden hosts cannot be the Step 3 fixture, the
production `tryCreate → defineSteps → tryStep3` orchestration is only hit via
the reflective `tryStep3` probe. That seems acceptable — I would only suggest
that the test name and comments not claim to cover `defineSteps` fall-through
any more.
## Things I would not chase in this commit
- A shared “nameable type” helper. `CallSiteGenerator` has a similar
constant-pool shape, but it is out of this commit and on the deprecated classic
call-site path. If a second call site appears later, `HiddenClassDefiner` would
be a more natural home than growing `InvokerFactory`.
- Extra tests for hidden parameter/return types without a hidden declaring
class. That shape is almost unreachable from Groovy source. Declaring-class
coverage (public + non-public + map-coerced proxy) is the production case, and
the three-type walk in `allTypesNameable` is still the right defensive check.
## Summary
I think the production structure is the right one: one sticky decline at
`tryCreate`, not a hidden-class special case in each define step. The notes
above are about making that decline sit with the other policy gates, and about
making the e2e test force generation under a property lock and assert
sticky-fail, plus renaming the old Step 3 test so it no longer claims to cover
`defineSteps` fall-through.
Thank you for the careful comments in the factory — they made the failure
mode much easier to follow.
> CachedMethod DirectInvoker: trampoline for a hidden declaring class throws
> NoClassDefFoundError on first invoke
> ---------------------------------------------------------------------------------------------------------------
>
> Key: GROOVY-12361
> URL: https://issues.apache.org/jira/browse/GROOVY-12361
> Project: Groovy
> Issue Type: Bug
> Reporter: Paul King
> Assignee: Paul King
> Priority: Major
>
> The generated {{DirectInvoker}} trampoline from GROOVY-12325 names the target
> method's declaring class in its bytecode ({{CHECKCAST}} on the receiver,
> {{INVOKEVIRTUAL}}/{{INVOKEINTERFACE}} on the declaring type, or the
> {{invokeExact}} descriptor on the classData path). When that declaring class
> is a *hidden class* (JEP 371), the name is not resolvable by any class
> loader: the trampoline defines and initialises fine, but the first call
> through it fails with {{NoClassDefFoundError}}. The error escapes
> {{CachedMethod.invoke}} as-is because generation is sticky-successful and the
> failure only happens at invocation time.
> Groovy itself produces hidden declaring classes: {{ProxyGeneratorAdapter}}
> defines its proxies as hidden nestmates (GROOVY-12223) whenever the
> superclass is on the same loader as Groovy, so any map-as-abstract-class
> coercion of an application-classpath type is affected once the method has
> been invoked {{groovy.cachedmethod.invoker.threshold}} times (default 1000).
> h3. Reproducer
> Precompile so that {{Shape}} sits next to the Groovy jar on the application
> class path (a script compiled by {{GroovyClassLoader}} gets an ordinary,
> non-hidden proxy and does not reproduce):
> {code:groovy}
> abstract class Shape { abstract String name() }
> class ProxyRepro {
> static void main(String[] args) {
> def s = [name: { 'circle' }] as Shape
> println "hidden proxy: " + s.getClass().isHidden()
> // two cold call sites share the CachedMethod; its hit count reaches
> the
> // trampoline threshold while each indy site is still below its own
> 600.times { assert s.name() == 'circle' }
> 600.times { assert s.name() == 'circle' }
> println "ok"
> }
> }
> {code}
> {noformat}
> $ java -cp groovy-6.0.0-SNAPSHOT.jar
> org.codehaus.groovy.tools.FileSystemCompiler -d out ProxyRepro.groovy
> $ java -cp groovy-6.0.0-SNAPSHOT.jar:out ProxyRepro
> hidden proxy: true
> Exception in thread "main" java.lang.NoClassDefFoundError:
> Shape1_groovyProxy/0x0000007001120000
> at
> org.codehaus.groovy.reflection.CachedMethod.invokeGenerated(CachedMethod.java:491)
> at
> org.codehaus.groovy.reflection.CachedMethod.invoke(CachedMethod.java:452)
> at groovy.lang.MetaMethod.doMethodInvoke(MetaMethod.java:298)
> at
> org.codehaus.groovy.vmplugin.v8.IndyInterface.invokeColdReflective(IndyInterface.java:650)
> ...
> {noformat}
> A single call site masks the bug because the indy site promotes to the full
> method-handle chain at the same hit count (1000) and stops going through
> {{CachedMethod.invoke}}; two sites sharing the method, any MOP-path
> invocation ({{invokeMethod}}, categories, per-instance metaclass,
> {{@Delegate}} etc.), or {{-Dgroovy.cachedmethod.invoker.threshold=0}} exposes
> it. Setting either {{-Dgroovy.indy.cold.reflection=false}} or
> {{-Dgroovy.cachedmethod.invoker.disable=true}} avoids it. JDK 21, Groovy
> master (6.0.0-SNAPSHOT).
> h3. Fix
> {{InvokerFactory.tryCreate}} should decline (sticky null, so
> {{CachedMethod.invoke}} stays reflective) when the declaring class, the
> return type or any parameter type (array components included) {{isHidden()}}.
> All four define steps are affected, including Step 3 (classData), whose
> {{invokeExact}} descriptor also names the type.
> {{InvokerFactoryTest.testDefineStepsFallsThroughToClassDataForHiddenNonPublicHost}}
> currently asserts the opposite and even notes that the resulting trampoline
> must not be invoked; it needs to assert {{null}} instead. A fix with tests is
> available on branch {{groovy12354spike}} (the {{allTypesNameable}} gate).
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
