Skip accessibility check in BoundField once it is known to have passed

[nileshpatil6]

Closes #2202

Follow-up to #3054, which I accidentally closed for good by deleting my fork while cleaning up old repos (sorry about that, GitHub won't let a PR reopen once the head repo is gone).

This is the simpler approach @eamonnmcmanus suggested there instead of my original AccessibleCache class: checkAccessible now takes an AtomicBoolean knownAccessible. If it's already true the reflective check is skipped, otherwise the check runs and sets it on success. The unusual inaccessible case just redoes the check and rebuilds the exception message every time, no caching of the failure.

The record constructor check in RecordAdapter passes a throwaway AtomicBoolean since it already only runs once per adapter.

Unlike last time I got a proper maven setup working locally, so this one is actually fully verified: mvn -pl gson verify passes end to end, 4619 tests, 0 failures, 0 errors (19 skipped), including spotless and error-prone.

[eamonnmcmanus]

Thanks for doing this! I think it is a worthwhile change, but we may be able to simplify.

[eamonnmcmanus]

I had a little difficulty persuading myself that this is fully correct. That is, if checkAccessible ever returns true for a given instance of the anonymous class below, then it always will, even if the source object changes. I think that is true, though. The caller used for the access check is always some code in Gson, so canAccess should be true only for members where we were able to call setAccessible. The result of canAccess should not depend on the source. So I think we're OK.

However, looking at the code I think we can probably simplify a bit. There are potentially a fair number of instances of the anonymous class below, and it's a pity for all of them to acquire two AtomicBoolean fields (for the local variables the class captures). First, I don't think we need two variables, since any given instance of the anonymous class will always have the same accessor value, which is either null or not. Second, even though it makes the code a bit bigger, I think it would be worth replacing the AtomicBoolean with a mutable private boolean knownAccessible field inside the anonymous class. Then:

if (blockInaccessible) {
  if (!knownAccessible) {
    if (accessor == null) {
      checkAccessible(source, field);
    } else {...}
    knownAccessible = true;
  }
}

and similarly for the readIntoField method.

[nileshpatil6]

Thanks, that's much better. Switched to a single private boolean knownAccessible on the anonymous class, so checkAccessible is back to its original two-arg form and the two AtomicBoolean captures are gone.

On whether one flag is enough, I convinced myself with the same argument you made plus one extra step, in case it helps anyone reading this later. accessor is only assigned when isRecord, and records deserialize through RecordAdapter.readField, which calls readIntoArray, not readIntoField. So the two members are never both consulted for the same BoundField instance: when accessor is non-null only write() ever runs a check, and when it is null both write() and readIntoField() check the same field. I put a short version of that in the javadoc on the flag.

One small thing worth flagging: in readIntoField the null check had to stay nested rather than folded into the outer condition, because that branch has an else if (isStaticFinalField) attached. Writing it as if (blockInaccessible && !knownAccessible) would let an already-checked field fall through into the static-final branch and throw the wrong error.

Also note the field is deliberately not volatile, so under concurrent use a thread may just not observe the write and redo the check. That is idempotent and gives the same answer, so it is benign, but say the word if you would rather it were volatile.

Rebased onto main since the branch had fallen behind. mvn -pl gson verify is green: 4619 tests, 0 failures, 0 errors, 19 skipped, spotless and error-prone included.

[nileshpatil6]

@Marcono1234 yes, that matches my reading. Worst case a thread observes a stale false and redoes the check, which is idempotent and always gives the same answer for a given member, so the race is benign and volatile would just put a barrier back on the hot path this change is removing.

Good point about protecting it from future refactoring though. I've added that to the javadoc on the field, including the one way the race could actually become unsafe: it is only benign because nothing negative is ever cached. If someone later extends this to remember failures, that assumption breaks. Spelled out so it survives outside this thread.

mvn -pl gson verify still green after the doc change, 4619 tests, 0 failures.

[Marcono1234]

Thanks! Also for considering this in the first place; I had overlooked that you had already mentioned this consideration in your previous comment, sorry.

[skrcode]

JAIPilot Cloud reviewed exact head 243a3fbe61abe7d26ae0fa8e0d07e7cdc10d1376. The production change already looks minimal, but the new positive-only accessibility cache had no repeated-call regression coverage.

I opened an AI-assisted, test-only draft against this branch: nileshpatil6#1

It checks that failed accessibility checks are never cached on either serialization or deserialization, and that alternating repeated operations remain behaviorally correct. The exact focused suite passed 18 tests and the full Gson module passed 4,622 tests with Spotless and Error Prone. These are behavior tests, not a direct cache-hit or timing claim. Independently diff-reviewed; please squash or cherry-pick with the required CLA/sign-off if useful.

[nileshpatil6]

@eamonnmcmanus friendly ping when you have a moment. The change you asked for landed a while back, so this is only waiting on a re-review to clear the blocking review.

Summary of what moved since your comment:

• The two captured AtomicBooleans are gone. There is now a single private boolean knownAccessible field on the anonymous BoundField subclass, and checkAccessible is back to its original two-argument form, matching the shape you sketched.
• One flag is sufficient, and the javadoc records why: accessor is fixed per instance and is non-null only for records, which deserialize through readIntoArray rather than readIntoField, so the two members are never both consulted for the same instance.
• Per @Marcono1234's point, the field is deliberately non-volatile. A stale false only causes a redundant, idempotent re-check. The javadoc states that this is benign only because nothing negative is ever cached, so a future change that remembers failures would have to revisit it.

mvn -pl gson verify is green: 4619 tests, 0 failures, 0 errors, 19 skipped, including spotless and error-prone. All 17 checks on the PR are passing.

On the test-only draft offered above: I have not folded it in. It is not my code and would need its author's CLA, and I would rather not grow this diff unless you actually want that coverage here. Happy to write equivalent tests myself if you think the caching behaviour warrants them.

[eamonnmcmanus]

Thanks again for doing this!

[eamonnmcmanus]

@skrcode feel free to send a separate PR with the test changes.