spf13 / afero

A FileSystem Abstraction System for Go
Apache License 2.0
6k stars 515 forks source link

Fs interface needs to support context.Context for all API calls #27

Open nt opened 8 years ago

nt commented 8 years ago

Proper implementations of Afero for rpc backed filesystems will require passing a context.Context.

See: https://godoc.org/golang.org/x/net/context

mbertschler commented 8 years ago

@nt could you point us to an example of a similar project where a Context is used? Would all the Fs functions need a second version where a Context is passed or how would the API look like?

nt commented 8 years ago

Here is an example: https://godoc.org/google.golang.org/api/storage/v1#BucketsListCall.Context see this blog post for details: https://blog.golang.org/context.

To generalize your API you would do:

type Fs interface {
    // Create creates a file in the filesystem, returning the file and an
    // error, if any happens.
    Create(ctx context.Context, name string) (File, error)

        ...
}

That would allow user implementing a rpc or http handler supporting contexts such as grpc or gorrilla to passit through as is. If the parent call is cancelled, fanned out rpcs to a remote Afero backend can be cancelled as well and resources can be released.

This is critical if you want to set deadline on calls and you do not know how long fs.Create("foo") will take.

spf13 commented 8 years ago

I understand the need for this. However most of the backends wouldn't need or be able to use this. One of the initial goals of afero was to be interoperable with the os package as much as possible. Our function signatures mirror that of the os pacakge. This would be a dramatic shift.

Additionally wouldn't the Context be better set on the entire Fs rather than per function. This would enable compatibility with current backends.

I see Sameer's note in his post that "At Google, we require that Go programmers pass a Context parameter as the first argument to every function on the call path between incoming and outgoing requests.".

Just trying to figure out how to incorporate this without breaking everything.

prochac commented 3 months ago

What about something like this? https://github.com/golang/go/issues/20280#issuecomment-1758770221

fs := NewAWSS3FS( /* ... */ ) // good 'ol XMLHttpRequest 😅
afs := &afero.AferoCtx{Fs: fs}
f, err := afs.Context(ctx).Create("foo.txt")
f.Close()
info, err := afs.Context(ctx).Stat("foo.txt")

BTW Go's context has been criticized precisely for this :/ https://faiface.github.io/post/context-should-go-away-go2/