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 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:
[email protected]
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
