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

net/http: add ServeFileFS, FileServerFS, NewFileTransportFS #51971

Closed
cespare opened this issue Mar 26, 2022 · 23 comments
Closed

net/http: add ServeFileFS, FileServerFS, NewFileTransportFS #51971

cespare opened this issue Mar 26, 2022 · 23 comments

Comments

@cespare
Copy link
Contributor

cespare commented Mar 26, 2022

Update 2: The final agreed-upon API is in this comment:

package http

func ServeFileFS(w ResponseWriter, r *Request, fsys fs.FS, name string)
func FileServerFS(root fs.FS) Handler
func NewFileTransportFS(fsys fs.FS) RoundTripper

Update: The current proposal is to add the function ServeFSFile to net/http. This is an analogue to ServeFile which interoperates with io/fs file systems.

// ServeFSFile replies to the request with the contents
// of the named file or directory from the file system fsys.
//
// Files opened from fsys must implement the io.Seeker interface.
//
// If the provided file ... [rest of doc is the same as ServeFile]
func ServeFSFile(w ResponseWriter, r *Request, fsys fs.FS, name string)

Below is the original proposal.


Abstract

To better interoperate with io/fs, I propose adding two functions to net/http.

  • ServeFSFile, the io/fs-based analogue to ServeFile.

    func ServeFSFile(w ResponseWriter, r *Request, fsys fs.FS, name string)
    
  • ServeFSContent, the io/fs-based analogue to ServeContent.

    func ServeFSContent(w ResponseWriter, r *Request, info fs.FileInfo, content io.Reader)
    

Background

The net/http package provides three built-in ways of serving files:

  • ServeFile, which serves a file by name
  • ServeContent, which serves a file from an io.ReadSeeker and some additional metadata
  • FileSystem, which is turned into a Handler using FileServer

These were written before the io/fs package existed and do not work with those interfaces.

As part of adding io/fs, net/http gained FS which converts an io.FS into a FileSystem.

However, ServeFile and ServeContent have no fs.FS-based equivalents.

Proposal

ServeFSFile

ServeFile lets the caller easily serve the contents of a single file from the OS file system.

ServeFSFile lets the caller do the same for an fs.FS.

// ServeFSFile replies to the request with the contents
// of the named file or directory from the file system fsys.
//
// If the provided file ... [rest of doc is the same as ServeFile]
func ServeFSFile(w ResponseWriter, r *Request, fsys fs.FS, name string)

Both of these functions take a filename. The name passed to ServeFile is OS-specific; the name passed to ServeFSFile follows the io/fs convention (slash-separated paths).

ServeFSContent

ServeContent is a lower-level function intended to serve the content of any file-like object. Unfortunately, it is not compatible with io/fs.

ServeContent takes an io.ReadSeeker; seeking is used to determine the size of the file. An fs.File is not (necessarily) a Seeker. However, the fs.FileInfo interface provides the file's size as well as name and modification time.

Therefore, instead of

name string, modtime time.Time, content io.ReadSeeker

we can pass in

info fs.FileInfo, content io.Reader

The behavior of ServeFSContent is otherwise the same as ServeContent:

// ServeFSContent replies to the request using the content in the
// provided Reader. The main benefit of ServeFSContent over io.Copy
// is that it handles Range requests properly, sets the MIME type, and
// handles If-Match, If-Unmodified-Since, If-None-Match, If-Modified-Since,
// and If-Range requests.
//
// ServeFSContent uses info to learn the file's name, modification time, and size.
// The size must be accurate but other attributes may have zero values.
//
// If the response's Content-Type header is not set, ServeFSContent
// first tries to deduce the type from name's file extension and,
// if that fails, falls back to reading the first block of the content
// and passing it to DetectContentType.
// The name is otherwise unused; in particular it can be empty and is
// never sent in the response.
//
// If the modification time is not the zero time or Unix epoch,
// ServeFSContent includes it in a Last-Modified header in the response.
// If the request includes an If-Modified-Since header, ServeFSContent uses
// the modification time to decide whether the content needs to be sent at all.
//
// If the caller has set w's ETag header formatted per RFC 7232, section 2.3,
// ServeFSContent uses it to handle requests using If-Match, If-None-Match,
// or If-Range.
func ServeFSContent(w ResponseWriter, r *Request, info fs.FileInfo, content io.Reader)

