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

io/fs: add file system interfaces #41190

Closed
rsc opened this issue Sep 2, 2020 · 115 comments
Closed

io/fs: add file system interfaces #41190

rsc opened this issue Sep 2, 2020 · 115 comments

Comments

@rsc
Copy link
Contributor

rsc commented Sep 2, 2020

In July, @robpike and I posted a draft design for file system interfaces. That doc links to a video, prototype code, and a Reddit discussion.

The feedback on that design has been almost entirely positive.

A few people raised concerns about the use of optional interfaces, but those are an established pattern in Go that we understand how to use well (informed in part by some earlier mistakes, such as optional interface like http.Hijacker with methods that cannot return an error to signal failure/unavailability).

A few people suggested radical redesigns of the os.File interface itself, but for better or worse installed base and the weight of history cautions against such drastic changes.

I propose to adopt the file system interfaces draft design for Go 1.16.

@rsc
Copy link
Contributor Author

rsc commented Sep 2, 2020

Accepting this proposal would also let us land the embedded files draft design in Go 1.16, which I've proposed in #41191.

@tooolbox
Copy link

tooolbox commented Sep 2, 2020

A few people raised concerns about the use of optional interfaces

This is my concern.

It's not that the alternative (attempting to define all FS methods in one huge interface which packages may then implement as no-op) is better. Rather, my perception is that the ergonomics of this approach are poor enough that it won't achieve broad community adoption and the level of composability and general success that interfaces like io.Reader and io.Writer have.

For example, it's clear that I will be able to pipe a zip file to text/template, and that's good, but I'm concerned about more general composability of filesystems and files. I can wrap a stack of io.Reader with confidence, but with io/fs it seems like some middle layer may not have the right optional interfaces and I will lose access to functionality.

In spite of my concerns, it seems like the best approach available to Go at this time, and I anticipate it will be accepted given that the very exciting #41191 depends upon it.

However, I have this inkling that the advent of generics may allow a more powerful/robust/safe abstraction. Has any thought been given to this, or to how io/fs could evolve in a backwards-compatible fashion if/when that occurs? Again, not to hold up this proposal, but I think I would be more excited if I knew what the future held.

@networkimprov
Copy link

networkimprov commented Sep 2, 2020

The feedback page:
https://www.reddit.com/r/golang/comments/hv976o/qa_iofs_draft_design/?sort=new

I think this API looks promising... and would benefit from a prototype phase.

A lot of feedback was posted, but there's been rather light discussion of the comments, presumably because you can't subscribe to a Reddit thread, and/or many in Go's github-centered community don't frequent Reddit. It would help to see a review and analysis of feedback here, and perhaps a roadmap to likely future features.

Problems were identified with the FileInfo interface, but not discussed and are in discussion #41188. Timeouts and/or interrupts bear consideration.

Landing a prototype in x/ seems like a logical step before stdlib. Go has long been deliberative and conservative about new features. Is this urgent somehow?

FWIW, my Go apps make heavy use of the filesystem, on Windows, MacOS, and Linux.

@earthboundkid

This comment has been minimized.

@earthboundkid

This comment has been minimized.

@rsc
Copy link
Contributor Author

rsc commented Sep 3, 2020

Discussion of ErrNotImplemented has moved to #41198.
I've marked @carlmjohnson's two comments above this one
as well as @randall77's comment below this one
as "off-topic" to try to funnel discussion over there.

@randall77

This comment has been minimized.

@rsc
Copy link
Contributor Author

rsc commented Sep 3, 2020

@networkimprov, see https://go.googlesource.com/proposal/+/master/design/draft-iofs.md#why-not-in-golang_org_x.
This isn't worth much without the standard library integration. It also can't be used for embed.Files from x.

@jimmyfrasche
Copy link
Member

I'd feel more comfortable with this if it included basic combinatory file systems either in io/fs or somewhere official like golang.org/x. They would still have the issue with not understanding nonstandard optional methods but they would at least be guaranteed to keep up with official optional methods.

The two file systems I'm thinking of are an overlay fs and one that can "mount" other file systems in subdirectories of its root. With those two you could stitch multiple fs together easily.

@rsc
Copy link
Contributor Author

rsc commented Sep 3, 2020

@jimmyfrasche I don't understand the difference between "an overlay fs" and "one that can mount other file systems in subdirectories of its root." I agree we should provide something like that, and we intend to. But those sound like the same thing to me. :-)

