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

proposal: net/http: add ServeFSFile #51971

Open
cespare opened this issue Mar 26, 2022 · 10 comments
Open

proposal: net/http: add ServeFSFile #51971

cespare opened this issue Mar 26, 2022 · 10 comments
Labels
Projects
Milestone

Comments

@cespare
Copy link
Contributor

@cespare cespare commented Mar 26, 2022

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

@ianlancetaylor ianlancetaylor commented Mar 26, 2022

CC @neild @bradfitz

@ianlancetaylor ianlancetaylor added this to Incoming in Proposals Mar 26, 2022
@carlmjohnson
Copy link
Contributor

@carlmjohnson carlmjohnson commented Mar 29, 2022

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 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 Mar 30, 2022
@rsc
Copy link
Contributor

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

@carlmjohnson
Copy link
Contributor

@carlmjohnson carlmjohnson commented Mar 30, 2022

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 rsc commented Apr 6, 2022

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

@neild
Copy link
Contributor

@neild 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.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 if we add http.ServeFSFile we should also add http.FSFileServer and http.NewFSFileTransport (taking an http.FS) and eventually deprecate everything related to http.FileSystem.

@cespare
Copy link
Contributor Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Development

No branches or pull requests

6 participants