ChaladiMohanVamsi commented on code in PR #13276:
URL: https://github.com/apache/iceberg/pull/13276#discussion_r2146959188
##########
aws/src/main/java/org/apache/iceberg/aws/s3/S3FileIO.java:
##########
@@ -434,9 +434,9 @@ private Map<String, PrefixedS3Client> clientByPrefix() {
if (null == clientByPrefix) {
synchronized (this) {
if (null == clientByPrefix) {
- this.clientByPrefix = Maps.newHashMap();
Review Comment:
- The issue is not about the field visibility problem, it is about field
being pre-maturely visible to all other threads before populating all the
required entries in the map.
- Assuming there are 2 threads executing the method.
- First thread entered the synchronized block and initialized empty
hashmap before populating the entries.
- It is possible that second thread don't even reach synchronized block as
the first check `(null == clientByPrefix)` evaluates to false because the first
thread assigned field to an empty hashmap.
- Second thread proceeds its computation consuming the empty hashmap and
could result in error.
- To cover this scenario, we should have field assignment as a last
statement of synchronized block. This ensures the map field is visible to all
other threads only after it is completely populated, until then other threads
will be waiting at synchronized block as expected.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
