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.

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

Jump to bottom

fix: make CloudStorageFileSystem#forBucket thread safe #719

Merged
merged 6 commits into from Sep 27, 2021
Merged

fix: make CloudStorageFileSystem#forBucket thread safe #719

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
8c30cb3
fix: make CloudStorageFileSystem#forBucket thread safe
BenWhitehead Sep 23, 2021
fc2903b
fix: replace usage of ServiceAccountCredentials with something unit t…
BenWhitehead Sep 23, 2021
878ed29
chore: leave singleton annotation
BenWhitehead Sep 23, 2021
a8343d6
fix: deps declaration
BenWhitehead Sep 23, 2021
431319f
fix: review feedback
BenWhitehead Sep 24, 2021
25c3df5
chore: move test to appropriate class
BenWhitehead Sep 24, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 6 additions & 0 deletions google-cloud-nio/pom.xml
View file
Open in desktop
Expand Up @@ -71,6 +71,12 @@
<artifactId>truth</artifactId>
<scope>test</scope>
</dependency>
<dependency>
<groupId>com.google.auth</groupId>
<artifactId>google-auth-library-credentials</artifactId>
<scope>test</scope>
</dependency>

<dependency>
<groupId>org.mockito</groupId>
<artifactId>mockito-core</artifactId>
Expand Down
105 changes: 73 additions & 32 deletions ...-cloud-nio/src/main/java/com/google/cloud/storage/contrib/nio/CloudStorageFileSystem.java
View file
Open in desktop
Expand Up @@ -18,14 +18,19 @@

import static com.google.common.base.Preconditions.checkArgument;
import static com.google.common.base.Preconditions.checkNotNull;
import static java.util.Objects.requireNonNull;

