FileSystem always created

[stefanofornari]

Is a design choice to create always a new S3FileSystem in both newFileSystem and getFileSystem? It kind of violates the intended behaviour of NIO file systems and should not be a big effort to create a caching mechanism.

I have also noticed that getPath() calls FileSystems.getFileSystem(uri), which then creates a new FileSystem. However S3FileSystem keeps some state like if open or open channels. Could this be a problem?

[markjschreiber]

FileSystem implementations include a close() method which is supposed to be called when done. This method closes all open channels (or should if it doesn't already). It would actually be a good idea to make the implementation implement AutoClosable so that it could be used in a "try-with-resources" block. It already has the required method.

One challenge of caching FileSystem implementations is that you would then have to carefully consider the behavior if it is used in multiple threads. One thread might close the object while another is trying to use it. What happens if there are two channels to the same object? Sharing them would be really bad. Even without threading if the object is closed, should further requests to the cache return the closed object or create a new one (probably the latter?). You could make the S3FileSystem unclosable like the default FileSystem which would be OK (S3 is never really closed) but we would have to think about how to clean up any channels (probably they would have to be closed individually).

The Java docs for the FileSystem interface say that:
"File systems are safe for use by multiple concurrent threads. The close method may be invoked at any time to close a file system but whether a file system is asynchronously closeable is provider specific and therefore unspecified. In other words, if a thread is accessing an object in a file system, and another thread invokes the close method then it may require to block until the first operation is complete. Closing a file system causes all open channels, watch services, and other closeable objects associated with the file system to be closed."

Allowing multiple instances is an easy way to get around this, but perhaps not the best way to do it. I'm open to proposals.

[stefanofornari]

I read two topics: closing a FS and multithreading; in both cases, if I got you right, you are concerned that instances of S3FileSystem are not thread-safe and the workaround is creating a new instance when the FS is requested. I think you have a point that we need to take into account the state of a FS when changed potentially concurrently.
However, I do not see (maybe I missed it) how these issues can be prevented by creating always a new instance. In fact I could get an instance calling getFileSystem() only once and access it from multiple threads.
Anyway, as long as the the overhead of creating a new instance is negligible, it is not a big deal; I see two main issues at the moment:

1. it is counter intuitive from a FileSystemSPI perspective: the javadoc does not mandate to use the new/get semantic, but not sticking to it makes it odd
2. getPath() creating a new FS can really have some undesired side effects like loosing the state of the FS

My suggestion would be to cache the FS instances by bucket as a first step and focus on thread safety later on.

[markjschreiber]

You are correct, creating a new FS instance doesn't make it inherently thread safe, it just makes it easier to avoid the need to have multiple threads use the same instance.

What would be an example of FS state that you are worried about loosing?

[stefanofornari]

The first one that comes into my mind is that a client of the API may want to close the FS to prevent further actions, and instead getPath() would create a new open FS. The other state we loose is the open channels.

In the future, I may think of toggling a FS between read only and read/write, or change credentials. Or if we want to simulate the current directory or implement a watching mechanism.

Now that I am looking into developing #11, there is a real issue because FileSystems provides the env map only when calling

newFileSystem(uri, env);

This means that getPath() will always return a new unset file system.

[stefanofornari]

In implementing #11 I am hitting more and more against the wall of the current architecture. Basically, the fact that S3ClientStore is unrelated to the setup provided in the provider makes it not very practical to use in real cases.
For example, I manage to make all tests pass with a uri as follows:

s3://key:secret@endpoint/bucket/resource

however, as soon as I use it in a real case like:

Stream<Path> stream = Files.walk(s3.getPath("/subdir1"));
for (Path path: stream.filter(Files::isRegularFile).toList()) {
    System.out.println(path);
}

I get the exception

20230604 151935 FINE software.amazon.awssdk.auth.credentials.AwsCredentialsProviderChain Unable to load credentials from SystemPropertyCredentialsProvider(): Unable to load credentials from system settings. Access key must be specified either via environment variable (AWS_ACCESS_KEY_ID) or system property (aws.accessKeyId).
software.amazon.awssdk.core.exception.SdkClientException: Unable to load credentials from system settings. Access key must be specified either via environment variable (AWS_ACCESS_KEY_ID) or system property (aws.accessKeyId).
	at software.amazon.awssdk.core.exception.SdkClientException$BuilderImpl.build(SdkClientException.java:111)
	at software.amazon.awssdk.auth.credentials.internal.SystemSettingsCredentialsProvider.resolveCredentials(SystemSettingsCredentialsProvider.java:58)
	at software.amazon.awssdk.auth.credentials.AwsCredentialsProviderChain.resolveCredentials(AwsCredentialsProviderChain.java:96)
	at software.amazon.awssdk.auth.credentials.internal.LazyAwsCredentialsProvider.resolveCredentials(LazyAwsCredentialsProvider.java:45)
	at software.amazon.awssdk.auth.credentials.DefaultCredentialsProvider.resolveCredentials(DefaultCredentialsProvider.java:126)
	at software.amazon.awssdk.core.internal.util.MetricUtils.measureDuration(MetricUtils.java:50)
	at software.amazon.awssdk.awscore.internal.authcontext.AwsCredentialsAuthorizationStrategy.resolveCredentials(AwsCredentialsAuthorizationStrategy.java:100)
	at software.amazon.awssdk.awscore.internal.authcontext.AwsCredentialsAuthorizationStrategy.addCredentialsToExecutionAttributes(AwsCredentialsAuthorizationStrategy.java:77)
	at software.amazon.awssdk.awscore.internal.AwsExecutionContextBuilder.invokeInterceptorsAndCreateExecutionContext(AwsExecutionContextBuilder.java:123)
	at software.amazon.awssdk.awscore.client.handler.AwsSyncClientHandler.invokeInterceptorsAndCreateExecutionContext(AwsSyncClientHandler.java:69)
	at software.amazon.awssdk.core.internal.handler.BaseSyncClientHandler.lambda$execute$1(BaseSyncClientHandler.java:78)
	at software.amazon.awssdk.core.internal.handler.BaseSyncClientHandler.measureApiCallSuccess(BaseSyncClientHandler.java:179)
	at software.amazon.awssdk.core.internal.handler.BaseSyncClientHandler.execute(BaseSyncClientHandler.java:76)
	at software.amazon.awssdk.core.client.handler.SdkSyncClientHandler.execute(SdkSyncClientHandler.java:45)
	at software.amazon.awssdk.awscore.client.handler.AwsSyncClientHandler.execute(AwsSyncClientHandler.java:56)
	at software.amazon.awssdk.services.s3.DefaultS3Client.getBucketLocation(DefaultS3Client.java:3510)
	at software.amazon.awssdk.services.s3.S3Client.getBucketLocation(S3Client.java:5578)

This is because S3ClientStore uses another client created differently to get the the bucket location (I am not sure why though, see #11 (comment)).

I could adapt S3ClientStore to fix the issue, but I feel it would not improve the overall architecture.

I'd like therefore propose a different approach, on which I could focus first: what about if every instance of a FS has its own S3Client? The idea is to create the client instance in the constructor of S3FileSystem so that it can be fully configured given provider's environment and uri. The benefit would be that everything related to the FS will be encapsulate in it.
If then we want to do caching, we can cache the S3FileSystem instances in S3FileSystemProvider, fixing also getFileSystem(), which will be able to return a previously created FS.

Let me know your thoughts.

[markjschreiber]

I think that sounds reasonable and provides better encapsulation. I admit I didn't consider alternative S3 providers that are not AWS. I think it may also be useful with buckets that exist on things like Snowball devices and AWS Outposts.

To allow for better testing and composition it might be useful to be able to supply a ClientProvider function to a FileSystem which will be responsible for building the client(s) and memoization/ caching if needed. It could also take care of any desired properties like creating a new client whenever a AWS API call encounters a 500 error. This is something that S3 suggests as a new client will likely resolve to a new S3 endpoint IP address.

[stefanofornari]

btw, I managed to fix the current behaviour in S3ClientStore. I will update the PR soon.

Then I can work on the idea above. I agree on the approach of a factory class (ie ClientProvider).

[stefanofornari]

As a first step, I'd create a client provider and use it in S3ClientStore. This would be just a refactoring and I would start from main. Let me know if you see any issue.

[markjschreiber]

Sounds good. Include a method or constructor to allow the client provider/ factory to be supplied to the S3ClientStore so that the default can be overridden.

[stefanofornari]

In order to cache the file systems, I am going to use the following key:

[hostname][:port]/bucket

This means that trying to create a file system (i.e. newFileSystem()) on the same endpoint and bucket with different credentials will result in a FileSystemAlreadyExistsException. If the passed URI contains some credentials, they will be used to access the bucket (see also #61).
Instead, getFileSystem() ignores credentials passed in the URI and returns an existing file system instance (if already created) regardless of the provided credentials.

Let me know your feedback.

[stefanofornari]

On Backward compatibility: the new version of S3FileSystemProvider and S3FileSystem will be quite different... I am not sure how to handle backward compatibility and I am looking for directions here. Options I see:

1. bump the version of the API breaking compatibility
2. create a new provider/filesystem class and use it by default by updating META-INF/services
3. create a new provider/filesystem class but use the former by default by NOT updating META-INF/services
4. trying to keep backward compatibility deprecating a lot of things

It's probably needed to look at the new implementation to make a decision, but I wanted to anticipate the discussion as it may drive the implementation. I will update the branch soon.

[stefanofornari]

On S3FileSystem: the current design needs the full URI to create instances of S3FileSystem. This made sense because so far the provider was doing little and delegated instead all the work to the FileSystem.
With the new design the provider acts as a factory for the file systems because it has to manage the life cycle of the instances (e.g. caching them), while S3FileSystem focuses more on the path logic. I therefore think the a file system instance should deal with local URLs, keeping context information such as endpoint and bucket in instance variables. This means it would not be created with a full path any more (i.e. s3://bucketname/dir/object.txt) but with an absolute path starting from the bucket (i.e. /dir/object.txt). The reason behind the choice is that it would be otherwise (potentially) possible that the provider receives a s3 uri in newFileSystem() and creates an instance file system with a different s3 uri. This makes the semantic of S3FileSystem unclear.