gianm commented on PR #15552: URL: https://github.com/apache/druid/pull/15552#issuecomment-1881735390
> Did it caused any real trouble (I deeply sorry for the it) - but I would be very interested in learning more about it if it did! I noticed this through reading the code, not through anything that happened in production. But this code hasn't shipped in a release yet, so probably not many people are running it in production yet. As to whether it's a real issue I would refer to: https://shipilev.net/blog/2016/close-encounters-of-jmm-kind/#wishful-benign-is-resilient There are now a bunch of races in various `ExprEval` implementations, so the question is are the races "benign" or not? Personally I try to avoid answering this question, and prefer to design things such that the races go away— by making objects thread-safe, or truly immutable, or by using them from only one thread. As an example, I do suspect the `asString()` has a non-benign race: ``` @Nullable public String asString() { if (!stringValueCached) { stringValue = Evals.asString(value); stringValueCached = true; } return stringValue; } ``` I think it's possible for thread 1 to write `stringValue = Evals.asString(value)` and `stringValueCached = true` in such a way that thread 2 reads them out-of-order, i.e., thread 2 sees `true` for `stringValueCached` but `null` for `stringValue`. (It breaks the "single read rule".) Anyway, IMO even benign races are best avoided— since they are easy to mess up— so even if it was benign, I would still suggest changing the design of the class. -- 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]
