fileserver: Don't persist parsed template (fix #4202)

Templates are parsed at request-time (like they are in the templates middleware) to allow live changes to the template while the server is running. Fixes race condition.

Also refactored use of a buffer so a buffer put back in the pool will not continue to be used (written to client) in the meantime.

A couple of benchmarks removed due to refactor, which is fine, since we know pooling helps here.

modules/caddyhttp/fileserver/browse.go

@@ -22,6 +22,7 @@ import (
 	"os"
 	"path"
 	"strings"
+	"sync"
 	"text/template"
 
 	"github.com/caddyserver/caddy/v2"
@@ -34,8 +35,6 @@ import (
 type Browse struct {
 	// Use this template file instead of the default browse template.
 	TemplateFile string `json:"template_file,omitempty"`
-
-	template *template.Template
 }
 
 func (fsrv *FileServer) serveBrowse(root, dirPath string, w http.ResponseWriter, r *http.Request, next caddyhttp.Handler) error {
@@ -75,11 +74,14 @@ func (fsrv *FileServer) serveBrowse(root, dirPath string, w http.ResponseWriter,
 
 	fsrv.browseApplyQueryParams(w, r, &listing)
 
-	// write response as either JSON or HTML
+	buf := bufPool.Get().(*bytes.Buffer)
-	var buf *bytes.Buffer
+	defer bufPool.Put(buf)
+
 	acceptHeader := strings.ToLower(strings.Join(r.Header["Accept"], ","))
+
+	// write response as either JSON or HTML
 	if strings.Contains(acceptHeader, "application/json") {
-		if buf, err = fsrv.browseWriteJSON(listing); err != nil {
+		if err := json.NewEncoder(buf).Encode(listing.Items); err != nil {
 			return caddyhttp.Error(http.StatusInternalServerError, err)
 		}
 		w.Header().Set("Content-Type", "application/json; charset=utf-8")
@@ -98,12 +100,11 @@ func (fsrv *FileServer) serveBrowse(root, dirPath string, w http.ResponseWriter,
 			browseTemplateContext: listing,
 		}
 
-		err = fsrv.makeBrowseTemplate(tplCtx)
+		tpl, err := fsrv.makeBrowseTemplate(tplCtx)
 		if err != nil {
 			return fmt.Errorf("parsing browse template: %v", err)
 		}
-
+		if err := tpl.Execute(buf, tplCtx); err != nil {
-		if buf, err = fsrv.browseWriteHTML(tplCtx); err != nil {
 			return caddyhttp.Error(http.StatusInternalServerError, err)
 		}
 		w.Header().Set("Content-Type", "text/html; charset=utf-8")
@@ -161,7 +162,7 @@ func (fsrv *FileServer) browseApplyQueryParams(w http.ResponseWriter, r *http.Re
 }
 
 // makeBrowseTemplate creates the template to be used for directory listings.
-func (fsrv *FileServer) makeBrowseTemplate(tplCtx *templateContext) error {
+func (fsrv *FileServer) makeBrowseTemplate(tplCtx *templateContext) (*template.Template, error) {
 	var tpl *template.Template
 	var err error
 
@@ -169,33 +170,17 @@ func (fsrv *FileServer) makeBrowseTemplate(tplCtx *templateContext) error {
 		tpl = tplCtx.NewTemplate(path.Base(fsrv.Browse.TemplateFile))
 		tpl, err = tpl.ParseFiles(fsrv.Browse.TemplateFile)
 		if err != nil {
-			return fmt.Errorf("parsing browse template file: %v", err)
+			return nil, fmt.Errorf("parsing browse template file: %v", err)
 		}
 	} else {
 		tpl = tplCtx.NewTemplate("default_listing")
 		tpl, err = tpl.Parse(defaultBrowseTemplate)
 		if err != nil {
-			return fmt.Errorf("parsing default browse template: %v", err)
+			return nil, fmt.Errorf("parsing default browse template: %v", err)
 		}
 	}
 
-	fsrv.Browse.template = tpl
+	return tpl, nil
-
-	return nil
-}
-
-func (fsrv *FileServer) browseWriteJSON(listing browseTemplateContext) (*bytes.Buffer, error) {
-	buf := bufPool.Get().(*bytes.Buffer)
-	defer bufPool.Put(buf)
-	err := json.NewEncoder(buf).Encode(listing.Items)
-	return buf, err
-}
-
-func (fsrv *FileServer) browseWriteHTML(tplCtx *templateContext) (*bytes.Buffer, error) {
-	buf := bufPool.Get().(*bytes.Buffer)
-	defer bufPool.Put(buf)
-	err := fsrv.Browse.template.Execute(buf, tplCtx)
-	return buf, err
 }
 
 // isSymlink return true if f is a symbolic link
@@ -224,3 +209,10 @@ type templateContext struct {
 	templates.TemplateContext
 	browseTemplateContext
 }
+
+// bufPool is used to increase the efficiency of file listings.
+var bufPool = sync.Pool{
+	New: func() interface{} {
+		return new(bytes.Buffer)
+	},
+}

modules/caddyhttp/fileserver/browse_test.go

@@ -1,68 +0,0 @@
-// Copyright 2015 Matthew Holt and The Caddy Authors
-//
-// Licensed under the Apache License, Version 2.0 (the "License");
-// you may not use this file except in compliance with the License.
-// You may obtain a copy of the License at
-//
-//     http://www.apache.org/licenses/LICENSE-2.0
-//
-// Unless required by applicable law or agreed to in writing, software
-// distributed under the License is distributed on an "AS IS" BASIS,
-// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-// See the License for the specific language governing permissions and
-// limitations under the License.
-
-package fileserver
-
-import (
-	"testing"
-	"text/template"
-)
-
-func BenchmarkBrowseWriteJSON(b *testing.B) {
-	fsrv := new(FileServer)
-	listing := browseTemplateContext{
-		Name:     "test",
-		Path:     "test",
-		CanGoUp:  false,
-		Items:    make([]fileInfo, 100),
-		NumDirs:  42,
-		NumFiles: 420,
-		Sort:     "",
-		Order:    "",
-		Limit:    42,
-	}
-	b.ResetTimer()
-
-	for n := 0; n < b.N; n++ {
-		fsrv.browseWriteJSON(listing)
-	}
-}
-
-func BenchmarkBrowseWriteHTML(b *testing.B) {
-	fsrv := new(FileServer)
-	fsrv.Browse = &Browse{
-		TemplateFile: "",
-		template:     template.New("test"),
-	}
-	listing := browseTemplateContext{
-		Name:     "test",
-		Path:     "test",
-		CanGoUp:  false,
-		Items:    make([]fileInfo, 100),
-		NumDirs:  42,
-		NumFiles: 420,
-		Sort:     "",
-		Order:    "",
-		Limit:    42,
-	}
-	tplCtx := &templateContext{
-		browseTemplateContext: listing,
-	}
-	fsrv.makeBrowseTemplate(tplCtx)
-	b.ResetTimer()
-
-	for n := 0; n < b.N; n++ {
-		fsrv.browseWriteHTML(tplCtx)
-	}
-}

modules/caddyhttp/fileserver/staticfiles.go

@@ -15,7 +15,6 @@
 package fileserver
 
 import (
-	"bytes"
 	"fmt"
 	weakrand "math/rand"
 	"mime"
@@ -24,7 +23,6 @@ import (
 	"path/filepath"
 	"strconv"
 	"strings"
-	"sync"
 	"time"
 
 	"github.com/caddyserver/caddy/v2"
@@ -178,7 +176,6 @@ func (fsrv *FileServer) ServeHTTP(w http.ResponseWriter, r *http.Request, next c
 		} else if os.IsPermission(err) {
 			return caddyhttp.Error(http.StatusForbidden, err)
 		}
-		// TODO: treat this as resource exhaustion like with os.Open? Or unnecessary here?
 		return caddyhttp.Error(http.StatusInternalServerError, err)
 	}
 
@@ -555,12 +552,6 @@ func (wr statusOverrideResponseWriter) WriteHeader(int) {
 
 var defaultIndexNames = []string{"index.html", "index.txt"}
 
-var bufPool = sync.Pool{
-	New: func() interface{} {
-		return new(bytes.Buffer)
-	},
-}
-
 const (
 	minBackoff, maxBackoff = 2, 5
 	separator              = string(filepath.Separator)