Questions

Should these functions instead be implemented outside the standard library?

It is not trivial to implement these functions outside of net/http. The proposed functions are building blocks upon which other functionality can be built; it is not possible to write these functions simply in terms of the existing net/http API.

The ServeFile and ServeContent functions do quite a lot of subtle work (path cleaning, redirects, translating OS errors to HTTP responses, content-type sniffing, and more). Implementing this proposal outside of net/http requires either copying a lot of its internal code or reimplementing a good amount of functionality (some of which comes with security implications).

I believe that we should add these proposed functions to net/http so that it supports io/fs just as well as it supports OS files.

Should ServeFSContent have a different signature?

We could simplify the signature of ServeFSContent by having it take an fs.File:

func ServeFSContent(w ResponseWriter, r *Request, f fs.File)

and then ServeFSContent would call f.Stat itself.

That's not entirely satisfying; it seems to be unusual to pass an fs.File around separately from an fs.FS, and Close is not used.

Another option would be to pass in all the fields explicitly. (This is the same as ServeContent except that instead of a ReadSeeker we pass in the size.) Since this is now not io/fs-specific at all, I gave it a new name:

func ServeReader(w http.ResponseWriter, r *Request, name string, modtime time.Time, size int64, content io.Reader)
@ianlancetaylor
Copy link
Contributor

CC @neild @bradfitz

@ianlancetaylor ianlancetaylor added this to Incoming in Proposals (old) Mar 26, 2022
@earthboundkid
Copy link
Contributor

When I wanted the equivalent of ServeFSFile, I bodged it together by converting the fs.FS to an http.FileSystem with http.FS, and then calling http.ServeContent with the resulting http.File objects. It would have been easier if I could have skipped all that.

@rsc
Copy link
Contributor

rsc commented Mar 30, 2022

There are two suggestions here.

(1) Add ServeFSContent(w, r, info, content) next to ServeContent(w, r, name, modtime, content), where info replaces name+modtime and content is loosened from ReadSeeker to Reader.

This seems like a mistake to me. In particular, ReadSeeker is not only there for finding the size. It is also there to serve range requests, and your clients will be very unhappy if you are serving large files and can't seek to serve the range requests. So I don't think we should change Reader to ReadSeeker. That leaves only name+modtime, and writing info.Name(), info.ModTime() instead of info seems like a small price to pay for having just one function instead of two.

(2) Add ServeFSFile(w, r, fsys, name) next to ServeFile(w, r, name). The alternative today is to do the open yourself and then call ServeContent.

This seems like a more plausible place where we could make things better. I would suggest calling it ServeFS instead of ServeFSFile though.

@rsc rsc moved this from Incoming to Active in Proposals (old) Mar 30, 2022
@rsc
Copy link
Contributor

rsc commented Mar 30, 2022

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

@earthboundkid
Copy link
Contributor

Based on the name, I would expect http.ServeFS to be the equivalent of http.FileServer(http.FS(fsys)). ServeFSFile is a bit clearer.

@rsc rsc changed the title proposal: net/http: Add ServeFSFile and ServeFSContent proposal: net/http: add ServeFSFile Apr 6, 2022
@rsc
Copy link
Contributor

rsc commented Apr 6, 2022

Retitled to be just about ServeFSFile. Does anyone object to adding that?

@neild
Copy link
Contributor

neild commented Apr 6, 2022

ServeFSFile confuses the http package's interactions with files and filesystems even more.

