Skip to content

Commit 78bcab9

Browse files
BenWhiteheadBenWhitehead
authored
fix: make CloudStorageFileSystem#forBucket thread safe (#719)
Replace manual attempt at caching via a HashMap with a guava Cache. Fix #691 Fix #698
1 parent b64b1b7 commit 78bcab9

File tree

4 files changed

+154
-35
lines changed

4 files changed

+154
-35
lines changed

‎java-storage-nio/google-cloud-nio/pom.xml‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -71,6 +71,12 @@
7171
<artifactId>truth</artifactId>
7272
<scope>test</scope>
7373
</dependency>
74+
<dependency>
75+
<groupId>com.google.auth</groupId>
76+
<artifactId>google-auth-library-credentials</artifactId>
77+
<scope>test</scope>
78+
</dependency>
79+
7480
<dependency>
7581
<groupId>org.mockito</groupId>
7682
<artifactId>mockito-core</artifactId>

‎java-storage-nio/google-cloud-nio/src/main/java/com/google/cloud/storage/contrib/nio/CloudStorageFileSystem.java‎

Lines changed: 78 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -18,14 +18,19 @@
1818

1919
import static com.google.common.base.Preconditions.checkArgument;
2020
import static com.google.common.base.Preconditions.checkNotNull;
21+
import static java.util.Objects.requireNonNull;
2122

