[
https://issues.apache.org/jira/browse/GROOVY-12314?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18110471#comment-18110471
]
ASF GitHub Bot commented on GROOVY-12314:
-----------------------------------------
paulk-asert commented on code in PR #2842:
URL: https://github.com/apache/groovy/pull/2842#discussion_r3909616210
##########
src/main/java/org/codehaus/groovy/reflection/CachedField.java:
##########
@@ -46,10 +46,19 @@ public CachedField(final Field field) {
}
private final Field field;
- private boolean madeAccessible;
+ private volatile boolean madeAccessible;
+ private boolean accessAttempted; // guarded by synchronization on this
private void makeAccessible() {
- ReflectionUtils.makeAccessibleInPrivilegedAction(field);
- madeAccessible = true;
+ // at most one attempt, remembering either outcome: a failed attempt
(strongly
+ // encapsulated declaring class) cannot succeed later. The attempt is
recorded
+ // only once it has completed, under synchronization, so a concurrent
caller
+ // waits instead of reading the field before setAccessible has taken
effect.
+ synchronized (this) {
Review Comment:
Implemented as you sketched in ed4ec398f8: the deep-reflection getter/setter
handles are created through Groovy's own lookup once `setAccessible` has
succeeded and cached; a handle from the caller's lookup is returned but never
cached. The synchronization is gone.
Three details on top of the sketch, for the record:
- **The one thing I kept from the synchronized version is the write
ordering**: `accessAttempted` is recorded only *after* the attempt completes
(both volatile). The benign race is exactly as you described for the handle
paths — duplicate `setAccessible`/unreflect on the same `Field` produce
equivalent handles — but the classic `Field.get`/`set` paths share the same
latch and have no handle to hand them safety, so the ordering is what gives a
thread that observes `accessAttempted` a happens-before edge to the outcome and
to the field's accessibility. That closes, without blocking, the race the
stress test caught (a concurrent first access could observe the latch before
`setAccessible` had taken effect, and the resulting `IllegalAccessException`
surfaced as a spurious missing property). The `ClassInfoSoftModeStressTest`
loop that failed 2/6 against the synchronized-less latch is 10/10 on this.
- **No deep setter is created for a final field**: after `setAccessible`,
`unreflectSetter` would happily hand out a working write handle for a
non-static final, so that case deliberately falls through to the caller's
lookup and keeps failing, preserving the `ReadOnlyPropertyException` semantics.
- **Ordering flip acknowledged**: this tries deep reflection before the
caller's lookup, the reverse of the GROOVY-9144/9596 shape. That ordering
existed to avoid needless `setAccessible` in the illegal-access-warning era; on
the JDKs we support, `setAccessible` either succeeds silently or throws, and I
checked the four cases (open/encapsulated × entitled/unentitled caller) all end
in the same outcome — the encapsulated-but-entitled case (`FilterReader#in`
from a subclass) pays one latched failed attempt and then goes through the
caller lookup as before.
On the exception handling in MetaClassImpl: I've left it in place for now.
After this change it is at least *deterministic* — the only
`IllegalAccessException` left on the classic path is the "deep path unavailable
and no lookup to ask" state, so converting it to missing-member handling no
longer risks masking a transient failure. I agree the conversions could become
plain control flow (the classic path can now ask whether deep access is
available before attempting the read), but I'd want that check to stay at the
use sites rather than migrate into `getEffectiveGetMetaProperty` — filtering
during selection is what the earlier round of this review removed, and it's
what kept the map-precedence and Selector entanglement alive. Happy to do that
simplification here or as an immediate follow-up, whichever you prefer.
> Align field-backed property access across compilation modes
> -----------------------------------------------------------
>
> Key: GROOVY-12314
> URL: https://issues.apache.org/jira/browse/GROOVY-12314
> Project: Groovy
> Issue Type: Improvement
> Reporter: Paul King
> Assignee: Paul King
> Priority: Major
> Labels: breaking
>
> Found while investigating GROOVY-12305 (see also GROOVY-12290, GROOVY-9967):
> a family of edge cases where property access that resolves to a *field*
> behaved differently under dynamic Groovy, {{@TypeChecked}} and
> {{@CompileStatic}} — different values, different failure modes, or silently
> wrong values. Each item is individually minor; taken together they justify
> aligning the three modes even though some behavior changes result.
> h3. Behavior before this ticket (measured on 4.0.33, 5.1.1, 6.0.0-beta-3 and
> master)
> *A. Non-public fields of foreign (JDK) classes*
> {code:groovy}
> def list = [1, 2]
> println list.modCount // protected field inherited from
> java.util.AbstractList
> println list.elementData // package-private field of java.util.ArrayList
> println list.@modCount // attribute access
> {code}
> ||access (dynamic, no add-opens)||4.0.33||5.1.1||beta-3 / master||
> |{{list.modCount}} (protected)|MissingPropertyException|*GroovyBugError*
> "BUG! UNCAUGHT EXCEPTION: member is protected"|*GroovyBugError*|
> |{{list.elementData}}
> (package-private)|MissingPropertyException|MissingPropertyException|MissingPropertyException|
> |{{list.@modCount}}|MissingFieldException|raw IllegalAccessException|raw
> IllegalAccessException|
> The GroovyBugError was a 4→5 regression:
> {{Selector$PropertySelector.chooseMeta}} wrapped the refused
> {{MethodHandles}} lookup in GroovyBugError (vmplugin/v8/Selector.java,
> GROOVY-9144/9596 code). It was not catchable as MissingPropertyException and
> presented as an internal bug. The {{.@}} error-shape change
> (MissingFieldException → raw IllegalAccessException escaping from
> {{CachedField.getProperty}}) was likewise a 4→5 regression.
> The static modes for the same expressions: {{@TypeChecked}} compiled the
> property reads ({{storeField}} admitted the fields — {{isFieldAccessible}}'s
> exact-receiver leniency still covered package-private, and {{storeField}}
> deliberately proceeded for inaccessible protected fields), then failed at
> runtime as above. {{@CompileStatic}} failed during class generation with
> {{Access to E#modCount is forbidden @ line -1, column -1}} — an unresolved
> type parameter as the receiver name and no source position.
> With {{--add-opens java.base/java.util=ALL-UNNAMED}}, every spelling above
> works and prints the field value, on all versions.
> *B. Collection {{size}}/{{length}} classgen shortcut*
> {{StaticTypesCallSiteWriter#makeGetPropertySite}} rewrote {{size}}/{{length}}
> on Collection receivers to {{size()}} *before* getter/map-rule/field lookup.
> Measured under {{@CompileStatic}} (the dynamic column is identical on all
> four versions):
> {code:groovy}
> class C { int getSize() { 999 } } // Groovy class
> // JColl: Java class extends ArrayList<Object> with public int size = 42,
> public int length = 99
> {code}
> ||scenario (CS)||4.0.33||5.1.1||beta-3 / master||dynamic (all versions)||
> |{{List l = \[1,2\]; l.size}}|2|2|STC error|MissingPropertyException|
> |{{l.length}}|STC error|STC error|STC error|MissingPropertyException|
> |Groovy class with {{getSize()}}: {{c.size}}|999|999|999|999|
> |Groovy class, public field {{size=42}}: {{d.size}}|42|42|42|42|
> |{{List<C> l; l.size}} (element {{getSize()}})|*2*|*2*|\[999, 999\]|\[999,
> 999\]|
> |Java class, public field {{size=42}}: {{j.size}}|*1*|*1*|*1*|42|
> |Java class, public field {{length=99}}: {{j.length}}|*1*|*1*|*1*|99|
> Notes:
> * The shortcut's only mainstream feeder was STC resolving {{l.size}} to
> ArrayList's *private* {{int size}} field via the exact-receiver leniency —
> closed by GROOVY-12290 — which is why rows 1 and 5 changed in 6.0.0-beta-3
> (aligning with dynamic semantics).
> * Row 5 on 4.x/5.x was a three-way divergence: dynamic gives \[999, 999\];
> {{@TypeChecked}} returned \[999, 999\] at runtime while statically typing the
> expression {{int}} (so {{int n = l.size}} type-checked cleanly then threw
> GroovyCastException); {{@CompileStatic}} gave 2.
> * Rows 6–7 were silently wrong values on *every* version: STC legitimately
> admits the public field, but the shortcut hijacked the access to {{size()}}.
> Groovy-class receivers were immune because
> {{makeGroovyObjectGetPropertySite}} has no such shortcut — the result
> differed depending on whether the receiver class was written in Java or
> Groovy.
> h3. Changes made (one commit each)
> # *MOP*: a field whose reflective access cannot be established (new
> {{CachedField#isAccessEstablishable}}, backed by {{checkCanSetAccessible}})
> is treated as absent during meta-property selection — property get/set and
> attribute get/set — so the normal missing-member handling applies. The indy
> selectors degrade to the generic MetaProperty or the sender-aware adapter
> path instead of throwing GroovyBugError. Forceable access (open modules,
> class-path classes, {{--add-opens}}) is unaffected. This commit alone fixes
> the 5.x GroovyBugError/IllegalAccessException regressions and is a back-port
> candidate for GROOVY_5_0_X.
> # *STC*: the GROOVY-12290 rule extended — the exact-receiver leniency no
> longer admits plain property syntax to *any* field of a foreign nest that
> Java access rules reject (was: private only), and an inaccessible protected
> field no longer backs a property at all. Resolution falls through to
> accessors, extensions or the map/list handling, so the checker rejects at
> compile time what the dynamic MOP reports as missing at run time. Escape
> hatches unchanged: attribute access, closure bodies, delegate-resolved
> access, nest-mates, and everything Java admits (same package, protected from
> a subclass).
> # *Classgen*: the Collection {{size}}/{{length}}-to-{{size()}} rewrite
> removed. Post-GROOVY-12290 it was vestigial, and the cases still reaching it
> produced wrong values; a public {{size}}/{{length}} field now resolves
> through the normal field handling.
> # *Tests*: the two {{DifferentPackageTest}} scenarios now expect the
> positioned type-checking error ("No such property") instead of class
> generation's "Access to ... is forbidden".
> # *Classgen*: the safety-net "Access to ... is forbidden" error now reports
> the placeholder's erasure (was "E") and falls back to the current statement's
> position (was line -1, column -1).
> # *indy*: the metaclass skips fields reflection cannot force, but that
> constraint belongs to the classic ({{Field.get}}) access path only — a sender
> that passes Java's access rules (e.g. a {{FilterReader}} subclass reading the
> protected {{in}} field) still reaches the field through its own
> {{MethodHandles}} lookup, exactly like javac-emitted bytecode. When the
> effective meta property comes back as a fallback, the property-get selector
> retries the raw field via the sender lookup before binding the fallback.
> (Selection itself must stay sender-blind: classic API calls pass the receiver
> class as the sender, which would otherwise grant phantom privileges.)
> h3. Resulting behavior (verified)
> ||scenario (no add-opens)||dynamic||@TypeChecked||@CompileStatic||
> |{{list.size}} / {{list.length}}|MissingPropertyException|STC error|STC error|
> |{{list.modCount}} read (protected, foreign sender)|MissingPropertyException
> (was BUG!)|STC error (was compiles→BUG!)|STC error (was line -1 classgen
> error)|
> |{{list.elementData}} read (package-private)|MissingPropertyException|STC
> error (was compiles→runtime MPE)|STC error (was line -1 classgen error)|
> |{{list.modCount}} write|ReadOnlyPropertyException (was BUG!-adjacent; 4.x
> threw IllegalArgumentException)|STC error|STC error|
> |{{list.@modCount}} read / write|MissingFieldException (was raw IAE)|STC
> error "Cannot access field" (unchanged)|STC error (unchanged)|
> |protected field of super class from a *subclass* (e.g.
> {{FilterReader#in}})|works (sender lookup)|works|works|
> |Groovy {{getSize()}} / Groovy public field / element spread|999 / 42 /
> \[999, 999\]|999 / 42 / \[999, 999\]|999 / 42 / \[999, 999\]|
> |Java public field {{size}}/{{length}}|42 / 99|42 / 99|42 / 99 (was 1)|
> ||with --add-opens||dynamic||@TypeChecked||@CompileStatic||
> |{{list.modCount}}|field value (kept)|STC error|STC error|
> |{{list.@modCount}}|field value|field value|field value|
> All three modes agree on every row: the same value, or the static modes
> rejecting at compile time exactly what dynamic reports as missing at run
> time. All failures are well-formed — catchable
> MissingPropertyException/MissingFieldException/ReadOnlyPropertyException at
> runtime, positioned STC errors at compile time; no GroovyBugError, no "line
> -1" classgen errors. The one deliberate asymmetry: with {{--add-opens}},
> dynamic property syntax can still read a protected field, while the static
> modes require the explicit {{.@}} spelling.
> h3. Behavior changes (accepted as the price of alignment)
> * {{@TypeChecked}}/{{@CompileStatic}} code reading package-private/protected
> foreign fields via property syntax stops compiling (previously it failed at
> runtime or with a malformed classgen error; {{.@}} remains rejected
> statically as before, and dynamic {{.@}} works under {{--add-opens}}).
> * Cross-package {{@PackageScope}} field misuse now fails during type checking
> with "No such property" instead of during class generation with "Access to
> ... is forbidden".
> * {{@CompileStatic}} on a Java Collection class with a public
> {{size}}/{{length}} field changes from the element count to the field value
> (bug fix, but observable).
> * A dynamic property *write* to a strongly encapsulated field now throws
> ReadOnlyPropertyException (accurate: the field exists but cannot be written
> that way; previously a raw IllegalAccessException-based failure,
> IllegalArgumentException on 4.x).
> * Already shipped in 6.0.0-beta-3 via GROOVY-12290, noted here for the
> migration notes: {{list.size}} under {{@CompileStatic}} is now a compile
> error; on 4.x/5.x it compiled and returned the element count.
> Validated with the full core test suite (17,244 tests) plus the complete
> scenario matrix above run against 4.0.33, 5.1.1, 6.0.0-beta-3 and the patched
> build, with and without {{--add-opens}}.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