Right now, we have http.FileSystem, http.Dir, and http.File types, http.FileServer and http.NewFileTransport functions which operate on these types, and http.FS to adapt a fs.FS to a http.FileSystem. The need for an adapter is an unfortunate legacy of http predating the fs package, but the general pattern is that http package functions operate on FileSystem.

If we add ServeFSFile, we now have some http package functions that operate on an http.FileSystem and some that operate on an fs.FS. I don't see a coherent explanation aside from historical path dependency for why you will use an adapter to pass a fs.FS to http.FileServer but pass a fs.FS directly to http.ServeFSFile

I think if we add http.ServeFSFile we should also add http.FSFileServer and http.NewFSFileTransport (taking an fs.FS) and eventually deprecate everything related to http.FileSystem.

@cespare
Copy link
Contributor Author

cespare commented Apr 8, 2022

@rsc

This seems like a mistake to me. In particular, ReadSeeker is not only there for finding the size. It is also there to serve range requests, and your clients will be very unhappy if you are serving large files and can't seek to serve the range requests. So I don't think we should change Reader to ReadSeeker.

fs.FS does not say that files need be seekable, and so I'd been approaching this proposal from the standpoint that accomodating fs.FS meant not relying on Seek. If you use http.FS to construct an ioFS, the file serving does not rely on Seek to discover the file size; it's only when it needs to sniff the file content or satisfy a range request that the lack of seekability would cause the request to fail. However, if you want to use http.ServeContent with an fs.File, this option is not available -- you would have to type assert to io.ReadSeeker from the jump.

(2) Add ServeFSFile(w, r, fsys, name) next to ServeFile(w, r, name). The alternative today is to do the open yourself and then call ServeContent.

Yeah, as long as you unconditionally type assert to a ReadSeeker.