@jimmyfrasche
Copy link
Member

I was thinking just:

func Overlay(fses ...FS) FS for the former and the latter would satisfy

interface {
  FS
  Mount(dirName string, fs FS) error
}

and not have any files other than those mounted.

@rsc
Copy link
Contributor Author

rsc commented Sep 3, 2020

Got it, thanks @jimmyfrasche: union vs replace.

@networkimprov
Copy link

networkimprov commented Sep 3, 2020

The io/fs integration with stdlib, and embed.Files can all be prototyped in x/

I wasn't suggesting x/ as the permanent home.

EDIT: Also, Readdir() & FileInfo have performance problems and missing features. The replacement APIs need prototypes. A draft is in #41188 (comment)

@muirdm
Copy link

muirdm commented Sep 3, 2020

I have two comments. I found similar comments in the reddit thread, but didn't see a satisfying discussion/conclusion. Apologies if I missed previous conclusions.

Wrapping

I think we should consider an official mechanism to wrap fs.FS objects (and probably fs.File objects). For example, I want to wrap an fs.FS to track the total number of bytes read. I need to intercept calls to fsys.Open and calls to fsys.ReadFile, if implemented. I also don't want to lose any other optional interfaces such as fs.GlobFS. Based on my experience with http.ResponseWriter, this is commonly needed, but hard and tedious to do correctly

For a concrete idea to discuss, something like this:

type FSWrapper struct {
  FS
  OpenFunc func(name string) (File, error)
  ReadFileFunc func(name string) ([]byte, error)
  // ... all other extensions ...
}

func (w *FSWrapper) ReadFile(name string) ([]byte, error) [
  rf, ok := w.FS.(ReadFileFS)
  if !ok {
    return nil, errors.ErrNotImplemented
  }

  if w.ReadFileFunc != nil {
    return w.ReadFileFunc(name)
  } else {
    return rf.ReadFileFunc(name)
  }
}

Granted there are cases where a generic wrapper would expose extensions you don't want to pass through. Anyway, I think at least the proposal would benefit from discussion or FAQ addressing wrapping.

Writing

It seems like we are starting with read-only because that is the simplest interface that enables the motivating embed feature. However, in some sense writing is more fundamental because you have to write before you can read (only half joking). I worry writing will be relegated to second class citizenship forever due to optional interfaces. For example, the hypothetical OpenFile extension:

func OpenFile(fsys FS, name string, flag int, perm os.FileMode) (File, error)

OpenFile returns an fs.File which has no Write method. It seems a bit strange to always have to type assert to get a writable file. I think the eternal friction between io.Writer and fs.File as proposed will be more painful than starting with a broader proposal.

In particular, I think we should consider:

  1. Make "OpenFile" be the core of fs.FS instead of "Open". OpenFile is more fundamental to file systems. We can add "func Open(fsys FS, name string) (File, error)" as a package function to emulate the simplicity of the proposed FS.Read method.
  2. Include "Write" in the fs.File interface. Write is as fundamental as Read for file systems.

@Cyberax
Copy link

Cyberax commented Sep 4, 2020

Guys, PLEASE just add context everywhere! It costs nothing to ignore it or add context.TODO() for callers, but it will make life of network filesystem implementers and users much easier. In particular, it's needed for better contextual logging and cancellation.

You're all strictly opposed to thread-local variables, but then why are you designing APIs without a way to pass a context?!?

@networkimprov
Copy link

Deadlines and interrupts have been suggested as another way to solve the same problem, without affecting every function signature. It's unlikely that the os package will add dozens of new APIs with Context, see also #41054.

Deadlines comment
Interrupts comment

@Merovius
Copy link
Contributor

Merovius commented Sep 4, 2020

I think the simplest way to solve this is to pass a context on filesystem creation. So instead of having a type implementing fs.FS directly, it would have a method WithContext(context.Context) fs.FS, which returns a child-instance bound to a given context.

@networkimprov
Copy link

Cancelling all pending ops by the stdlib file API (which will implement fs.FS) is not desirable. It probably isn't useful for other FS types, as well. The common case, in my experience, is interrupting any pending ops trying paths within a tree rooted at a certain path. An API for that looks like one of:

(f *MyFs) SetDeadline(t time.Time, basepath string) error // if deadline past, interruption is immediate