2223
import com.google.api.gax.paging.Page;
2324
import com.google.cloud.storage.Bucket;
2425
import com.google.cloud.storage.Storage;
2526
import com.google.cloud.storage.StorageException;
2627
import com.google.cloud.storage.StorageOptions;
2728
import com.google.common.base.Strings;
29+
import com.google.common.cache.CacheBuilder;
30+
import com.google.common.cache.CacheLoader;
31+
import com.google.common.cache.LoadingCache;
2832
import com.google.common.collect.ImmutableSet;
33+
import com.google.common.util.concurrent.UncheckedExecutionException;
2934
import java.io.IOException;
3035
import java.net.URI;
3136
import java.net.URISyntaxException;
@@ -38,10 +43,9 @@
3843
import java.nio.file.attribute.FileTime;
3944
import java.nio.file.attribute.UserPrincipalLookupService;
4045
import java.util.HashMap;
41-
import java.util.HashSet;
42-
import java.util.Map;
4346
import java.util.Objects;
4447
import java.util.Set;
48+
import java.util.concurrent.ExecutionException;
4549
import javax.annotation.CheckReturnValue;
4650
import javax.annotation.Nullable;
4751
import javax.annotation.concurrent.ThreadSafe;
@@ -66,8 +70,21 @@ public final class CloudStorageFileSystem extends FileSystem {
6670
private final CloudStorageFileSystemProvider provider;
6771
private final String bucket;
6872
private final CloudStorageConfiguration config;
69-
private static final Map<CloudStorageConfiguration, Set<CloudStorageFileSystemProvider>>
70-
CONFIG_TO_PROVIDERS_MAP = new HashMap<>();
73+
private static final LoadingCache<ProviderCacheKey, CloudStorageFileSystemProvider>
74+
PROVIDER_CACHE_BY_CONFIG =
75+
CacheBuilder.newBuilder()
76+
.build(
77+
new CacheLoader<ProviderCacheKey, CloudStorageFileSystemProvider>() {
78+
@Override
79+
public CloudStorageFileSystemProvider load(ProviderCacheKey key) {
80+
CloudStorageConfiguration config = key.cloudStorageConfiguration;
81+
StorageOptions storageOptions = key.storageOptions;
82+
String userProject = config.userProject();
83+
return (storageOptions == null)
84+
? new CloudStorageFileSystemProvider(userProject)
85+
: new CloudStorageFileSystemProvider(userProject, storageOptions);
86+
}
87+
});
7188

7289
// Users can change this: then this affects every filesystem object created
7390
// later, including via SPI. This is meant to be done only once, at the beginning
@@ -144,32 +161,7 @@ public static CloudStorageFileSystem forBucket(String bucket) {
144161
*/
145162
@CheckReturnValue
146163
public static CloudStorageFileSystem forBucket(String bucket, CloudStorageConfiguration config) {
147-
checkArgument(
148-
!bucket.startsWith(URI_SCHEME + ":"), "Bucket name must not have schema: %s", bucket);
149-
checkNotNull(config);
150-
return new CloudStorageFileSystem(
151-
getCloudStorageFileSystemProvider(config, null), bucket, config);
152-
}
153-
154-
private static CloudStorageFileSystemProvider getCloudStorageFileSystemProvider(
155-
CloudStorageConfiguration config, StorageOptions storageOptions) {
156-
CloudStorageFileSystemProvider newProvider =
157-
(storageOptions == null)
158-
? new CloudStorageFileSystemProvider(config.userProject())
159-
: new CloudStorageFileSystemProvider(config.userProject(), storageOptions);
160-
Set<CloudStorageFileSystemProvider> existingProviders = CONFIG_TO_PROVIDERS_MAP.get(config);
161-
if (existingProviders == null) {
162-
existingProviders = new HashSet<>();
163-
} else {
164-
for (CloudStorageFileSystemProvider existiningProvider : existingProviders) {
165-
if (existiningProvider.equals(newProvider)) {
166-
return existiningProvider;
167-
}
168-
}
169-
}
170-
existingProviders.add(newProvider);
171-
CONFIG_TO_PROVIDERS_MAP.put(config, existingProviders);
172-
return newProvider;
164+
return forBucket(bucket, config, null);
173165
}
174166

175167
/**
@@ -192,8 +184,16 @@ public static CloudStorageFileSystem forBucket(
192184
String bucket, CloudStorageConfiguration config, @Nullable StorageOptions storageOptions) {
193185
checkArgument(
194186
!bucket.startsWith(URI_SCHEME + ":"), "Bucket name must not have schema: %s", bucket);
195-
return new CloudStorageFileSystem(
196-
getCloudStorageFileSystemProvider(config, storageOptions), bucket, checkNotNull(config));
187+
checkNotNull(config);
188+
CloudStorageFileSystemProvider result;
189+
ProviderCacheKey providerCacheKey = new ProviderCacheKey(config, storageOptions);
190+
try {
191+
result = PROVIDER_CACHE_BY_CONFIG.get(providerCacheKey);
192+
} catch (ExecutionException | UncheckedExecutionException e) {
193+
throw new IllegalStateException(
194+
"Unable to resolve CloudStorageFileSystemProvider for the provided configuration", e);
195+
}
196+
return new CloudStorageFileSystem(result, bucket, config);
197197
}
198198

199199
CloudStorageFileSystem(
@@ -335,4 +335,50 @@ public String toString() {
335335
throw new AssertionError(e);
336336
}
337337
}
338+
339+
/**
340+
* In order to cache a {@link CloudStorageFileSystemProvider} we track the config used to
341+
* instantiate that provider. This class creates an immutable key encapsulating the config to
342+
* allow reliable resolution from the cache.
343+
*/
344+
private static final class ProviderCacheKey {
345+
private final CloudStorageConfiguration cloudStorageConfiguration;
346+
@Nullable private final StorageOptions storageOptions;
347+
348+
public ProviderCacheKey(
349+
CloudStorageConfiguration cloudStorageConfiguration,
350+
@Nullable StorageOptions storageOptions) {
351+
this.cloudStorageConfiguration =
352+
requireNonNull(cloudStorageConfiguration, "cloudStorageConfiguration must be non null");
353+
this.storageOptions = storageOptions;
354+
}
355+
356+
@Override
357+
public boolean equals(Object o) {
358+
if (this == o) {
359+
return true;
360+
}
361+
if (!(o instanceof ProviderCacheKey)) {
362+
return false;
363+
}
364+
ProviderCacheKey that = (ProviderCacheKey) o;
365+
return cloudStorageConfiguration.equals(that.cloudStorageConfiguration)
366+
&& Objects.equals(storageOptions, that.storageOptions);
367+
}
368+
369+
@Override
370+
public int hashCode() {
371+
return Objects.hash(cloudStorageConfiguration, storageOptions);
372+
}
373+
374+
@Override
375+
public String toString() {
376+
return "ConfigTuple{"
377+
+ "cloudStorageConfiguration="
378+
+ cloudStorageConfiguration
379+
+ ", storageOptions="
380+
+ storageOptions
381+
+ '}';
382+
}
383+
}
338384
}

‎java-storage-nio/google-cloud-nio/src/test/java/com/google/cloud/storage/contrib/nio/CloudStorageFileSystemTest.java‎

Lines changed: 59 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,12 +23,16 @@
2323
import static org.junit.Assert.assertNotSame;
2424
import static org.junit.Assert.assertSame;
2525

26+
import com.google.api.gax.rpc.internal.QuotaProjectIdHidingCredentials;
27+
import com.google.auth.Credentials;
28+
import com.google.cloud.NoCredentials;
2629
import com.google.cloud.storage.StorageOptions;
2730
import com.google.cloud.storage.contrib.nio.testing.LocalStorageHelper;
2831
import com.google.cloud.testing.junit4.MultipleAttemptsRule;
2932
import com.google.common.testing.EqualsTester;
3033
import com.google.common.testing.NullPointerTester;
3134
import java.io.IOException;
35+
import java.lang.reflect.Field;
3236
import java.net.URI;
3337
import java.nio.channels.SeekableByteChannel;
3438
import java.nio.file.FileSystem;
@@ -374,6 +378,46 @@ public void testDifferentProvider() throws IOException {
374378
assertEquals("new-bucket", destFileSystem.bucket());
375379
}
376380
}
381+
382+
// port of test from
383+
// https://github.com/broadinstitute/cromwell/pull/6491/files#diff-758dbbe823e71cc26fee7bc89cd5c434dfb76e604d51005b8327db59aab96068R300-R336
384+
@Test
385+
public void ensureMultipleInstancesDoNotCorruptCredentials() throws Exception {
386+
387+
CloudStorageConfiguration config =
388+
CloudStorageConfiguration.builder()
389+
.permitEmptyPathComponents(true)
390+
.stripPrefixSlash(true)
391+
.usePseudoDirectories(true)
392+
.build();
393+
394+
Credentials noCredentials = NoCredentials.getInstance();
395+
Credentials saCredentials = new QuotaProjectIdHidingCredentials(noCredentials);
396+
397+
StorageOptions noOptions =
398+
StorageOptions.newBuilder()
399+
.setProjectId("public-project")
400+
.setCredentials(noCredentials)
401+
.build();
402+
403+
StorageOptions saOptions =
404+
StorageOptions.newBuilder()
405+
.setProjectId("private-project")
406+
.setCredentials(saCredentials)
407+
.build();
408+
409+
CloudStorageFileSystem noFs =
410+
CloudStorageFileSystem.forBucket("public-bucket", config, noOptions);
411+
CloudStorageFileSystem saFs =
412+
CloudStorageFileSystem.forBucket("private-bucket", config, saOptions);
413+
414+
CloudStoragePath noPath = noFs.getPath("public-file");
415+
CloudStoragePath saPath = saFs.getPath("private-file");
416+
417+
assertThat(credentialsForPath(noPath)).isEqualTo(noCredentials);
418+
assertThat(credentialsForPath(saPath)).isEqualTo(saCredentials);
419+
}
420+
377421
/**
378422
* Delete the given directory and all of its contents if non-empty.
379423
*
@@ -402,4 +446,19 @@ public FileVisitResult postVisitDirectory(Path dir, IOException exc) throws IOEx
402446
private void assertMatches(FileSystem fs, PathMatcher matcher, String toMatch, boolean expected) {
403447
assertThat(matcher.matches(fs.getPath(toMatch).getFileName())).isEqualTo(expected);
404448
}
449+
450+
private static Credentials credentialsForPath(Path p)
451+
throws NoSuchFieldException, IllegalAccessException {
452+
CloudStorageFileSystemProvider cloudFilesystemProvider =
453+
(CloudStorageFileSystemProvider) p.getFileSystem().provider();
454+
Field storageOptionsField =
455+
cloudFilesystemProvider.getClass().getDeclaredField("storageOptions");
456+
storageOptionsField.setAccessible(true);
457+
StorageOptions storageOptions =
458+
(StorageOptions) storageOptionsField.get(cloudFilesystemProvider);
459+
Field credentialsField =
460+
storageOptions.getClass().getSuperclass().getDeclaredField("credentials");
461+
credentialsField.setAccessible(true);
462+
return (Credentials) credentialsField.get(storageOptions);
463+
}
405464
}

‎java-storage-nio/google-cloud-nio/src/test/java/com/google/cloud/storage/contrib/nio/CloudStorageIsDirectoryTest.java‎

Lines changed: 11 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,7 @@
3232
import org.junit.Before;
3333
import org.junit.Rule;
3434
import org.junit.Test;
35+
import org.junit.rules.TestName;
3536
import org.junit.runner.RunWith;
3637
import org.junit.runners.JUnit4;
3738

@@ -40,12 +41,17 @@
4041
public class CloudStorageIsDirectoryTest {
4142
@Rule public final MultipleAttemptsRule multipleAttemptsRule = new MultipleAttemptsRule(3);
4243

44+
@Rule public final TestName testName = new TestName();
45+
4346
private StorageOptions mockOptions;
4447
private Storage mockStorage;
4548

4649
@Before
4750
public void before() {
48-
mockOptions = mock(StorageOptions.class);
51+
mockOptions =
52+
mock(
53+
StorageOptions.class,
54+
String.format("storage-options-mock_%s", testName.getMethodName()));
4955
mockStorage = mock(Storage.class);
5056
when(mockOptions.getService()).thenReturn(mockStorage);
5157
CloudStorageFileSystemProvider.setStorageOptions(mockOptions);
@@ -54,7 +60,7 @@ public void before() {
5460
@Test
5561
public void testIsDirectoryNoUserProject() {
5662
CloudStorageFileSystem fs =
57-
CloudStorageFileSystem.forBucket("bucket", CloudStorageConfiguration.DEFAULT);
63+
CloudStorageFileSystem.forBucket("bucket", CloudStorageConfiguration.DEFAULT, mockOptions);
5864
when(mockStorage.get(BlobId.of("bucket", "test", null)))
5965
.thenThrow(new IllegalArgumentException());
6066
Page<Blob> pages = mock(Page.class);
@@ -74,7 +80,9 @@ public void testIsDirectoryNoUserProject() {
7480
public void testIsDirectoryWithUserProject() {
7581
CloudStorageFileSystem fs =
7682
CloudStorageFileSystem.forBucket(
77-
"bucket", CloudStorageConfiguration.builder().userProject("project-id").build());
83+
"bucket",
84+
CloudStorageConfiguration.builder().userProject("project-id").build(),
85+
mockOptions);
7886
when(mockStorage.get(BlobId.of("bucket", "test", null)))
7987
.thenThrow(new IllegalArgumentException());
8088
Page<Blob> pages = mock(Page.class);

0 commit comments

Comments
 (0)