Skip to content

Do not return a null name when the name cache is cleared concurrently - #739

Merged
MaximPlusov merged 1 commit into
veraPDF:integrationfrom
softvision-dev:asatom-single-cache-lookup
Oct 9, 2026
Merged

MaximPlusov merged 1 commit into
veraPDF:integrationfrom
softvision-dev:asatom-single-cache-lookup

Conversation

@softvisionfd

@softvisionfd softvisionfd commented Oct 8, 2026 •

Copy link
Copy Markdown
Contributor

Summary

ASAtom.getASAtom(String) looks up a dynamic name with containsKey followed by get. Since
#730, ASAtom.clearCache() can empty that cache at runtime. If it runs on another thread
between the two calls, getASAtom returns null for a non-null name, and the caller receives
a null atom. CodeRabbit pointed this out on #730.

Changes

  • getASAtom reads the cache with a single get and returns the result only when it is
    non-null; otherwise it creates the atom as before.
  • The predefined-name lookup is unchanged; that map is never cleared.

Verification

The race is timing-dependent, so this PR adds no unit test for it. A stress probe outside the
code base (19 threads calling getASAtom for 64 names while one thread calls clearCache() in
a loop, 4 seconds per run) returned null between 31,000 and 33,000 times per run before this
change and never after it, over three runs each.

Backward compatibility

No behaviour change when clearCache() is not called concurrently. Java 8 compatible; all 386
parser tests pass.

Summary by CodeRabbit

  • Refactor
    • Streamlined PDF atom lookup while preserving existing behavior. This internal optimization does not change how atom names are resolved or affect user-facing functionality.

@coderabbitai

coderabbitai Bot commented Oct 8, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: defaults
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 30d34817-4d37-4677-93f0-0637adb20921

📥 Commits

Reviewing files that changed from the base of the PR and between a598f30 and 84093cf.


📒 Files selected for processing (1)
  • src/main/java/org/verapdf/as/ASAtom.java

Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.



📝 Walkthrough

Walkthrough

getASAtom now uses one CACHED_PDF_NAMES lookup. It returns the cached atom when the result is non-null.

Changes

Atom Lookup

Layer / File(s) Summary
Cached atom lookup
src/main/java/org/verapdf/as/ASAtom.java
getASAtom replaces separate containsKey and get calls with one lookup. When the result is null, atom creation continues.

Priority: ⬇️ Low

Estimated code review effort: 1 (Trivial) | ~5 minutes

Change: Bug fix

Suggested reviewers: lonelymidoriya


Merge Risk: ⚪ Minimal · up to 84093

The change addresses the reported null-return race, and the supplied context identifies no remaining actionable merge risk.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check Passed The title clearly and concisely describes the main change: preventing null results when the name cache is cleared concurrently.
Docstring Coverage Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 1 files.
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.


✨ Finishing Touches 💡 1
🛠️ Fix failing CI checks 💡
  • Commit to this branch
  • Create a new PR

🧪 Generate unit tests (beta)
  • Create a new PR


  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

getASAtom looked up a dynamic name with containsKey followed by get. Since
clearCache() can empty the cache at runtime, a clear on another thread
between the two calls made getASAtom return null for a non-null name. Read
the cache with a single get and fall back to creating the atom when the
result is null. The predefined-name lookup is unchanged; that map is never
cleared.
@MaximPlusov
MaximPlusov force-pushed the asatom-single-cache-lookup branch from ba88663 to 84093cf Compare October 9, 2026 10:51
@MaximPlusov
MaximPlusov merged commit f422c5b into veraPDF:integration Oct 9, 2026
7 of 8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants