Skip to content
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.

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

CloudStorageFileSystem: ConcurrentModificationException belies thread safety #691

Closed
cjllanwarne opened this issue Sep 8, 2021 · 7 comments · Fixed by #719
Closed

CloudStorageFileSystem: ConcurrentModificationException belies thread safety #691

cjllanwarne opened this issue Sep 8, 2021 · 7 comments · Fixed by #719
Assignees
Labels
api: storage priority: p1 type: bug

Comments

@cjllanwarne
Copy link

@cjllanwarne cjllanwarne commented Sep 8, 2021

We recently updated from an older version of the storage NIO library and are now seeing ConcurrentModificationExceptions from CloudStorageFileSystem. At a guess, it seems like this function is perhaps not perfectly resilient if CONFIG_TO_PROVIDERS_MAP is updated while the existingProviders value is being iterated over?

Caused by: java.util.ConcurrentModificationException: null
	at java.base/java.util.HashMap$HashIterator.nextNode(Unknown Source)
	at java.base/java.util.HashMap$KeyIterator.next(Unknown Source)
	at com.google.cloud.storage.contrib.nio.CloudStorageFileSystem.getCloudStorageFileSystemProvider(CloudStorageFileSystem.java:164)
	at com.google.cloud.storage.contrib.nio.CloudStorageFileSystem.forBucket(CloudStorageFileSystem.java:196)
	at [...our client code]
@product-auto-label product-auto-label bot added the api: storage label Sep 8, 2021
@breilly2
Copy link

@breilly2 breilly2 commented Sep 8, 2021

private static CloudStorageFileSystemProvider getCloudStorageFileSystemProvider(
Looks very problematic:

  private static CloudStorageFileSystemProvider getCloudStorageFileSystemProvider(
      CloudStorageConfiguration config, StorageOptions storageOptions) {
    CloudStorageFileSystemProvider newProvider =
        (storageOptions == null)
            ? new CloudStorageFileSystemProvider(config.userProject())
            : new CloudStorageFileSystemProvider(config.userProject(), storageOptions);
    Set<CloudStorageFileSystemProvider> existingProviders = CONFIG_TO_PROVIDERS_MAP.get(config);
    if (existingProviders == null) {
      existingProviders = new HashSet<>();
    } else {
      for (CloudStorageFileSystemProvider existiningProvider : existingProviders) {
        if (existiningProvider.equals(newProvider)) {
          return existiningProvider;
        }
      }
    }
    existingProviders.add(newProvider);
    CONFIG_TO_PROVIDERS_MAP.put(config, existingProviders);
    return newProvider;
  }
  1. CloudStorageFileSystem is annotated as @ThreadSafe. Perhaps its instances are, but this static function is not. existingProviders is a HashSet which gets modified near the end of the function without any synchronization. Concurrent callers with the same config and storageOptions can run into this ConcurrentModificationException.
  2. The values of existingProviders are CloudStorageFileSystemProvider instances (which are oddly annotated as @Singleton). CloudStorageFileSystemProvider's equals and hashCode are based on the storage property which is modified after object instantiation (from initStorage called from various places). Therefore, once a "cached" instance has been initialized, existingProvider.equals(newProvider) will yield false (since newProvider hasn't yet been initialized).

@andrewsg andrewsg added priority: p1 type: bug labels Sep 9, 2021
@JesseLovelace
Copy link
Contributor

@JesseLovelace JesseLovelace commented Sep 20, 2021

Thanks for the feedback, we're taking a look at this. How often does this issue come up for you?

@aednichols
Copy link

@aednichols aednichols commented Sep 20, 2021

The issue reproduces more or less continuously when our multitenant server app is in production with the affected release.

We also created a unit test that triggers it in a minimal scenario.

@JesseLovelace
Copy link
Contributor

@JesseLovelace JesseLovelace commented Sep 21, 2021

Thanks! We'll have an update on this tomorrow

@aednichols
Copy link

@aednichols aednichols commented Sep 22, 2021

Sounds great, thank you!

BenWhitehead added a commit that referenced this issue Sep 23, 2021
Replace manual attempt at caching via a HashMap with a guava Cache.

Fix #691
Fix #698
@BenWhitehead
Copy link
Contributor

@BenWhitehead BenWhitehead commented Sep 23, 2021

Thank you very much for providing the minimised repo test that helped a lot.

I've proposed a fix in #719 which fixes the thread safety issue, and fixes the actual caching mapping outlined in #698 as well.

BenWhitehead added a commit that referenced this issue Sep 27, 2021
Replace manual attempt at caching via a HashMap with a guava Cache.

Fix #691
Fix #698
@aednichols
Copy link

@aednichols aednichols commented Sep 27, 2021

Excellent, thank you very much! Appreciate the fast turnaround.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: storage priority: p1 type: bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants