leventov commented on a change in pull request #6662: Double-checked locking
bugs
URL: https://github.com/apache/incubator-druid/pull/6662#discussion_r254161160
##########
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:
To extend Gian's point from
https://github.com/apache/incubator-druid/pull/6662#discussion_r237013157,
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
```java
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:
```java
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.
----------------------------------------------------------------
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]