import com.google.api.gax.paging.Page;
import com.google.cloud.storage.Bucket;
import com.google.cloud.storage.Storage;
import com.google.cloud.storage.StorageException;
import com.google.cloud.storage.StorageOptions;
import com.google.common.base.Strings;
import com.google.common.cache.CacheBuilder;
import com.google.common.cache.CacheLoader;
import com.google.common.cache.LoadingCache;
import com.google.common.collect.ImmutableSet;
import com.google.common.util.concurrent.UncheckedExecutionException;
import java.io.IOException;
import java.net.URI;
import java.net.URISyntaxException;
Expand All @@ -38,10 +43,9 @@
import java.nio.file.attribute.FileTime;
import java.nio.file.attribute.UserPrincipalLookupService;
import java.util.HashMap;
import java.util.HashSet;
import java.util.Map;
import java.util.Objects;
import java.util.Set;
import java.util.concurrent.ExecutionException;
import javax.annotation.CheckReturnValue;
import javax.annotation.Nullable;
import javax.annotation.concurrent.ThreadSafe;
Expand All @@ -66,8 +70,21 @@ public final class CloudStorageFileSystem extends FileSystem {
private final CloudStorageFileSystemProvider provider;
private final String bucket;
private final CloudStorageConfiguration config;
private static final Map<CloudStorageConfiguration, Set<CloudStorageFileSystemProvider>>
CONFIG_TO_PROVIDERS_MAP = new HashMap<>();
private static final LoadingCache<ConfigTuple, CloudStorageFileSystemProvider>
PROVIDER_CACHE_BY_CONFIG =
CacheBuilder.newBuilder()
.build(
new CacheLoader<ConfigTuple, CloudStorageFileSystemProvider>() {
@Override
public CloudStorageFileSystemProvider load(ConfigTuple key) {
CloudStorageConfiguration config = key.cloudStorageConfiguration;
StorageOptions storageOptions = key.storageOptions;
String userProject = config.userProject();
return (storageOptions == null)
? new CloudStorageFileSystemProvider(userProject)
: new CloudStorageFileSystemProvider(userProject, storageOptions);
}
});

// Users can change this: then this affects every filesystem object created
// later, including via SPI. This is meant to be done only once, at the beginning
Expand Down Expand Up @@ -144,32 +161,7 @@ public static CloudStorageFileSystem forBucket(String bucket) {
*/
@CheckReturnValue
public static CloudStorageFileSystem forBucket(String bucket, CloudStorageConfiguration config) {
checkArgument(
!bucket.startsWith(URI_SCHEME + ":"), "Bucket name must not have schema: %s", bucket);
checkNotNull(config);
return new CloudStorageFileSystem(
getCloudStorageFileSystemProvider(config, null), bucket, config);
}

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;
return forBucket(bucket, config, null);
}

/**
Expand All @@ -192,8 +184,16 @@ public static CloudStorageFileSystem forBucket(
String bucket, CloudStorageConfiguration config, @Nullable StorageOptions storageOptions) {
checkArgument(
!bucket.startsWith(URI_SCHEME + ":"), "Bucket name must not have schema: %s", bucket);
return new CloudStorageFileSystem(
getCloudStorageFileSystemProvider(config, storageOptions), bucket, checkNotNull(config));
checkNotNull(config);
CloudStorageFileSystemProvider result;
ConfigTuple configTuple = new ConfigTuple(config, storageOptions);
try {
result = PROVIDER_CACHE_BY_CONFIG.get(configTuple);
} catch (ExecutionException | UncheckedExecutionException e) {
throw new IllegalStateException(
"Unable to resolve CloudStorageFileSystemProvider for the provided configuration", e);
}
return new CloudStorageFileSystem(result, bucket, checkNotNull(config));
BenWhitehead marked this conversation as resolved.
Show resolved Hide resolved
}

CloudStorageFileSystem(
Expand Down Expand Up @@ -335,4 +335,45 @@ public String toString() {
throw new AssertionError(e);
}
}

private static final class ConfigTuple {
BenWhitehead marked this conversation as resolved.
Show resolved Hide resolved
private final CloudStorageConfiguration cloudStorageConfiguration;
@Nullable private final StorageOptions storageOptions;

public ConfigTuple(
CloudStorageConfiguration cloudStorageConfiguration,
@Nullable StorageOptions storageOptions) {
this.cloudStorageConfiguration =
requireNonNull(cloudStorageConfiguration, "cloudStorageConfiguration must be non null");
this.storageOptions = storageOptions;
}

@Override
public boolean equals(Object o) {
if (this == o) {
return true;
}
if (!(o instanceof ConfigTuple)) {
return false;
}
ConfigTuple that = (ConfigTuple) o;
return cloudStorageConfiguration.equals(that.cloudStorageConfiguration)
&& Objects.equals(storageOptions, that.storageOptions);
}

@Override
public int hashCode() {
return Objects.hash(cloudStorageConfiguration, storageOptions);
}
Comment on lines +369 to +372
Copy link

Choose a reason for hiding this comment

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

These overrides of equals and hashCode remind me of Brian's comment (2) in #691 (comment) - is this now ok here because we're no longer looking up via HashMap (in which case is the override still necessary?) Or are we now waiting until after initialization before caching? Or is Brian's comment still potentially a problem?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The .equals from that comment was comparing instances of CloudStorageFileSystemProvider which aren't immutable. Here we're holding on to CloudStorageConfig which is immutable via AutoValue and a StorageOptions which has a stable hashCode & equals which don't include transient state. So in this case, we're only keying off the config none of the running state.

Copy link

Choose a reason for hiding this comment

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

Great, thanks! Looks good to me!


@Override
public String toString() {
return "ConfigTuple{"
+ "cloudStorageConfiguration="
+ cloudStorageConfiguration
+ ", storageOptions="
+ storageOptions
+ '}';
}
}
}
59 changes: 59 additions & 0 deletions ...rc/test/java/com/google/cloud/storage/contrib/nio/CloudStorageFileSystemProviderTest.java
View file
Open in desktop
Expand Up @@ -27,13 +27,18 @@
import static java.nio.file.StandardOpenOption.TRUNCATE_EXISTING;
import static java.nio.file.StandardOpenOption.WRITE;

import com.google.api.gax.rpc.internal.QuotaProjectIdHidingCredentials;
import com.google.auth.Credentials;
import com.google.cloud.NoCredentials;
import com.google.cloud.storage.StorageOptions;
import com.google.cloud.storage.contrib.nio.testing.LocalStorageHelper;
import com.google.cloud.testing.junit4.MultipleAttemptsRule;
import com.google.common.collect.ImmutableList;
import com.google.common.testing.NullPointerTester;
import java.io.IOException;
import java.io.InputStream;
import java.io.OutputStream;
import java.lang.reflect.Field;
import java.net.URI;
import java.nio.ByteBuffer;
import java.nio.channels.ReadableByteChannel;
Expand Down Expand Up @@ -795,11 +800,65 @@ public void testFromSpace() throws Exception {
assertThat(path4.toString()).isEqualTo("/with/a%20percent");
}

// port of test from
// https://github.com/broadinstitute/cromwell/pull/6491/files#diff-758dbbe823e71cc26fee7bc89cd5c434dfb76e604d51005b8327db59aab96068R300-R336
@Test
public void cromwell6491() throws Exception {
BenWhitehead marked this conversation as resolved.
Show resolved Hide resolved

CloudStorageConfiguration config =
CloudStorageConfiguration.builder()
.permitEmptyPathComponents(true)
.stripPrefixSlash(true)
.usePseudoDirectories(true)
.build();

Credentials noCredentials = NoCredentials.getInstance();
Credentials saCredentials = new QuotaProjectIdHidingCredentials(noCredentials);

StorageOptions noOptions =
StorageOptions.newBuilder()
.setProjectId("public-project")
.setCredentials(noCredentials)
.build();

StorageOptions saOptions =
StorageOptions.newBuilder()
.setProjectId("private-project")
.setCredentials(saCredentials)
.build();

CloudStorageFileSystem noFs =
CloudStorageFileSystem.forBucket("public-bucket", config, noOptions);
CloudStorageFileSystem saFs =
CloudStorageFileSystem.forBucket("private-bucket", config, saOptions);

CloudStoragePath noPath = noFs.getPath("public-file");
CloudStoragePath saPath = saFs.getPath("private-file");

assertThat(credentialsForPath(noPath)).isEqualTo(noCredentials);
assertThat(credentialsForPath(saPath)).isEqualTo(saCredentials);
}

private static CloudStorageConfiguration permitEmptyPathComponents(boolean value) {
return CloudStorageConfiguration.builder().permitEmptyPathComponents(value).build();
}

private static CloudStorageConfiguration usePseudoDirectories(boolean value) {
return CloudStorageConfiguration.builder().usePseudoDirectories(value).build();
}

private static Credentials credentialsForPath(Path p)
throws NoSuchFieldException, IllegalAccessException {
CloudStorageFileSystemProvider cloudFilesystemProvider =
(CloudStorageFileSystemProvider) p.getFileSystem().provider();
Field storageOptionsField =
cloudFilesystemProvider.getClass().getDeclaredField("storageOptions");
storageOptionsField.setAccessible(true);
StorageOptions storageOptions =
(StorageOptions) storageOptionsField.get(cloudFilesystemProvider);
Field credentialsField =
storageOptions.getClass().getSuperclass().getDeclaredField("credentials");
credentialsField.setAccessible(true);
return (Credentials) credentialsField.get(storageOptions);
}
}
14 changes: 11 additions & 3 deletions ...d-nio/src/test/java/com/google/cloud/storage/contrib/nio/CloudStorageIsDirectoryTest.java
View file
Open in desktop
Expand Up @@ -32,6 +32,7 @@
import org.junit.Before;
import org.junit.Rule;
import org.junit.Test;
import org.junit.rules.TestName;
import org.junit.runner.RunWith;
import org.junit.runners.JUnit4;

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

@Rule public final TestName testName = new TestName();

private StorageOptions mockOptions;
private Storage mockStorage;

@Before
public void before() {
mockOptions = mock(StorageOptions.class);
mockOptions =
mock(
StorageOptions.class,
String.format("storage-options-mock_%s", testName.getMethodName()));
mockStorage = mock(Storage.class);
when(mockOptions.getService()).thenReturn(mockStorage);
CloudStorageFileSystemProvider.setStorageOptions(mockOptions);
Expand All @@ -54,7 +60,7 @@ public void before() {
@Test
public void testIsDirectoryNoUserProject() {
CloudStorageFileSystem fs =
CloudStorageFileSystem.forBucket("bucket", CloudStorageConfiguration.DEFAULT);
CloudStorageFileSystem.forBucket("bucket", CloudStorageConfiguration.DEFAULT, mockOptions);
when(mockStorage.get(BlobId.of("bucket", "test", null)))
.thenThrow(new IllegalArgumentException());
Page<Blob> pages = mock(Page.class);
Expand All @@ -74,7 +80,9 @@ public void testIsDirectoryNoUserProject() {
public void testIsDirectoryWithUserProject() {
CloudStorageFileSystem fs =
CloudStorageFileSystem.forBucket(
"bucket", CloudStorageConfiguration.builder().userProject("project-id").build());
"bucket",
CloudStorageConfiguration.builder().userProject("project-id").build(),
mockOptions);
when(mockStorage.get(BlobId.of("bucket", "test", null)))
.thenThrow(new IllegalArgumentException());
Page<Blob> pages = mock(Page.class);
Expand Down