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

Open
rsc opened this issue Sep 2, 2020 · 95 comments
Open

io/fs: add file system interfaces #41190

rsc opened this issue Sep 2, 2020 · 95 comments

Comments

@rsc
Copy link
Contributor

@rsc 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 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 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 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.

@carlmjohnson

This comment was marked as off-topic.

@carlmjohnson

This comment was marked as off-topic.

@rsc
Copy link
Contributor Author

@rsc 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 was marked as off-topic.

@rsc
Copy link
Contributor Author

@rsc 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

@jimmyfrasche jimmyfrasche commented Sep 3, 2020

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 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

@jimmyfrasche jimmyfrasche commented Sep 3, 2020

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 rsc commented Sep 3, 2020

Got it, thanks @jimmyfrasche: union vs replace.

@networkimprov
Copy link

@networkimprov 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 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 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

@networkimprov networkimprov commented Sep 4, 2020

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

@Merovius 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

@networkimprov networkimprov commented Sep 5, 2020

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 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 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 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.

@rsc
Copy link
Contributor Author

@rsc rsc commented Oct 14, 2020

No change in consensus, and #41467 is accepted, so accepting.

@rsc rsc moved this from Likely Accept to Accepted in Proposals Oct 14, 2020
@gopherbot
Copy link

@gopherbot gopherbot commented Oct 16, 2020

Change https://golang.org/cl/243909 mentions this issue: testing/iotest: add TestReader to test readers

@gopherbot
Copy link

@gopherbot gopherbot commented Oct 16, 2020

Change https://golang.org/cl/243905 mentions this issue: go/build: allow io/fs to depend on time

@gopherbot
Copy link

@gopherbot gopherbot commented Oct 16, 2020

Change https://golang.org/cl/243900 mentions this issue: os: use keyed literals for PathError

@gopherbot
Copy link

@gopherbot gopherbot commented Oct 16, 2020

Change https://golang.org/cl/243907 mentions this issue: all: update references to symbols moved from os to io/fs

@gopherbot
Copy link

@gopherbot gopherbot commented Oct 16, 2020

Change https://golang.org/cl/243906 mentions this issue: io/fs: move FileInfo, FileMode, PathError, ErrInvalid, ... from os to io/fs

gopherbot pushed a commit that referenced this issue Oct 16, 2020
In preparation for moving os.FileInfo into io/fs.
Also keep syscall from depending on io again.
We want to keep them separated, in case io ever
needs to start depending on time.

For #41190.

Change-Id: I98350fa03accf4a20c75ddebb0e961aa1ccccd2c
Reviewed-on: https://go-review.googlesource.com/c/go/+/243905
Trust: Russ Cox <rsc@golang.org>
Run-TryBot: Russ Cox <rsc@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Rob Pike <r@golang.org>
@gopherbot
Copy link

@gopherbot gopherbot commented Oct 16, 2020

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

@gopherbot
Copy link

@gopherbot gopherbot commented Oct 17, 2020

Change https://golang.org/cl/243911 mentions this issue: os: add DirFS

@gopherbot
Copy link

@gopherbot gopherbot commented Oct 17, 2020

Change https://golang.org/cl/243908 mentions this issue: io/fs: add FS, File, ReadDirFile; move DirEntry from os

@gopherbot
Copy link

@gopherbot gopherbot commented Oct 17, 2020

Change https://golang.org/cl/243910 mentions this issue: testing/fstest: new package for testing file system code

@rsc rsc modified the milestones: Proposal, Go1.16 Oct 17, 2020
@rsc rsc changed the title proposal: io/fs: add file system interfaces io/fs: add file system interfaces Oct 17, 2020
@gopherbot
Copy link

@gopherbot gopherbot commented Oct 18, 2020

Change https://golang.org/cl/243912 mentions this issue: io/fs: add ReadFile and ReadFileFS

@gopherbot
Copy link

@gopherbot gopherbot commented Oct 18, 2020

Change https://golang.org/cl/243914 mentions this issue: io/fs: add ReadDir and ReadDirFS

@gopherbot
Copy link

@gopherbot gopherbot commented Oct 18, 2020

