AWS, GCP: Fix double-checked-locking pattern in S3FileIO, GCSFileIO.

[ChaladiMohanVamsi]

Fix: double-checked-locking pattern in S3FileIO, GCSFileIO.

Ensure Thread-Safe Initialization of clientByPrefix and storageByPrefix

This PR addresses a thread-safety issue during the lazy initialization of the clientByPrefix map. Previously, the shared map was being assigned before it was fully populated, which could lead to race conditions and visibility issues in a concurrent environment.

Changes:

• Introduced a local variable localClientByPrefix inside the synchronized block.
• Populated this local map with all required entries.
• Only after full initialization, assigned it to the shared clientByPrefix field.

Benefits:

• Prevents other threads from observing a partially initialized map.
• Eliminates the risk of NullPointerException due to premature assignment.
• Aligns with best practices for safe publication in multi-threaded contexts.

- this.clientByPrefix = Maps.newHashMap();
+ Map<String, PrefixedS3Client> localClientByPrefix = Maps.newHashMap();
...
+ this.clientByPrefix = localClientByPrefix;

[amogh-jahagirdar]

I'm not sure I follow the issue on how there can be visibility issues since the field is volatile and is assigned in a synchronized block?

[ChaladiMohanVamsi]

• 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.

[amogh-jahagirdar]

Thanks for the detailed explanation, yeah this makes sense now. It's possible that there are 2 threads executing clientsByPrefix(); currently, the first thread will make clientsByPrefix non-null and the second thread will incorrectly think everything is populated even though it isn't. I agree with the fix that we should only make clientsByPrefix non-null at the end of population.

[amogh-jahagirdar]

Thanks for the detailed explanation, yeah this makes sense now. It's possible that there are 2 threads executing clientsByPrefix(); currently, the first thread will make clientsByPrefix non-null and the second thread will incorrectly think everything is populated even though it isn't. I agree with the fix that we should only make clientsByPrefix non-null at the end of population.

[amogh-jahagirdar]

Thanks @ChaladiMohanVamsi!