Stop stating files in dir when request context is cancelled

[abdusco]

I am using Caddy to serve a dir using file_server middleware. The directory is hosted on a network drive. So, stating files aren't really cheap, because each stat requires performing a network IO, and it adds up when listing a directory with hundreds of files.

Caddy doesn't pass down the request context to directory listing

caddy/modules/caddyhttp/fileserver/browse.go

Line 85 in b4e28af

listing, err := fsrv.loadDirectoryContents(dir.(fs.ReadDirFile), root, path.Clean(r.URL.Path), repl)

This means if I close the tab before the page loads (before caddy finishes stating all files in directory), caddy keeps on stating and it slows down the disk access for everything else.

caddy/modules/caddyhttp/fileserver/browse.go

Lines 139 to 148 in b4e28af

	func (fsrv *FileServer) loadDirectoryContents(dir fs.ReadDirFile, root, urlPath string, repl *caddy.Replacer) (browseTemplateContext, error) {
	files, err := dir.ReadDir(10000) // TODO: this limit should probably be configurable
	if err != nil && err != io.EOF {
	return browseTemplateContext{}, err
	}
	// user can presumably browse "up" to parent folder if path is longer than "/"
	canGoUp := len(urlPath) > 1
	return fsrv.directoryListing(files, canGoUp, root, urlPath, repl), nil

It should check if context is cancelled, and stop doing needless disk IO.

[abdusco]

I feel like passing request context down to this method and bailing out if it's cancelled should work:

func (fsrv *FileServer) serveBrowse(root, dirPath string, w http.ResponseWriter, r *http.Request, next caddyhttp.Handler) error {
	// ...
	listing, err := fsrv.loadDirectoryContents(r.Context(), ...)
	// ...
}


func (fsrv *FileServer) directoryListing(ctx context.Context, entries []fs.DirEntry, canGoUp bool, root, urlPath string, repl *caddy.Replacer) browseTemplateContext {
	filesToHide := fsrv.transformHidePaths(repl)

	var dirCount, fileCount int
	fileInfos := []fileInfo{}

	for _, entry := range entries {
		if err := ctx.Err(); err != nil {
			break // or return err?
		}

but I'm not sure about how this affects the performance:

files, err := dir.ReadDir(10000) // TODO: this limit should probably be configurable 

How does walking the dir with filepath.WalkDir() compare to this?

[abdusco]

Also, I can take over this issue and submit a PR with the solution I outlined above, unless you have something in mind.

[mholt]

@abdusco Yes, this sounds like a great idea. Would you like to submit a PR then? Your proposal above sounds like how I'd do it. (For some reason I didn't realize you could just check ctx.Err() to see if it was done, I always selected on ctx.Done() but checking the Err() is even easier!)

You could compare using WalkDir() if you want, I don't have a network drive setup handy to test with right now. I'd be curious which one is faster!

[abdusco]

Before the fix:

func (fsrv *FileServer) directoryListing(ctx context.Context, entries []fs.DirEntry, canGoUp bool, root, urlPath string, repl *caddy.Replacer) browseTemplateContext {
	// ...
	go func() {
		select {
		case <-ctx.Done():
			fsrv.logger.Error("listing cancelled")
		}
	}()
	for _, entry := range entries {
		// if err := ctx.Err(); err != nil {
		// 	break
		// }
		
		// ...
	}
	fsrv.logger.Error("listing finished")

2022/10/08 04:38:31.937 ERROR   http.handlers.file_server       listing cancelled
2022/10/08 04:38:59.884 ERROR   http.handlers.file_server       listing finished

Listing continues for ~30s more.

After the fix:

2022/10/08 04:42:55.004 ERROR   http.handlers.file_server       listing finished
2022/10/08 04:42:55.007 ERROR   http.handlers.file_server       listing cancelled

[abdusco]

One more point of optimization, but it's not so straightforward:

Ideally, stat should be performed lazily, and executed after we apply the pagination. However this breaks sorting by size & time, because we wouldn't have those info at hand.

[abdusco]

filepath.WalkDir fs.WalkDir calls dir.ReadDir(-1) internally, so it wouldn't make any difference (or it would be even slower for dirs with >10000 items) if we refactor.

[mholt]

Ah, right, I think I knew that, but forgot.

Thank you for the fix.

I'd be open to paginating the browse listings, if possible. (Could be a future TODO)