Skip to content

Commit 5a59ff6

Browse files
jderegclaude
andcommitted
Fix trySetAccessible() caching bug causing Field inaccessibility
WeakHashMap uses equals()-based lookup, but Field.equals() matches by declaring class + name + type, not identity. When getDeclaredFields() was called with different predicates, the JVM returned different Field instances for the same logical field. The cache returned TRUE for the second instance (via equals match), but setAccessible(true) was never called on it, leaving it inaccessible. This caused Traverser to silently skip inaccessible fields, breaking GraphComparator.applyDelta(). Fix: only cache FALSE (failures) to avoid expensive repeated exceptions. setAccessible(true) on an already-accessible field is a cheap no-op. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1 parent 9e33b6e commit 5a59ff6

1 file changed

Lines changed: 9 additions & 13 deletions

File tree

src/main/java/com/cedarsoftware/util/ClassUtilities.java

Lines changed: 9 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -2785,23 +2785,19 @@ private static Object invokeConstructorWithPlan(Converter converter, Object[] su
27852785
Collections.synchronizedMap(new WeakHashMap<>());
27862786

27872787
static void trySetAccessible(AccessibleObject object) {
2788-
// Check cache first to avoid repeated failed attempts
2789-
Boolean prev = accessibilityCache.get(object);
2790-
if (Boolean.FALSE.equals(prev)) {
2791-
// Known to fail under current VM/module setup – skip the throwing call
2792-
// This reduces noisy exceptions on hot paths (constructor/method selection loops)
2793-
// in JPMS-sealed modules
2788+
// Check cache for known failures only. We only cache FALSE (failures) to avoid
2789+
// expensive repeated exception throwing on JPMS-sealed modules. We do NOT cache
2790+
// TRUE because WeakHashMap uses equals()-based lookup, and Field.equals() matches
2791+
// by declaring class + name + type. Different Field instances for the same logical
2792+
// field (from separate getDeclaredFields() calls) would incorrectly share a TRUE
2793+
// cache entry, causing the second instance to never get setAccessible(true) called.
2794+
// Calling setAccessible(true) on an already-accessible field is a cheap no-op.
2795+
if (Boolean.FALSE.equals(accessibilityCache.get(object))) {
27942796
return;
27952797
}
2796-
if (Boolean.TRUE.equals(prev)) {
2797-
// Already accessible, no need to set again
2798-
return;
2799-
}
2800-
2801-
// Not in cache, attempt to set accessible
2798+
28022799
try {
28032800
object.setAccessible(true);
2804-
accessibilityCache.put(object, Boolean.TRUE);
28052801
} catch (SecurityException e) {
28062802
accessibilityCache.put(object, Boolean.FALSE);
28072803
if (LOG.isLoggable(Level.FINE)) {

0 commit comments

Comments
 (0)