diff --git a/godoc/page.go b/godoc/page.go index 96f0dbd6..b296b278 100644 --- a/godoc/page.go +++ b/godoc/page.go @@ -5,7 +5,6 @@ package godoc import ( - "log" "net/http" "runtime" ) @@ -31,11 +30,7 @@ func (p *Presentation) ServePage(w http.ResponseWriter, page Page) { page.SearchBox = p.Corpus.IndexEnabled page.Playground = p.ShowPlayground page.Version = runtime.Version() - if err := p.GodocHTML.Execute(w, page); err != nil && err != http.ErrBodyNotAllowed { - // Only log if there's an error that's not about writing on HEAD requests. - // See Issues 5451 and 5454. - log.Printf("GodocHTML.Execute: %s", err) - } + applyTemplateToResponseWriter(w, p.GodocHTML, page) } func (p *Presentation) ServeError(w http.ResponseWriter, r *http.Request, relpath string, err error) { diff --git a/godoc/search.go b/godoc/search.go index 48cdf081..7ba5dc4c 100644 --- a/godoc/search.go +++ b/godoc/search.go @@ -7,7 +7,6 @@ package godoc import ( "bytes" "fmt" - "log" "net/http" "regexp" "strings" @@ -135,9 +134,5 @@ func (p *Presentation) serveSearchDesc(w http.ResponseWriter, r *http.Request) { data := map[string]interface{}{ "BaseURL": fmt.Sprintf("http://%s", r.Host), } - if err := p.SearchDescXML.Execute(w, &data); err != nil && err != http.ErrBodyNotAllowed { - // Only log if there's an error that's not about writing on HEAD requests. - // See Issues 5451 and 5454. - log.Printf("searchDescXML.Execute: %s", err) - } + applyTemplateToResponseWriter(w, p.SearchDescXML, &data) } diff --git a/godoc/server.go b/godoc/server.go index dd3887a5..f152adc3 100644 --- a/godoc/server.go +++ b/godoc/server.go @@ -6,6 +6,7 @@ package godoc import ( "bytes" + "expvar" "fmt" "go/ast" "go/build" @@ -392,6 +393,45 @@ func applyTemplate(t *template.Template, name string, data interface{}) []byte { return buf.Bytes() } +type writerCapturesErr struct { + w io.Writer + err error +} + +func (w *writerCapturesErr) Write(p []byte) (int, error) { + n, err := w.w.Write(p) + if err != nil { + w.err = err + } + return n, err +} + +var httpErrors *expvar.Map + +func init() { + httpErrors = expvar.NewMap("httpWriteErrors").Init() +} + +// applyTemplateToResponseWriter uses an http.ResponseWriter as the io.Writer +// for the call to template.Execute. It uses an io.Writer wrapper to capture +// errors from the underlying http.ResponseWriter. If an error is found, an +// expvar will be incremented. Other template errors will be logged. This is +// done to keep from polluting log files with error messages due to networking +// issues, such as client disconnects and http HEAD protocol violations. +func applyTemplateToResponseWriter(rw http.ResponseWriter, t *template.Template, data interface{}) { + w := &writerCapturesErr{w: rw} + err := t.Execute(w, data) + // There are some cases where template.Execute does not return an error when + // rw returns an error, and some where it does. So check w.err first. + if w.err != nil { + // For http errors, increment an expvar. + httpErrors.Add(w.err.Error(), 1) + } else if err != nil { + // Log template errors. + log.Printf("%s.Execute: %s", t.Name(), err) + } +} + func redirect(w http.ResponseWriter, r *http.Request) (redirected bool) { canonical := pathpkg.Clean(r.URL.Path) if !strings.HasSuffix(canonical, "/") { diff --git a/godoc/server_test.go b/godoc/server_test.go new file mode 100644 index 00000000..9fa411fd --- /dev/null +++ b/godoc/server_test.go @@ -0,0 +1,75 @@ +package godoc + +import ( + "errors" + "expvar" + "net/http" + "net/http/httptest" + "testing" + "text/template" +) + +var ( + // NOTE: with no plain-text in the template, template.Execute will not + // return an error when http.ResponseWriter.Write does return an error. + tmpl = template.Must(template.New("test").Parse("{{.Foo}}")) +) + +type withFoo struct { + Foo int +} + +type withoutFoo struct { +} + +type errResponseWriter struct { +} + +func (*errResponseWriter) Header() http.Header { + return http.Header{} +} + +func (*errResponseWriter) WriteHeader(int) { +} + +func (*errResponseWriter) Write(p []byte) (int, error) { + return 0, errors.New("error") +} + +func TestApplyTemplateToResponseWriter(t *testing.T) { + for _, tc := range []struct { + desc string + rw http.ResponseWriter + data interface{} + expVars int + }{ + { + desc: "no error", + rw: &httptest.ResponseRecorder{}, + data: &withFoo{}, + expVars: 0, + }, + { + desc: "template error", + rw: &httptest.ResponseRecorder{}, + data: &withoutFoo{}, + expVars: 0, + }, + { + desc: "ResponseWriter error", + rw: &errResponseWriter{}, + data: &withFoo{}, + expVars: 1, + }, + } { + httpErrors.Init() + applyTemplateToResponseWriter(tc.rw, tmpl, tc.data) + gotVars := 0 + httpErrors.Do(func(expvar.KeyValue) { + gotVars++ + }) + if gotVars != tc.expVars { + t.Errorf("applyTemplateToResponseWriter(%q): got %d vars, want %d", tc.desc, gotVars, tc.expVars) + } + } +}