Repository navigation
Do not return a null name when the name cache is cleared concurrently - #739
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (1)
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthrough
ChangesAtom Lookup
Priority: ⬇️ Low Estimated code review effort: 1 (Trivial) | ~5 minutes Change: Bug fix Suggested reviewers: Merge Risk: ⚪ Minimal · up to 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)✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
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. Comment |
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.
ba88663 to
84093cf
Compare
Summary
ASAtom.getASAtom(String)looks up a dynamic name withcontainsKeyfollowed byget. Since#730,
ASAtom.clearCache()can empty that cache at runtime. If it runs on another threadbetween the two calls,
getASAtomreturnsnullfor a non-null name, and the caller receivesa null atom. CodeRabbit pointed this out on #730.
Changes
getASAtomreads the cache with a singlegetand returns the result only when it isnon-null; otherwise it creates the atom as before.
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
getASAtomfor 64 names while one thread callsclearCache()ina loop, 4 seconds per run) returned
nullbetween 31,000 and 33,000 times per run before thischange and never after it, over three runs each.
Backward compatibility
No behaviour change when
clearCache()is not called concurrently. Java 8 compatible; all 386parser tests pass.
Summary by CodeRabbit