Jackie-Jiang opened a new pull request, #19137: URL: https://github.com/apache/pinot/pull/19137
## Summary Several lazily-initialized caches on objects that are read by multiple threads are written to non-`volatile` fields, 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 the `volatile` was 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 threads `CombinePlanNode` uses to build per-segment plans in parallel; the arrays are reached from `InPredicateEvaluatorFactory`, `NotInPredicateEvaluatorFactory` and `PredicateUtils`. Arrays get no final-field protection, so a racing thread can read the array reference with its elements still at `0` — silently matching the wrong rows rather than failing. `IN` is common enough that this is the main motivation for the change. **`RegexpLikePredicate`** and **`Re2jPattern`** — the compiled pattern, shared the same way. This is only exposed when the RE2J engine is configured: `JavaUtilPattern` holds its delegate in a `final` field, so JLS 17.5 covers it, whereas `Re2jPattern` did not. `Re2jPattern._pattern` is now `final` as 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 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. ### Why `volatile` uniformly rather than case by case Some of these values are safe today without `volatile`, because the cached object happens to have only `final` fields — 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 two `Pattern` implementations here already disagreed on exactly that point. So the fields are made `volatile` uniformly, and the finality argument is used only to judge which of these is urgent — not whether to fix it. ### Also `JavaUtilPattern._pattern` and `Re2jPattern._pattern` are narrowed to `private`. Nothing outside those classes referenced them — both callers go through `getPattern()` — 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. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
