Safely publish lazily-initialized caches shared across threads - #19137
Merged
Conversation
Several lazily-initialized caches are read concurrently but written to non-volatile fields, so their contents are published unsafely. On weakly-ordered hardware (e.g. AArch64) a racing thread can observe the non-null reference while the writes that filled it are still invisible to it. - `BaseInPredicate`: the eight parsed-value arrays. A predicate belongs to the query's filter tree, which is shared while `CombinePlanNode` builds the per-segment plans in parallel; the arrays are reached from `InPredicateEvaluatorFactory` / `NotInPredicateEvaluatorFactory` / `PredicateUtils`. Arrays get no final-field protection, so a stale read yields `0` for the primitive arrays and silently matches the wrong rows rather than failing. - `LiteralContext`: the eight lazily converted values, shared the same way. - `RegexpLikePredicate`: the compiled pattern. Only exposed when the RE2J engine is configured, since `Re2jPattern` held its delegate in a non-final field while `JavaUtilPattern` did not; that field is now final too, which fixes the hazard at its source. - `AbstractIndexType`: an index type is a process-wide singleton held by `IndexService`, used concurrently by the threads that load, reload and refresh segments. - `DateTimeFieldSpec`: the format and granularity specs hang off a cached `Schema` that query threads read concurrently. Every one of these getters already used the racy-single-check idiom; only the `volatile` was missing. The fields are made `volatile` uniformly rather than case by case: whether a given cached value happens to be safe today depends on the field modifiers of a class in another module, which is not an invariant worth depending on. Also narrows the two `Pattern` wrappers' delegate fields to `private`, which nothing outside those classes referenced.
xiangfu0
approved these changes
Jul 31, 2026
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #19137 +/- ##
============================================
- Coverage 65.61% 65.61% -0.01%
Complexity 1423 1423
============================================
Files 3439 3439
Lines 218184 218189 +5
Branches 34660 34663 +3
============================================
- Hits 143161 143155 -6
- Misses 63465 63480 +15
+ Partials 11558 11554 -4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Several lazily-initialized caches on objects that are read by multiple threads are written to non-
volatilefields, so their contents are published unsafely. On weakly-ordered hardware (e.g. AArch64/Graviton) a racing thread can observe the non-null reference while the writes that filled it are still invisible to it.This is the same defect class as #19117 (
DataSchema.getStoredColumnDataTypes), found by auditing for the rest of the pattern. Every getter changed here already used the racy-single-check idiom — read the field into a local, check it, publish it — so only thevolatilewas missing.Changes, most to least impactful
BaseInPredicate— the eight lazily parsed value arrays. A predicate belongs to the query's filter tree, which is shared across the threadsCombinePlanNodeuses to build per-segment plans in parallel; the arrays are reached fromInPredicateEvaluatorFactory,NotInPredicateEvaluatorFactoryandPredicateUtils. Arrays get no final-field protection, so a racing thread can read the array reference with its elements still at0— silently matching the wrong rows rather than failing.INis common enough that this is the main motivation for the change.RegexpLikePredicateandRe2jPattern— the compiled pattern, shared the same way. This is only exposed when the RE2J engine is configured:JavaUtilPatternholds its delegate in afinalfield, so JLS 17.5 covers it, whereasRe2jPatterndid not.Re2jPattern._patternis nowfinalas well, which closes the hazard at its source for every consumer rather than only for this caller.LiteralContext— the eight lazily converted values, in the same shared expression tree.AbstractIndexType— an index type is a process-wide singleton held byIndexService, used concurrently by the threads that load, reload and refresh segments.DateTimeFieldSpec— the format and granularity specs hang off a cachedSchemathat query threads read concurrently.Why
volatileuniformly rather than case by caseSome of these values are safe today without
volatile, because the cached object happens to have onlyfinalfields — JLS 17.5 covers those, and everything reachable from them, even through a data race. That is a poor thing to depend on: it makes one class's thread-safety hinge on the field modifiers of a class in another module, with nothing at either site recording the dependency, and it breaks silently when a field is added or a constructor is bypassed. The twoPatternimplementations here already disagreed on exactly that point. So the fields are madevolatileuniformly, and the finality argument is used only to judge which of these is urgent — not whether to fix it.Also
JavaUtilPattern._patternandRe2jPattern._patternare narrowed toprivate. Nothing outside those classes referenced them — both callers go throughgetPattern()— and the two classes are otherwise mirror images, so this keeps them consistent.No tests are included: an unsafe-publication race is not reliably reproducible in a unit test without a
jcstress-style harness, which this repo does not currently have. This matches how #19117 was handled.