Skip to content

Fix double-checked locking in predicate for SelectorDimFilter #8990

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
qutang1 wants to merge 2 commits into from
Closed

Fix double-checked locking in predicate for SelectorDimFilter #8990

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0c8e9ea
Fix double-checked locking in predicate for SelectorDimFilter
qutang1 Dec 4, 2019
96cb38c
Remove unused declaration
qutang1 Dec 4, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
122 changes: 57 additions & 65 deletions processing/src/main/java/org/apache/druid/query/filter/SelectorDimFilter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,8 @@
import com.google.common.base.Preconditions;
import com.google.common.base.Predicate;
import com.google.common.base.Predicates;
import com.google.common.base.Supplier;
import com.google.common.base.Suppliers;
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Range;
import com.google.common.collect.RangeSet;
Expand Down Expand Up @@ -56,11 +58,11 @@ public class SelectorDimFilter implements DimFilter
@Nullable
private final FilterTuning filterTuning;

private final Object initLock = new Object();

private DruidLongPredicate longPredicate;
private DruidFloatPredicate floatPredicate;
private DruidDoublePredicate druidDoublePredicate;
private volatile DruidLongPredicate longPredicate;
private volatile DruidFloatPredicate floatPredicate;
private volatile DruidDoublePredicate druidDoublePredicate;



@JsonCreator
public SelectorDimFilter(
Expand All @@ -71,7 +73,9 @@ public SelectorDimFilter(
)
{
Preconditions.checkArgument(dimension != null, "dimension must not be null");

this.longPredicate = makeLongPredicateSupplier().get();
Copy link
Copy Markdown
Member

@leventov leventov Dec 6, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

@qutang1 qutang1 Dec 7, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);
}

}
`

Copy link
Copy Markdown
Member

@leventov leventov Dec 7, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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().

this.floatPredicate = makeFloatPredicateSupplier().get();
this.druidDoublePredicate = makeDoublePredicateSupplier().get();
this.dimension = dimension;
this.value = NullHandling.emptyToNullIfNeeded(value);
this.extractionFn = extractionFn;
Expand Down Expand Up @@ -121,21 +125,18 @@ public Predicate<String> makeStringPredicate()
@Override
public DruidLongPredicate makeLongPredicate()
{
initLongPredicate();
return longPredicate;
}

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

@Override
public DruidDoublePredicate makeDoublePredicate()
{
initDoublePredicate();
return druidDoublePredicate;
}
};
Expand Down Expand Up @@ -227,76 +228,67 @@ public Set<String> getRequiredColumns()
}


private void initLongPredicate()
private DruidLongPredicate createLongPredicate()
{
if (longPredicate != null) {
return;
if (value == null) {
return DruidLongPredicate.MATCH_NULL_ONLY;
}
synchronized (initLock) {
if (longPredicate != null) {
return;
}
if (value == null) {
longPredicate = DruidLongPredicate.MATCH_NULL_ONLY;
return;
}
final Long valueAsLong = GuavaUtils.tryParseLong(value);
if (valueAsLong == null) {
longPredicate = DruidLongPredicate.ALWAYS_FALSE;
} else {
// store the primitive, so we don't unbox for every comparison
final long unboxedLong = valueAsLong.longValue();
longPredicate = input -> input == unboxedLong;
}
final Long valueAsLong = GuavaUtils.tryParseLong(value);
if (valueAsLong == null) {
return DruidLongPredicate.ALWAYS_FALSE;
} else {
// store the primitive, so we don't unbox for every comparison
final long unboxedLong = valueAsLong.longValue();
return input -> input == unboxedLong;
}
}

private Supplier<DruidLongPredicate> makeLongPredicateSupplier()
{
Supplier<DruidLongPredicate> longPredicate = () -> createLongPredicate();
return Suppliers.memoize(longPredicate);
}

private void initFloatPredicate()
private DruidFloatPredicate createFloatPredicate()
{
if (floatPredicate != null) {
return;
if (value == null) {
return DruidFloatPredicate.MATCH_NULL_ONLY;
}
synchronized (initLock) {
if (floatPredicate != null) {
return;
}

if (value == null) {
floatPredicate = DruidFloatPredicate.MATCH_NULL_ONLY;
return;
}
final Float valueAsFloat = Floats.tryParse(value);
final Float valueAsFloat = Floats.tryParse(value);

if (valueAsFloat == null) {
floatPredicate = DruidFloatPredicate.ALWAYS_FALSE;
} else {
final int floatBits = Float.floatToIntBits(valueAsFloat);
floatPredicate = input -> Float.floatToIntBits(input) == floatBits;
}
if (valueAsFloat == null) {
return DruidFloatPredicate.ALWAYS_FALSE;
} else {
final int floatBits = Float.floatToIntBits(valueAsFloat);
return input -> Float.floatToIntBits(input) == floatBits;
}
}

private void initDoublePredicate()
private Supplier<DruidFloatPredicate> makeFloatPredicateSupplier()
{
Supplier<DruidFloatPredicate> floatPredicate = () -> createFloatPredicate();
return Suppliers.memoize(floatPredicate);
}


private DruidDoublePredicate createDoublePredicate()
{
if (druidDoublePredicate != null) {
return;
if (value == null) {
return DruidDoublePredicate.MATCH_NULL_ONLY;
}
synchronized (initLock) {
if (druidDoublePredicate != null) {
return;
}
if (value == null) {
druidDoublePredicate = DruidDoublePredicate.MATCH_NULL_ONLY;
return;
}
final Double aDouble = Doubles.tryParse(value);
final Double aDouble = Doubles.tryParse(value);

if (aDouble == null) {
druidDoublePredicate = DruidDoublePredicate.ALWAYS_FALSE;
} else {
final long bits = Double.doubleToLongBits(aDouble);
druidDoublePredicate = input -> Double.doubleToLongBits(input) == bits;
}
if (aDouble == null) {
return DruidDoublePredicate.ALWAYS_FALSE;
} else {
final long bits = Double.doubleToLongBits(aDouble);
return input -> Double.doubleToLongBits(input) == bits;
}
}

private Supplier<DruidDoublePredicate> makeDoublePredicateSupplier()
{
Supplier<DruidDoublePredicate> doublePredicate = () -> createDoublePredicate();
return Suppliers.memoize(doublePredicate);
}
}