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

Storage API: Inappropriate caching and re-use of differently-authenticated filesystem providers #698

Closed
cjllanwarne opened this issue Sep 13, 2021 · 1 comment · Fixed by #719
Assignees
Labels
api: storage priority: p1 type: bug

Comments

@cjllanwarne
Copy link

@cjllanwarne cjllanwarne commented Sep 13, 2021

We recently updated to the latest version of the java-storage-nio library but almost immediately ran into issues with threads handling requests from different users having their filesystem providers and authentication mixed up. This resulted - luckily - in only “access denied” exceptions before we noticed and reverted the update but might have been much worse.

The problem seems to be coming from the changes made in #168 whereby the CloudStorageFileSystem is now caching providers in its CONFIG_TO_PROVIDERS_MAP (see: get, put) but not recognizing that differently authenticated providers might nevertheless be sharing a single value of config.

We believe that this would not be the case if the CONFIG_TO_PROVIDERS_MAP were to be keyed on the combination of config (which might be a single global value, as in our case) AND storageOptions (which contains the authentication information).

Side note: As Brian also mentioned in #691, the HashSets in that file seem to be storing objects whose hashes can mutate (and indeed, would be expected to mutate when their auth state is changed), which might also be causing unexpected behavior on lookup (which might actually have saved us from even more unexpected filesystem provider re-use).

@product-auto-label product-auto-label bot added the api: storage label Sep 13, 2021
@Breathtender Breathtender added priority: p1 type: bug labels Sep 14, 2021
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

I've proposed a fix in #719 which fixes the thread safety issue, and fixes the actual caching mapping outlined here 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
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.

4 participants