Reading between the lines of these comments, it sounds to me like you're saying it's reasonable that code which wants to serve fs.FS files (whether that is in net/http or not) can require that the files from the FS are seekable. Have I got that right? (This means you couldn't use zip.Reader for any of these purposes.)

For some reason, when I wrote this proposal I had it in my head that embed.FS files are not seekable (which is not the case). So I'm basically fine with saying that only seekable file systems are allowed here, in which case:

  • ServeFSContent isn't needed, since we will just do ServeContent(w, r, name, modtime, f.(io.ReadSeeker))
  • ServeFSFile will say that its fs.FS argument must yield seekable files

I updated the proposal description.

@cespare
Copy link
Contributor Author

cespare commented Apr 8, 2022

@neild that direction sounds great to me. (This proposal came out of some exercises where I wrote a bunch of code that uses both net/http and io/fs but tries to avoid mentioning http.File{System} at all. To my mind, they're inherently deprecated due to the existence of io/fs, even if they aren't marked as such.)

To me, adding ServeFSFile feels like a straightforward addition that moves us in the right direction and shouldn't add confusion. This particular function is an alternative to ServeFile which uses the OS file system; it is not a duplicate or equivalent of any http.File-using function.

If we add ServeFSFile, we now have some http package functions that operate on an http.File and some that operate on an fs.File. I don't see a coherent explanation aside from historical path dependency for why you will use an adapter to pass a fs.FS to http.FileServer but pass a fs.File directly to http.ServeFSFile

I think I see what you're getting at, but to be precise, there are no http package functions that take http.Files and ServeFSFile takes an fs.FS and a name, not an fs.File. It is a direct alternative to ServeFile, not to FileServer.

@rsc
Copy link
Contributor

rsc commented May 11, 2022

I need to reread all this and think more carefully about it. It's clear there's a can of worms we should try to avoid opening.

@Merovius
Copy link
Contributor

In particular, ReadSeeker is not only there for finding the size. It is also there to serve range requests, and your clients will be very unhappy if you are serving large files and can't seek to serve the range requests.

IMO Range requests are not required by the standard and if the server does not advertise itself as satisfying Range requests, the clients have no reason to be unhappy. And if I only have an io.Reader, I'd much rather have the machinery for caching without the Range requests, than having neither. Even the Content-Type sniffing could easily be solved by buffering the first 512 bytes of an io.Reader.

IMO the machinery ServeContent provides - even without the optional Range requests - is useful and complex enough to justify being as broad as possible in the inputs we accept. And it's not like an io.Reader couldn't be type-asserted to io.ReadSeeker (or io.ReaderAt, while we're at it) so content which can support Range requests does so.

@Merovius
Copy link
Contributor

Merovius commented Jul 27, 2022

While we're at it, I'm also inclined to think that net/httpshouldn't do Content-Type sniffing by itself in any case. AIUI setting the Content-Type header is most useful in cases where the server knows the correct Content-Type and wants to prevent the client from guessing incorrectly. But if it's not known, there is no reason to believe the server's guess is any better than the client's.

I understand that we can't change ServeContent, but if we add new, similar APIs, I'd be in favor of at least considering to drop this behavior.

[edit] For context, RFC 7231 says about Content-Header:

A sender that generates a message containing a payload body SHOULD
generate a Content-Type header field in that message unless the
intended media type of the enclosed representation is unknown to the
sender
. If a Content-Type header field is not present, the recipient
MAY either assume a media type of "application/octet-stream"
(RFC2046, Section 4.5.1) or examine the data to determine its type.

(emphasis mine)

Which IMO both supports the claim that Content-Type is optional, especially if the server does not know it specifically. [/edit]

@rsc
Copy link
Contributor

rsc commented Oct 26, 2022

ServeFSFile confuses the http package's interactions with files and filesystems even more.

Right now, we have http.FileSystem, http.Dir, and http.File types, http.FileServer and http.NewFileTransport functions which operate on these types, and http.FS to adapt a fs.FS to a http.FileSystem. The need for an adapter is an unfortunate legacy of http predating the fs package, but the general pattern is that http package functions operate on FileSystem.

If we add ServeFSFile, we now have some http package functions that operate on an http.FileSystem and some that operate on an fs.FS. I don't see a coherent explanation aside from historical path dependency for why you will use an adapter to pass a fs.FS to http.FileServer but pass a fs.FS directly to http.ServeFSFile

I think if we add http.ServeFSFile we should also add http.FSFileServer and http.NewFSFileTransport (taking an fs.FS) and eventually deprecate everything related to http.FileSystem.

Sorry for the delay. I was confused by the comment, and it took me a while to page everything to understand it. In doing so I realized my confusion was caused by some typos that I have corrected in the original and in this quote. (They were mentions of http.File and fs.File that should have been http.FileSystem and fs.FS.)

I agree with the comment, now that I understand it. For naming I think we should put the FS at the end of the name like we did in template.ParseFS, os.DirFS, and so on. So that would be http.ServeFileFS, http.FileServerFS, and http.NewFileTransportFS. Specifically:

package http

func ServeFileFS(w ResponseWriter, r *Request, fsys fs.FS, name string)
func FileServerFS(root fs.FS) Handler
func NewFileTransportFS(fsys fs.FS) RoundTripper

Thoughts?

@cespare
Copy link
Contributor Author

cespare commented Oct 27, 2022

@rsc that sounds great to me.

My original proposal was trying to be as minimal as possible and only ServeFileFS requires more than a line or to implement on top of what's in net/http today. But adding all of the necessary parts so that http.FileSystem and friends can be deprecated (now or in the future) is even better.

@rsc rsc changed the title proposal: net/http: add ServeFSFile proposal: net/http: add ServeFileFS, FileServerFS, NewFileTransportFS Nov 2, 2022
@rsc
Copy link
Contributor

rsc commented Nov 2, 2022

Then the new API is:

package http

func ServeFileFS(w ResponseWriter, r *Request, fsys fs.FS, name string)
func FileServerFS(root fs.FS) Handler
func NewFileTransportFS(fsys fs.FS) RoundTripper

Retitled. Does anyone object to adding this API?

@tdakkota
Copy link

tdakkota commented Nov 2, 2022

I agree with the comment, now that I understand it. For naming I think we should put the FS at the end of the name like we did in template.ParseFS, os.DirFS, and so on. So that would be http.ServeFileFS, http.FileServerFS, and http.NewFileTransportFS.

I think it is better to move these functions to separate package, like net/http/httpfs or io/fs/httpfs.

Then:

package httpfs

func ServeFile(w http.ResponseWriter, r *http.Request, fsys fs.FS, name string)
func FileServer(root fs.FS) http.Handler
func NewFileTransport(fsys fs.FS) http.RoundTripper

@rsc
Copy link
Contributor

rsc commented Nov 9, 2022

We're not going to move just these three into a separate package. http.FS is already in net/http.

@rsc
Copy link
Contributor

rsc commented Nov 9, 2022

Based on the discussion above, this proposal seems like a likely accept.
— rsc for the proposal review group

@rsc
Copy link
Contributor

rsc commented Nov 16, 2022

No change in consensus, so accepted. 🎉
This issue now tracks the work of implementing the proposal.
— rsc for the proposal review group

@rsc rsc changed the title proposal: net/http: add ServeFileFS, FileServerFS, NewFileTransportFS net/http: add ServeFileFS, FileServerFS, NewFileTransportFS Nov 16, 2022
@rsc rsc modified the milestones: Proposal, Backlog Nov 16, 2022
@earthboundkid
Copy link
Contributor

Is anyone working on this for 1.21?

@cespare
Copy link
Contributor Author

cespare commented Mar 22, 2023

@carlmjohnson It's on my todo list but I'm having trouble finding the time. You (or anyone else) should feel free to take it if you want.

mauri870 added a commit to mauri870/go that referenced this issue Jul 28, 2023
These new apis are analogous to ServeFile, FileServer and
NewFileTransport respectively.

Fixes golang#51971
mauri870 added a commit to mauri870/go that referenced this issue Jul 28, 2023
These new apis are analogous to ServeFile, FileServer and
NewFileTransport respectively. The main difference is that these
functions operate on an fs.FS.

Fixes golang#51971
mauri870 added a commit to mauri870/go that referenced this issue Jul 28, 2023
These new apis are analogous to ServeFile, FileServer and
NewFileTransport respectively. The main difference is that these
functions operate on an fs.FS.

Fixes golang#51971
@gopherbot
Copy link

Change https://go.dev/cl/513956 mentions this issue: net/http: add ServeFileFS, FileServerFS, NewFileTransportFS

@gopherbot
Copy link

Change https://go.dev/cl/549198 mentions this issue: doc/go1.22: document minor net/http changes

gopherbot pushed a commit that referenced this issue Dec 12, 2023
For #51971
For #61679

Change-Id: Ie7b44201a9c40f5563c6d6051d22ae807ad0480d
Reviewed-on: https://go-review.googlesource.com/c/go/+/549198
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Auto-Submit: Damien Neil <dneil@google.com>
Reviewed-by: Mauri de Souza Meneguzzo <mauri870@gmail.com>
Reviewed-by: Roland Shoemaker <roland@golang.org>
ezz-no pushed a commit to ezz-no/go-ezzno that referenced this issue Feb 18, 2024
For golang#51971
For golang#61679

Change-Id: Ie7b44201a9c40f5563c6d6051d22ae807ad0480d
Reviewed-on: https://go-review.googlesource.com/c/go/+/549198
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Auto-Submit: Damien Neil <dneil@google.com>
Reviewed-by: Mauri de Souza Meneguzzo <mauri870@gmail.com>
Reviewed-by: Roland Shoemaker <roland@golang.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Accepted
Development

Successfully merging a pull request may close this issue.

8 participants