Change https://golang.org/cl/243913 mentions this issue: io/fs: add Stat and StatFS

@gopherbot
Copy link

@gopherbot gopherbot commented Oct 18, 2020

Change https://golang.org/cl/243915 mentions this issue: io/fs: add Glob and GlobFS

@gopherbot
Copy link

@gopherbot gopherbot commented Oct 18, 2020

Change https://golang.org/cl/243916 mentions this issue: io/fs: add Walk

@gopherbot
Copy link

@gopherbot gopherbot commented Oct 18, 2020

Change https://golang.org/cl/243939 mentions this issue: net/http: add FS to convert fs.FS to FileSystem

@gopherbot
Copy link

@gopherbot gopherbot commented Oct 18, 2020

Change https://golang.org/cl/243938 mentions this issue: html/template, text/template: add ParseFS

@gopherbot
Copy link

@gopherbot gopherbot commented Oct 18, 2020

Change https://golang.org/cl/243937 mentions this issue: archive/zip: make Reader implement fs.FS

gopherbot pushed a commit that referenced this issue Oct 20, 2020
Necessary to move PathError to io/fs.

For #41190.

Change-Id: I05e87675f38a22f0570d4366b751b6169f7a1b13
Reviewed-on: https://go-review.googlesource.com/c/go/+/243900
Trust: Russ Cox <rsc@golang.org>
Run-TryBot: Russ Cox <rsc@golang.org>
Reviewed-by: Rob Pike <r@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
gopherbot pushed a commit that referenced this issue Oct 20, 2020
… io/fs

First step of creating the new io/fs package.

For #41190.

Change-Id: I1339b1abdd533b0f1deab283628088b2f706fb5b
Reviewed-on: https://go-review.googlesource.com/c/go/+/243906
Trust: Russ Cox <rsc@golang.org>
Run-TryBot: Russ Cox <rsc@golang.org>
Run-TryBot: Emmanuel Odeke <emm.odeke@gmail.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Rob Pike <r@golang.org>
Reviewed-by: Emmanuel Odeke <emm.odeke@gmail.com>
gopherbot pushed a commit that referenced this issue Oct 20, 2020
The old os references are still valid, but update our code
to reflect best practices and get used to the new locations.

Code compiled with the bootstrap toolchain
(cmd/asm, cmd/dist, cmd/compile, debug/elf)
must remain Go 1.4-compatible and is excluded.

For #41190.

Change-Id: I8f9526977867c10a221e2f392f78d7dec073f1bd
Reviewed-on: https://go-review.googlesource.com/c/go/+/243907
Trust: Russ Cox <rsc@golang.org>
Run-TryBot: Russ Cox <rsc@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Rob Pike <r@golang.org>
gopherbot pushed a commit that referenced this issue Oct 20, 2020
There are many reader behaviors that are subtle and
worth testing, and it's nice to have one complete tester
instead of many incomplete ones.

For #41190, which will use this as part of a larger
file system implementation tester.

Change-Id: Ib4cc7fae94b0d9b45dfacadc52baa77ad3761322
Reviewed-on: https://go-review.googlesource.com/c/go/+/243909
Trust: Russ Cox <rsc@golang.org>
Run-TryBot: Russ Cox <rsc@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Rob Pike <r@golang.org>
Reviewed-by: Emmanuel Odeke <emm.odeke@gmail.com>
gopherbot pushed a commit that referenced this issue Oct 20, 2020
These are the core interfaces for the io/fs design.
See #41190 and https://golang.org/s/draft-iofs-design for details.

DirEntry was left behind in the previous move from os
but is needed for ReadDirFile, so it moves in this commit.

Also apply a couple comment changes suggested in
the review of CL 261540.

For #41190.

Change-Id: I087741545139ed30b9ba5db728a0bad71129500b
Reviewed-on: https://go-review.googlesource.com/c/go/+/243908
Trust: Russ Cox <rsc@golang.org>
Run-TryBot: Russ Cox <rsc@golang.org>
Reviewed-by: Emmanuel Odeke <emm.odeke@gmail.com>
Reviewed-by: Rob Pike <r@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Proposals
Accepted
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
You can’t perform that action at this time.