Double-checked locking bugs by kamaci · Pull Request #6662 · apache/druid

kamaci · 2018-11-26T11:29:06Z

Using double-checked locking for the lazy initialization of any other type of primitive or mutable object risks a second thread using an uninitialized or partially initialized member while the first thread is still creating it, and crashing the program.

leventov

Thanks for these findings! The bugs are real, but they should be fixed differently.

leventov · 2018-11-26T15:34:37Z

The former code here is indeed buggy, however putting synchronized on getUnderlyingProvider() is not how this method should be fixed.

Please make the following:

Declare config field final

Add a comment noting that FileSessionCredentialsProvider is safely initialized because it contains a final field sessionCredentials. Also a comment should be added on that field noting that final modifier should not be removed from it, because it has concurrency implications in LazyFileSessionCredentialsProvider.

In getUnderlyingProvider(), provider should be read into a local variable, local variable updated in the synchronized block along with the field, and the local is returned in the last line of the method. This is because currently the second read of provider in the last line of the method could return null even if the first read in the first line returned non-null and the synchronized block was not entered.

@leventov do you mean that?

private final AWSCredentialsConfig config; @Nullable private FileSessionCredentialsProvider provider; public LazyFileSessionCredentialsProvider(AWSCredentialsConfig config) { this.config = config; } //FileSessionCredentialsProvider is safely initialized because it contains a final field sessionCredentials. private synchronized FileSessionCredentialsProvider getUnderlyingProvider() { FileSessionCredentialsProvider syncedProvider = provider; if (syncedProvider == null) { synchronized (config) { syncedProvider = new FileSessionCredentialsProvider(config.getFileSessionCredentials()); provider = syncedProvider; } } return syncedProvider; }

No synchronized on the method

