Fix double-checked locking in predicate for SelectorDimFilter

[qutang1]

Fixes (partially) #8911.

Fixed the bug:

Double-checked locking bugs

[qutang1]

@leventov ready for review

[leventov]

The semantic is changed: now predicates are not lazy. Here, you cannot use memoize() as in other classes - you should more scrupulously follow the "golden" examples forged in #6662.

[qutang1]

Hi Leventov, I am not sure if I understand this case correctly : basically the original code is saying I want 3 private predicates only be initialized once when SelectorDimFilter::toFilter() method has been called + DruidPredicateFactory::makeDoublePredicate() has been called, is it correct? My question is since DruidPredicateFactory is final(already thread safe), the whole purpose of the old logic is just not init the predicate twice ,there's no thread safety issue has been involved, all I need to do is change 3 predicate fields to final?
` @OverRide
public Filter toFilter()
{
if (extractionFn == null) {
return new SelectorFilter(dimension, value, filterTuning);
} else {

  final DruidPredicateFactory predicateFactory = new DruidPredicateFactory()
  {
    @Override
    public Predicate<String> makeStringPredicate()
    {
      return Predicates.equalTo(value);
    }

    @Override
    public DruidLongPredicate makeLongPredicate()
    {
      initLongPredicate();
      return longPredicate;
    }

    @Override
    public DruidFloatPredicate makeFloatPredicate()
    {
      initFloatPredicate();
      return floatPredicate;
    }

    @Override
    public DruidDoublePredicate makeDoublePredicate()
    {
      initDoublePredicate();
      return druidDoublePredicate;
    }
  };
  return new DimensionPredicateFilter(dimension, predicateFactory, extractionFn, filterTuning);
}

}
`

[leventov]

DruidPredicateFactory is final(already thread safe)

DruidPredicateFactory is not thread safe, as far as I can tell.

You cannot initialize predicates eagerly since it may not be possible: e. g. if the value is "1.0", it will not be parseable as long in createLongPredicate(). The method will not throw because it uses tryParseLong(), but nevertheless this is fragile.

The problem with the current code is that it allows spurious null returns of predicates due to double-read of a non-volatile field, as described here, see here for more theoretical foundations.

You can fix this by either applying proper double-checked locking, or using racy single check initialization.

BTW, also makes sense to cache makeStringPredicate().

[stale]

This pull request has been marked as stale due to 60 days of inactivity. It will be closed in 4 weeks if no further activity occurs. If you think that's incorrect or this pull request should instead be reviewed, please simply write any comment. Even if closed, you can still revive the PR at any time or discuss it on the dev@druid.apache.org list. Thank you for your contributions.

[stale]

This pull request/issue has been closed due to lack of activity. If you think that is incorrect, or the pull request requires review, you can revive the PR at any time.