(f *MyFs) InterruptPending(basepath string) error

Note that os.File already supports Read & Write deadlines.

I doubt that io/fs wants context as a dependency. Where needed, you could easily wire context into an fs.FS implementation to do one of the above.

@Cyberax
Copy link

Cyberax commented Sep 5, 2020

Gah. The deadlines/interrupts design is just horrible. No, it's seriously horrible. The whole idea for not including thread IDs in Golang was to make sure APIs are forced to deal with clients potentially running in multiple goroutines.

Introducing the per-FS state will defeat this purpose, making the FS object behave more like a TCP connection rather than a dispatcher for an underlying FS. And only one goroutine at a time would be able to use it, otherwise they might step on each others' toes with deadlines. Never mind the badness of introducing a hidden state where it arguably shouldn't even be in the first place.

What are the actual downsides of simply adding context.Context to every method?

@Cyberax
Copy link

Cyberax commented Sep 5, 2020

I think the simplest way to solve this is to pass a context on filesystem creation. So instead of having a type implementing fs.FS directly, it would have a method WithContext(context.Context) fs.FS, which returns a child-instance bound to a given context.

This will require the FS implementation to be a thin wrapper that supplies context to the underlying implementation. Certainly doable, but still ugly.

And it will still introduce dependency on context.Context in the FS code.

@tv42
Copy link

tv42 commented Sep 5, 2020

@Cyberax All you need is f, err := fsys.WithContext(ctx).Open(p) to make that one open file obey that one context. Easy sharing.

This has been before with x.IO(ctx) returning io.Reader or such, to keep the io.Reader interface. It's a pretty simple layer.

@gopherbot
Copy link

Change https://golang.org/cl/268417 mentions this issue: io/fs: fix reference to WalkFunc

gopherbot pushed a commit that referenced this issue Nov 11, 2020
The comment explains differences between WalkDirFunc and WalkFunc,
but when this code moved out of path/filepath, we forgot to change
the reference to be filepath.WalkFunc. Fix that.

(The text should not be deleted, because path/filepath does not
contain this type - WalkDirFunc - nor this text anymore.)

Pointed out by Carl Johnson on CL 243916 post-submit.

For #41190.

Change-Id: I44c64d0b7e60cd6d3694cfd6d0b95468ec4612fe
Reviewed-on: https://go-review.googlesource.com/c/go/+/268417
Trust: Russ Cox <rsc@golang.org>
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
@gopherbot
Copy link

Change https://golang.org/cl/273946 mentions this issue: all: update references to symbols moved from io/ioutil to io

@mattn
Copy link
Member

mattn commented Dec 3, 2020

In contrast, reading is easier to make abstract. A particular reader may well not care about any particular detail like modification times, uids, or compression algorithms. That's fine: it can ignore them. And another reader that does care can look at info.ModTime() or even use info.Sys() to get implementation-specific additional detail. Both can exist simultaneously for the same file: the abstract reader and the more specific reader. Using an abstract reader does not preclude also using a specific reader.

Do you have plan to provide way to update ModTime in embed file? Current implementation of ModTime return always zero-time. So http.FS does not set Last-Modified header. i.e. browser does not cache the contents. I know it's possible to implement with adding ETag but not easy way, I think.

@thomasf
Copy link

thomasf commented Dec 3, 2020

IIRC ETag is already implemented in http.FS

@mattn
Copy link
Member

mattn commented Dec 3, 2020

FS support ETag but it seems only scan. FS does not set ETag. I wrote small example code. But server does not set ETag header.

@Merovius
Copy link
Contributor

Merovius commented Dec 3, 2020

@mattn Wrong issue. You are looking for #41191

@mattn
Copy link
Member

mattn commented Dec 3, 2020

Ah, u are right.

@flowchartsman
Copy link

flowchartsman commented Jan 18, 2021

Got it, thanks @jimmyfrasche: union vs replace.

