[GitHub] [druid] gianm commented on pull request #13049: Synchronize query context getMergedParams()

[GitBox <gi...@apache.org>]

gianm commented on PR #13049:
URL: https://github.com/apache/druid/pull/13049#issuecomment-1240050449

   > This enhancement is a back-door that converts `QueryContext` from immutable to mutable.
   
   QueryContext is definitely mutable even without that: it's got a bunch of methods for adding and removing context parameters. They each call `invalidateMergedParams()`, which sets `mergedParams` to null. It's believable that this is what leads to the NPEs: some thread is calling `getMergedParams()` while some other thread is calling one of the mutator methods. It's also believable it's due to some weirdness due to `mergedParams` not being volatile, and two threads entering `getMergedParams()` at the same time even though there are no mutations going on.
   
   Either way, it feels sketchy to patch over it by synchronizing `invalidateMergedParams()` alone. In particular, there's still two remaining areas where callers need to worry about concurrency safety:
   
   - nothing intrinsic to QueryContext guarantees that mutators like `removeUserParam` can run safely concurrently with `getMergedParams`; it's up to the caller to avoid doing this
   - nothing intrinsic to QueryContext guarantees that there is a happens-before relationship between mutations and calls to getMergedParams; it's up to the caller to ensure this is set up properly
   
   For these reasons the change in the patch feels like a half-measure. QueryContext takes on some responsibility for concurrency safety, where previously it had none. But it still leaves a lot up to the caller. I'd rather either leave it _all_ up to the caller, which means not doing this change, and instead, doing another change where we prevent callers from using the context in such a way; _or_, leave _none_ of it to the caller, which means making QueryContext fully concurrency-safe.


-- 
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: commits-unsubscribe@druid.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org