There should be another check inside the synchronized block (because currently it's not double checked locking :)

I would put the comment before the provider = syncedProvider; line:

// The provider field doesn't need to be volatile because FileSessionCredentialsProvider // is safely initialized because it contains a final field sessionCredentials.

Added "The provider field doesn't need to be volatile" because that's what specifically needs to be explained here.

Also for the comment to be accurate, this PR should be merged not earlier than this one: #6664

This post helps make sense of what @leventov is saying: https://shipilev.net/blog/2014/safe-public-construction/. I admit I had to read it before understanding what was meant by the comment "FileSessionCredentialsProvider is safely initialized because it contains a final field sessionCredentials".

@leventov - am I understanding you right - are you suggesting that we should rely on the fact that an object with at least one final field will always be safely initialized, and skip the volatile? i.e. in the terms used in the post linked above, you are suggesting it's best to do "Unsafe Local DCL + Safe Singleton" in this case?

If so, I don't feel super comfortable about that. The reason is that it is easy to screw it up: removing a final field in a different class (the class being initialized) breaks the holder class's correctness. Comments help with this problem, but IMO, it makes life easier to make the provider field volatile, which makes the correctness obvious from just looking at the container class (in this case, LazyFileSessionCredentialsProvider). Limiting the scope of what a maintainer needs to consider to a single file is a good thing, since it makes future maintenance easier and less error-prone.

Separately from that, it is not clear to me that this behavior is in fact guaranteed. Please educate me if I am missing something (I haven't studied the relevant sections of the Java spec) but it seems like the "do a totally safe initialization if any final is written" behavior is just an implementation detail of a particular VM and not something guaranteed. If that is right, then to get guaranteed safe initialization of an entire class, all fields should be final (or volatile).

Long story short, in this (non-perf-critical) case I am advocating for "Safe DCL", meaning the code below. And in performance critical code, "Unsafe Local DCL + Safe Singleton" could make sense, although it's worth verifying. The linked post above is 4 years old and it just did one set of tests, but, in that test that pattern didn't run any faster than "Safe DCL" or "Safe Local DCL" on x86.

private final AWSCredentialsConfig config; @Nullable private volatile FileSessionCredentialsProvider provider; public LazyFileSessionCredentialsProvider(AWSCredentialsConfig config) { this.config = config; } private FileSessionCredentialsProvider getUnderlyingProvider() { if (provider == null) { synchronized (config) { if (provider == null) { provider = new FileSessionCredentialsProvider(config.getFileSessionCredentials()); } } } return provider; }

Also since volatile you don't need that "safely initialized" comment on the field write line, but you need to write a similar comment on the field declaration, saying that the field is declared volatile in order to ensure safe publication of the object without worrying about final modifiers on the fields of the created object.

Here is the implementation with reference to Effective Java, Second Edition Item 71: Use lazy initialization judiciously:

private final AWSCredentialsConfig config; /* The field is declared volatile in order to ensure safe publication of the object without worrying about final modifiers on the fields of the created object */ @Nullable private volatile FileSessionCredentialsProvider provider; public LazyFileSessionCredentialsProvider(AWSCredentialsConfig config) { this.config = config; } //FileSessionCredentialsProvider is safely initialized because it contains a final field sessionCredentials. private FileSessionCredentialsProvider getUnderlyingProvider() { FileSessionCredentialsProvider syncedProvider = provider; if (syncedProvider == null) { synchronized (config) { syncedProvider = provider; if (syncedProvider == null) { syncedProvider = new FileSessionCredentialsProvider(config.getFileSessionCredentials()); provider = syncedProvider; } } } return syncedProvider; }

👍 except the final thing, the comment should be a javadoc comment:

/** * The field is declared volatile in order to ensure safe publication of the object * in {@link #getUnderlyingProvider()} without worrying about final modifiers * on the fields of the created object */

This line should also be removed:

//FileSessionCredentialsProvider is safely initialized because it contains a final field sessionCredentials.

To extend Gian's point from #6662 (comment), there is another reason why I think that double-checked locking without volatile should be prohibited in Druid entirely. When some code is added that bypasses the initialization path, and only checks that the field is initialized, a-la

if (lazilyInitializedField != null) { doSomethingWith(lazilyInitializedField); }

There is a race and possibility of NPE. The second read of the field might result in null, despite the first read returned non-null and the check was passed. The correct code is:

Foo lazilyInitializedField = this.lazilyInitializedField; if (lazilyInitializedField != null) { doSomethingWith(lazilyInitializedField); }

But I think it's unrealistic to expect this level of attention to details and discipline from all developers and reviewers.

leventov · 2018-11-26T15:46:13Z

This class also requires changes, similar to points 2. and 3. LazyFileSessionCredentialsProvider.

For p. 2, please add a comment that the returned compiled script object is an anonymous inner class, therefore it has an implicit final field (a reference to the object of the enclosing class), therefore it is safely initialized without volatile on compiledScript.

For p. 3, checkAndCompileScript() should be made to return the compiledScript object and that object must be used when checkAndCompileScript() is called, instead of re-reading compiledScript.

leventov · 2018-11-26T15:47:08Z

Same as in JavaScriptAggregatorFactory

leventov · 2018-11-26T15:47:19Z

Same as in JavaScriptAggregatorFactory

leventov · 2018-11-26T15:48:12Z

Please add @Nullable to the provider field

leventov · 2018-11-26T15:48:38Z

Please add @Nullable to compiledScript

leventov · 2018-11-26T15:48:50Z

Please add @Nullable to fn

leventov · 2018-11-26T15:49:00Z

Please add @Nullable

leventov · 2018-11-26T15:50:31Z

Same as above

leventov · 2018-11-26T15:50:45Z

Please add @Nullable to predicateFactory.

leventov · 2018-12-03T12:56:59Z

Could you please add dependency to org.checkerframework:checker-qual to the project and annotate this field @MonotonicNonNull?

Still @Nullable

leventov · 2018-12-03T12:59:21Z

@EnsuresNonNull("provider")

leventov · 2018-12-03T13:01:08Z

@MonotonicNonNull

Do you mean: https://checkerframework.org/api/org/checkerframework/checker/nullness/qual/MonotonicNonNull.html This is not included at druid-processing dependencies?

Just for this part, documentation says that:

Use of @MonotonicNonNull on a static field is a code smell: it may indicate poor design. You should consider whether it is possible to make the field a member field that is set in the constructor.

Nullness Annotations

compiledScript is not static

Same problem applies to interface import. Here is a related explanation: https://stackoverflow.com/questions/29097160/why-are-nonnull-annotation-on-fqn-not-allowed On demand static import solves the problem as follows, otherwise it gives that error:

Static Member Qualifying Type may Not be Annotated

@MonotonicNonNull @Nullable private volatile ScriptAggregator compiledScript;

It shouldnt have MonotonicNonNull AND Nullable. Just MonotonicNonNull

leventov · 2018-12-03T13:02:21Z

Please make it to return a value. Rename to getCompiledScript(). @EnsuresNonNull("compiledScript")

leventov · 2018-12-03T13:03:01Z

Same as in other classes

leventov · 2018-12-03T13:03:11Z

All the same comments as in the prev. classes

leventov · 2018-12-03T13:03:24Z

All the same comments as in the prev. classes

leventov · 2018-12-03T20:26:47Z

@kamaci note that you must not force push PR branch: https://github.com/apache/incubator-druid/blob/master/CONTRIBUTING.md#if-your-pull-request-shows-conflicts-with-master

kamaci · 2018-12-03T20:39:52Z

@kamaci note that you must not force push PR branch: https://github.com/apache/incubator-druid/blob/master/CONTRIBUTING.md#if-your-pull-request-shows-conflicts-with-master

👍

…nicNonNull.

clintropolis · 2018-12-03T23:19:36Z

@kamaci note that you must not force push PR branch: https://github.com/apache/incubator-druid/blob/master/CONTRIBUTING.md#if-your-pull-request-shows-conflicts-with-master

I feel like maybe this is a bit less important now since github has added history for force pushes https://blog.github.com/changelog/2018-11-15-force-push-timeline-event/, at least as long as squashing/fixing are not done, unless there are other reasons I'm missing.

leventov · 2018-12-03T23:26:08Z

Still @Nullable

leventov · 2018-12-03T23:27:42Z

+  /**
+   * The field is declared volatile in order to ensure safe publication of the object
+   * in {@link #getUnderlyingProvider()} without worrying about final modifiers
+   * on the fields of the created object


I realised that unlikely that many readers of this code will understand this passage without the reference to the thread. Please add , see https://github.com/apache/incubator-druid/pull/6662#discussion_r237013157 in the end

leventov · 2018-12-03T23:29:10Z

  public Aggregator factorize(final ColumnSelectorFactory columnFactory)
  {
-    checkAndCompileScript();
+    getCompiledScript();


This method should be used instead of compiledScript a few lines below, that was the whole point. Same in other places where getCompiledScript() is called.

leventov · 2018-12-03T23:29:32Z

+  /**
+   * The field is declared volatile in order to ensure safe publication of the object
+   * in {@link #compileScript(String, String, String)} without worrying about final modifiers
+   * on the fields of the created object


, see https://github.com/apache/incubator-druid/pull/6662#discussion_r237013157

leventov · 2018-12-03T23:29:56Z

+  /**
+   * The field is declared volatile in order to ensure safe publication of the object
+   * in {@link #compile(String)} without worrying about final modifiers
+   * on the fields of the created object


, see https://github.com/apache/incubator-druid/pull/6662#discussion_r237013157

leventov · 2018-12-03T23:30:32Z

   * script compilation.
   */
+  @EnsuresNonNull("fn")
  private void checkAndCompileScript()


Please convert to lazy getter and use it as in the previous class.

leventov · 2018-12-03T23:31:12Z

All the same comments as in the prev. classes

leventov · 2018-12-03T23:31:22Z

All the same comments as in the prev. classes

kamaci · 2018-12-04T06:59:04Z

+    Preconditions.checkState(config.isEnabled(), "JavaScript is disabled");
+
+    Function syncedFn = fn;
+    if (syncedFn == null) {


@leventov Seems that here is always null at new case. Do I miss anything?

I don't understand you message.

Seems that first check for syncedFn

if (syncedFn == null)

is unnecessary.

On the other hand, I've just sent another commit to address your comments.

@leventov is everything OK for my PR?

leventov · 2018-12-05T15:59:04Z

  public Aggregator factorize(final ColumnSelectorFactory columnFactory)
  {
-    checkAndCompileScript();
+    compiledScript = getCompiledScript();


It reassigns the field again, that is not needed. Either those lines should be

JavaScriptAggregator.ScriptAggregator compiledScript = getCompiledScript();

Or (IMO) it's simpler just to use getCompiledScript() without introducing a local variable, in methods where it's needed only once.

Same below

leventov · 2018-12-05T15:59:44Z

  public Object compute(Map<String, Object> combinedAggregators)
  {
-    checkAndCompileScript();
+    fn = getCompiledScript();


Same as above, either introduce a local variable or just use getCompiledScript() in the expression, but don't reassign the volatile field.

leventov · 2018-12-05T16:01:14Z

  public String apply(@Nullable Object value)
  {
-    checkAndCompileScript();
+    fn = getCompiledScript();


Same as above.

leventov · 2018-12-05T16:01:35Z

+   * on the fields of the created object
+   */
+  @MonotonicNonNull
+  private volatile Function<Object, String> fn;


Mention https://github.com/apache/incubator-druid/pull/6662#discussion_r237013157

leventov · 2018-12-05T16:02:06Z

+   * The field is declared volatile in order to ensure safe publication of the object
+   * in {@link JavaScriptPredicateFactory(String, ExtractionFn)} without worrying about final modifiers
+   * on the fields of the created object
+   */


Mention https://github.com/apache/incubator-druid/pull/6662#discussion_r237013157

leventov · 2018-12-05T16:02:42Z

  public Filter toFilter()
  {
-    checkAndCreatePredicateFactory();
+    predicateFactory = getPredicateFactory();


Same as above

leventov · 2018-12-06T16:16:56Z

  public BufferAggregator factorizeBuffered(final ColumnSelectorFactory columnSelectorFactory)
  {
-    checkAndCompileScript();
+    compiledScript = getCompiledScript();


Still reassign. Also other occurrences in this PR.

leventov · 2018-12-07T15:14:31Z

@kamaci please merge master into your PR branch to make CI happy

leventov · 2018-12-07T16:10:24Z

However, not needed, CI passed itself

kamaci · 2018-12-07T19:04:12Z

👍

* Double-checked locking bug is fixed. * @nullable is removed since there is no need to use along with @MonotonicNonNull. * Static import is removed. * Lazy initialization is implemented. * Local variables used instead of volatile ones. * Local variables used instead of volatile ones.

kamaci force-pushed the feature-lockfix branch 3 times, most recently from 406caaf to e996a2c Compare November 26, 2018 11:59

leventov requested changes Nov 26, 2018

View reviewed changes

leventov added the Bug label Nov 26, 2018

leventov changed the title ~~Double-checked locking bug is fixed.~~ Double-checked locking bugs Nov 26, 2018

kamaci force-pushed the feature-lockfix branch 3 times, most recently from 2adbacf to a9746d8 Compare December 3, 2018 09:33

leventov reviewed Dec 3, 2018

View reviewed changes

kamaci force-pushed the feature-lockfix branch from 2c7f4fe to 8e9b971 Compare December 3, 2018 20:19

kamaci force-pushed the feature-lockfix branch from 0be7cc1 to de26d3a Compare December 3, 2018 20:34

Double-checked locking bug is fixed.

e36b84b

kamaci force-pushed the feature-lockfix branch from 1d74d03 to e36b84b Compare December 3, 2018 20:37

kamaci added 2 commits December 3, 2018 23:44

@nullable is removed since there is no need to use along with @Monoto…

fb5eaa8

…nicNonNull.

Static import is removed.

14d9689

leventov requested changes Dec 3, 2018

View reviewed changes

kamaci commented Dec 4, 2018

View reviewed changes

Lazy initialization is implemented.

0814d5b

leventov requested changes Dec 5, 2018

View reviewed changes

Local variables used instead of volatile ones.

abbfab5

leventov requested changes Dec 6, 2018

View reviewed changes

Local variables used instead of volatile ones.

9641323

leventov approved these changes Dec 7, 2018

View reviewed changes

leventov merged commit bbb283f into apache:master Dec 7, 2018

jon-wei added this to the 0.14.0 milestone Feb 20, 2019

jon-wei mentioned this pull request Feb 23, 2019

Update LICENSE and NOTICE files #7026

Merged

leventov mentioned this pull request Nov 20, 2019

Fix double-checked locking in predicate suppliers in BoundDimFilter, InDimFilter, SelectorDimFilter #8911

Open

leventov mentioned this pull request Dec 6, 2019

Fix double-checked locking in predicate for SelectorDimFilter #8990

Closed

Conversation

kamaci commented Nov 26, 2018

Uh oh!

leventov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kamaci Nov 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

leventov Nov 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gianm Nov 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kamaci Nov 27, 2018 •

edited

Loading

leventov Nov 27, 2018 •

edited

Loading

gianm Nov 28, 2018 •

edited

Loading