I wanted to follow up on the mounting/overlay discussion a bit. I was experimenting with a package this weekend for self-hosting a copy of Swagger UI with http.FileServer, with the goal to allow the user to provide their own specification file embedded in the filesystem. When creating the handler, I ended up simulating a union FS by dipping into testing/fstest and creating a MapFS containing the spec file and a templatized index.html. This worked well enough, but it's kludgy since it relies on explicit paths, doesn't address the case of directory listings, and is almost certainly abusing poor fstest. Eventually I found myself thinking it would be nice to simply take an fs.FS or even an embed.FS when creating the handler. This could allow the user to provide alternate CSS files or favicons for example, with perhaps some checks on allowed globs to ensure that the incoming filesytem didn't replace anything that would cause issues. This ended up being a rabbit hole, though initial testing indicates it's probably do-able at least for the limited use in an http.FileServer.

All that said, this use-case is fairly specific, but I can see it being useful functionality elsewhere, and I was wondering if there were any plans to allow for layering filesystems or something like fs.Mount(f FS, dir string) or fs.Overlay(layers ...FS).

I can see the implementation for the latter being relatively complex, but, at least for this case I could easily work around it with mount-like functionality, and the logic for that doesn't seem too crazy, you'd just maybe want to ensure that the mount path doesn't already exist and then do some mapping on the path in a wrapped type from there, similar to how Sub works, right?

Edit: having gotten most of the way through a prototype implementation of anfs.Mount testing against merging two embed.FS, the wrapping and interfaces involved actually make it quite tricky. I hit a wall attempting to get the "mounted" FS root into a toplevel []fs.DirEntry for synthesizing it as a subdirectory in the containing filesystem. It started to get messy, which is a surefire sign I'm fighting the design too much, and I may need to rethink it. It still seems like useful functionality, but certainly doesn't appear straightforward from what I'm seeing.

@golang golang deleted a comment Jan 18, 2021
@gopherbot
Copy link

Change https://golang.org/cl/285093 mentions this issue: doc/go1.16: add notes about package-specific fs.FS changes

gopherbot pushed a commit that referenced this issue Jan 21, 2021
For #40700
For #41190

Change-Id: I964d6856d5cad62c859d0f3a7afdd349a8ad87cb
Reviewed-on: https://go-review.googlesource.com/c/go/+/285093
Trust: Ian Lance Taylor <iant@golang.org>
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
@gopherbot
Copy link

Change https://golang.org/cl/285592 mentions this issue: doc/go1.16: mention os.DirEntry and types moved from os to io/fs

@gopherbot
Copy link

Change https://golang.org/cl/285593 mentions this issue: doc/go1.16: mention os.DirFS in os section

@gopherbot
Copy link

Change https://golang.org/cl/285673 mentions this issue: doc/go1.16: mention new testing/iotest functions

gopherbot pushed a commit that referenced this issue Jan 25, 2021
For #38781
For #40700
For #41190

Change-Id: I72f1055e51edb517041d3861640734ba6ef5f342
Reviewed-on: https://go-review.googlesource.com/c/go/+/285673
Trust: Ian Lance Taylor <iant@golang.org>
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
gopherbot pushed a commit that referenced this issue Jan 25, 2021
For #40700
For #41467
For #41190

Change-Id: Id94e7511c98c38a22b1f9a55af6e200c9df07fd3
Reviewed-on: https://go-review.googlesource.com/c/go/+/285592
Trust: Ian Lance Taylor <iant@golang.org>
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
gopherbot pushed a commit that referenced this issue Jan 25, 2021
For #40700
For #41190

Change-Id: I8ade6efd5be09003fc3e5db5a9b91ba6e0f023f7
Reviewed-on: https://go-review.googlesource.com/c/go/+/285593
Trust: Ian Lance Taylor <iant@golang.org>
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
@odeke-em
Copy link
Member

odeke-em commented Feb 6, 2021

Well done everyone for the discourse, the work, and reviews on io/fs during the Go1.16 cycle. I believe we can now close this issue given all the prior work: @rsc @robpike @ianlancetaylor would y’all like to do the honors of closing it, or shall I? Thank you.

@odeke-em
Copy link
Member

Closing this issue as folks seem swamped, congrats everyone!

gopherbot pushed a commit that referenced this issue Apr 5, 2021
Update references missed in CL 263142.

For #41190

Change-Id: I778760a6a69bd0440fec0848bdef539c9ccb4ee1
GitHub-Last-Rev: dda42b0
GitHub-Pull-Request: #42874
Reviewed-on: https://go-review.googlesource.com/c/go/+/273946
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
Trust: Cherry Zhang <cherryyz@google.com>
@golang golang locked and limited conversation to collaborators Feb 11, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
No open projects
Development

No branches or pull requests