[GitHub] gianm commented on a change in pull request #6662: Double-checked locking bugs

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

gianm commented on a change in pull request #6662: Double-checked locking bugs
URL: https://github.com/apache/incubator-druid/pull/6662#discussion_r237013157
 
 

 ##########
 File path: aws-common/src/main/java/org/apache/druid/common/aws/LazyFileSessionCredentialsProvider.java
 ##########
 @@ -32,14 +32,10 @@ public LazyFileSessionCredentialsProvider(AWSCredentialsConfig config)
     this.config = config;
   }
 
-  private FileSessionCredentialsProvider getUnderlyingProvider()
+  private synchronized FileSessionCredentialsProvider getUnderlyingProvider()
 
 Review comment:
   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.
   
   ```java
   private final AWSCredentialsConfig config;
   
   @Nullable
   private volatile FileSessionCredentialsProvider provider;
   
   public LazyFileSessionCredentialsProvider(AWSCredentialsConfig config)
   {
     this.config = config;
   }
   
   private synchronized FileSessionCredentialsProvider getUnderlyingProvider()
   {
     if (provider == null) {
       synchronized (config) {
         if (provider == null) {
           provider = new FileSessionCredentialsProvider(config.getFileSessionCredentials());
         }
       }
     }
     return provider;
   }
   ```

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

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