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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
