blackdrag commented on code in PR #2842:
URL: https://github.com/apache/groovy/pull/2842#discussion_r3908117320
##########
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:
I wonder if we can simplify the accessibility/handle caching here and avoid
the synchronization.
Since `setAccessible(true)` changes the accessibility of the `Field` itself,
once Groovy has successfully established deep access, we can use Groovy's own
lookup to create the getter/setter handle and cache that handle. A handle
created with the caller's `Lookup` should not be cached, since it is
caller-specific.
This would give us roughly:
```java
MethodHandle h = getter;
if (h != null) return h;
if (!accessAttempted) {
try {
if
(ReflectionUtils.makeAccessibleInPrivilegedAction(field).isPresent()) {
h = getter = MethodHandles.lookup().unreflectGetter(field);
return h;
}
} finally {
accessAttempted = true;
}
}
return lookup.unreflectGetter(field);
```
and analogously for the setter.
`accessAttempted` would then mean only that we have already tried Groovy's
deep-reflection path. It could be `volatile`, allowing the benign race where
multiple threads perform the same `setAccessible`/handle creation concurrently.
I don't think that race matters, since all threads operate on the same `Field`
and the resulting handles are equivalent, whereas synchronizing the
initialization adds blocking just to avoid that duplicate work.
The important part for me is that the successful deep-reflection handle can
be cached, while a handle obtained from the caller's `Lookup` cannot. This also
means that if `setAccessible` fails, we remember that attempt and fall back to
the caller's `Lookup` without repeatedly trying the deep-reflection path.
Using this implementation also changes the need for the exception handling
changes in MetaClassImpl
--
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]
