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 Issues related to the googleapis/java-storage-nio API. priority: p1 Important issue which blocks shipping the next release. Will be fixed prior to next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.

Comments

@cjllanwarne
Copy link

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 Issues related to the googleapis/java-storage-nio API. label Sep 8, 2021
@breilly2
Copy link

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 Important issue which blocks shipping the next release. Will be fixed prior to next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns. labels Sep 9, 2021
@JesseLovelace
Copy link
Contributor

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

@aednichols
Copy link

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

Thanks! We'll have an update on this tomorrow

@aednichols
Copy link

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
Collaborator

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

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 Issues related to the googleapis/java-storage-nio API. priority: p1 Important issue which blocks shipping the next release. Will be fixed prior to next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants