Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
+8 −6
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
623b993
Updated the field assignment as final statement in double-checked-loc…
ChaladiMohanVamsi Jun 8, 2025
2891984
gcsfileio: Updated the field assignment as final statement in double-…
ChaladiMohanVamsi Jun 8, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions aws/src/main/java/org/apache/iceberg/aws/s3/S3FileIO.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -434,9 +434,9 @@ private Map<String, PrefixedS3Client> clientByPrefix() {
if (null == clientByPrefix) {
synchronized (this) {
if (null == clientByPrefix) {
this.clientByPrefix = Maps.newHashMap();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@ChaladiMohanVamsi ChaladiMohanVamsi Jun 14, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Map<String, PrefixedS3Client> localClientByPrefix = Maps.newHashMap();

clientByPrefix.put(
localClientByPrefix.put(
ROOT_PREFIX, new PrefixedS3Client(ROOT_PREFIX, properties, s3, s3Async));
storageCredentials.stream()
.filter(c -> c.prefix().startsWith(ROOT_PREFIX))
Expand All @@ -449,11 +449,12 @@ private Map<String, PrefixedS3Client> clientByPrefix() {
.putAll(storageCredential.config())
.buildKeepingLast();

clientByPrefix.put(
localClientByPrefix.put(
storageCredential.prefix(),
new PrefixedS3Client(
storageCredential.prefix(), propertiesWithCredentials, s3, s3Async));
});
this.clientByPrefix = localClientByPrefix;
}
}
}
Expand Down
7 changes: 4 additions & 3 deletions gcp/src/main/java/org/apache/iceberg/gcp/gcs/GCSFileIO.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -193,9 +193,9 @@ private Map<String, PrefixedStorage> storageByPrefix() {
if (null == storageByPrefix) {
synchronized (this) {
if (null == storageByPrefix) {
this.storageByPrefix = Maps.newHashMap();
Map<String, PrefixedStorage> localStorageByPrefix = Maps.newHashMap();

storageByPrefix.put(
localStorageByPrefix.put(
ROOT_STORAGE_PREFIX,
new PrefixedStorage(ROOT_STORAGE_PREFIX, properties, storageSupplier));
storageCredentials.stream()
Expand All @@ -209,13 +209,14 @@ private Map<String, PrefixedStorage> storageByPrefix() {
.putAll(storageCredential.config())
.buildKeepingLast();

storageByPrefix.put(
localStorageByPrefix.put(
storageCredential.prefix(),
new PrefixedStorage(
storageCredential.prefix(),
propertiesWithCredentials,
storageSupplier));
});
this.storageByPrefix = localStorageByPrefix;
}
}
}
Expand Down