~sircmpwn/godocs.io

This thread contains a patchset. You're looking at the original emails, but you may wish to use the patch review UI. Review patch
1

[PATCH gddo] gddo-server: Improve HTTP error handling

Details
Message ID
<20210430194316.18382-1-me@adnano.co>
DKIM signature
pass
Download raw message
Patch: +58 -130
---
Fixes a few cases where errors returned Internal Server Error instead of
a Not Found page.

 gddo-server/api.go            |  16 -----
 gddo-server/http.go           | 131 ++++++++++++++--------------------
 gddo-server/play.go           |   8 +--
 gddo-server/server.go         |   4 --
 internal/httputil/httputil.go |  25 -------
 internal/httputil/static.go   |   4 --
 6 files changed, 58 insertions(+), 130 deletions(-)
 delete mode 100644 internal/httputil/httputil.go

diff --git a/gddo-server/api.go b/gddo-server/api.go
index c9bc613..db08125 100644
--- a/gddo-server/api.go
+++ b/gddo-server/api.go
@@ -52,19 +52,3 @@ func (s *Server) serveAPIImporters(resp http.ResponseWriter, req *http.Request)
	resp.Header().Set("Content-Type", jsonMIMEType)
	return json.NewEncoder(resp).Encode(&data)
}

func serveAPIHome(resp http.ResponseWriter, req *http.Request) error {
	return &httpError{status: http.StatusNotFound}
}

func handleAPIError(resp http.ResponseWriter, req *http.Request, status int, err error) {
	var data struct {
		Error struct {
			Message string `json:"message"`
		} `json:"error"`
	}
	data.Error.Message = http.StatusText(status)
	resp.Header().Set("Content-Type", jsonMIMEType)
	resp.WriteHeader(status)
	json.NewEncoder(resp).Encode(&data)
}
diff --git a/gddo-server/http.go b/gddo-server/http.go
index 402db23..e4960fe 100644
--- a/gddo-server/http.go
+++ b/gddo-server/http.go
@@ -27,25 +27,6 @@ const (
	htmlMIMEType = "text/html; charset=utf-8"
)

type httpError struct {
	status int   // HTTP status code.
	err    error // Optional reason for the HTTP error.
}

func (err *httpError) Error() string {
	if err.err != nil {
		return fmt.Sprintf("status %d, reason %s", err.status, err.err.Error())
	}
	return fmt.Sprintf("Status %d", err.status)
}

func errorText(err error) string {
	if errors.Is(err, context.DeadlineExceeded) {
		return "Timeout getting package files from the version control system."
	}
	return "Internal server error."
}

func (s *Server) HTTPHandler() (http.Handler, error) {
	staticServer := httputil.StaticServer{
		Dir:    s.cfg.AssetsDir,
@@ -61,10 +42,7 @@ func (s *Server) HTTPHandler() (http.Handler, error) {

	handler := func(f func(http.ResponseWriter, *http.Request) error) http.Handler {
		return requestCleaner{
			h: errorHandler{
				fn:    f,
				errFn: s.handleError,
			},
			h: s.errorHandler(f),
		}
	}
	mux.Handle("/-/about", handler(s.serveAbout))
@@ -180,7 +158,7 @@ func (s *Server) servePackage(resp http.ResponseWriter, req *http.Request) error
		select {
		case s.importGraphSem <- struct{}{}:
		default:
			return &httpError{status: http.StatusTooManyRequests}
			return errors.New("too many requests")
		}
		defer func() { <-s.importGraphSem }()

@@ -262,7 +240,17 @@ func (s *Server) serveRefresh(resp http.ResponseWriter, req *http.Request) error
		err = ctx.Err()
	}
	if err != nil {
		setFlashMessages(resp, []flashMessage{{ID: "refresh", Args: []string{errorText(err)}}})
		// TODO: Merge this with other error handling?
		var msg string
		if errors.Is(err, context.DeadlineExceeded) {
			msg = "Timeout encountered while fetching module source code."
		} else {
			msg = "Internal server error."
		}
		setFlashMessages(resp, []flashMessage{{
			ID:   "refresh",
			Args: []string{msg},
		}})
	}
	http.Redirect(resp, req, "/"+importPath, http.StatusFound)
	return nil
@@ -341,18 +329,6 @@ func (s *Server) serveOpenSearch(resp http.ResponseWriter, req *http.Request) er
	return s.templates.ExecuteHTTP(resp, "opensearch.xml", http.StatusOK, root)
}

func logError(req *http.Request, err error, rv interface{}) {
	if err != nil {
		var buf bytes.Buffer
		fmt.Fprintf(&buf, "Error serving %s: %v\n", req.URL, err)
		if rv != nil {
			fmt.Fprintln(&buf, rv)
			buf.Write(debug.Stack())
		}
		log.Print(buf.String())
	}
}

type requestCleaner struct {
	h http.Handler
}
@@ -365,17 +341,6 @@ func (rc requestCleaner) ServeHTTP(w http.ResponseWriter, req *http.Request) {
	rc.h.ServeHTTP(w, req2)
}

func (s *Server) handleError(resp http.ResponseWriter, req *http.Request, status int, err error) {
	if msgs := errorMessages(err); msgs != nil {
		s.templates.ExecuteHTML(resp, "notfound.html", status,
			struct{ Messages []flashMessage }{msgs})
		return
	}
	resp.Header().Set("Content-Type", textMIMEType)
	resp.WriteHeader(http.StatusInternalServerError)
	io.WriteString(resp, errorText(err))
}

func errorMessages(err error) []flashMessage {
	switch {
	case errors.Is(err, context.DeadlineExceeded):
@@ -394,37 +359,51 @@ func errorMessages(err error) []flashMessage {
	return nil
}

type errorHandler struct {
	fn    func(resp http.ResponseWriter, req *http.Request) error
	errFn httputil.Error
}

func (eh errorHandler) ServeHTTP(resp http.ResponseWriter, req *http.Request) {
	defer func() {
		if rv := recover(); rv != nil {
			err := errors.New("handler panic")
			logError(req, err, rv)
			eh.errFn(resp, req, http.StatusInternalServerError, err)
func (s *Server) errorHandler(fn func(http.ResponseWriter, *http.Request) error) http.HandlerFunc {
	return func(resp http.ResponseWriter, req *http.Request) {
		defer func() {
			if rv := recover(); rv != nil {
				err := errors.New("handler panic")
				logError(req, err, rv)
				resp.Header().Set("Content-Type", textMIMEType)
				resp.WriteHeader(http.StatusInternalServerError)
				io.WriteString(resp, "Internal server error.")
			}
		}()

		rb := new(httputil.ResponseBuffer)
		err := fn(rb, req)
		if err == nil {
			rb.WriteTo(resp)
			return
		}
	}()

	rb := new(httputil.ResponseBuffer)
	err := eh.fn(rb, req)
	if err == nil {
		rb.WriteTo(resp)
	} else if e, ok := err.(*httpError); ok {
		if e.status >= 500 {
			logError(req, err, nil)
		if errors.Is(err, proxy.ErrNotFound) ||
			errors.Is(err, proxy.ErrInvalidArgument) ||
			errors.Is(err, context.DeadlineExceeded) ||
			errors.Is(err, ErrBlocked) ||
			errors.Is(err, ErrMismatch) ||
			errors.Is(err, ErrNoPackages) {

			msgs := errorMessages(err)
			s.templates.Execute(resp, "notfound.html",
				struct{ Messages []flashMessage }{msgs})
			return
		}
		eh.errFn(resp, req, e.status, e.err)
	} else if errors.Is(err, proxy.ErrNotFound) ||
		errors.Is(err, proxy.ErrInvalidArgument) ||
		errors.Is(err, ErrBlocked) {
		eh.errFn(resp, req, http.StatusNotFound, nil)
	} else if errors.Is(err, context.DeadlineExceeded) {
		eh.errFn(resp, req, http.StatusNotFound, err)
	} else {

		resp.Header().Set("Content-Type", textMIMEType)
		resp.WriteHeader(http.StatusInternalServerError)
		io.WriteString(resp, "Internal server error.")
		logError(req, err, nil)
		eh.errFn(resp, req, http.StatusInternalServerError, err)
	}
}

func logError(req *http.Request, err error, rv interface{}) {
	var buf bytes.Buffer
	fmt.Fprintf(&buf, "Error serving %s: %v\n", req.URL, err)
	if rv != nil {
		fmt.Fprintln(&buf, rv)
		buf.Write(debug.Stack())
	}
	log.Print(buf.String())
}
diff --git a/gddo-server/play.go b/gddo-server/play.go
index 3db1a52..0d5a2d0 100644
--- a/gddo-server/play.go
+++ b/gddo-server/play.go
@@ -7,6 +7,7 @@
package main

import (
	"errors"
	"fmt"
	"io/ioutil"
	"net/http"
@@ -75,13 +76,10 @@ func (s *Server) playURL(pdoc *doc.Package, id string) (string, error) {
				return "", err
			}
			if resp.StatusCode > 399 {
				return "", &httpError{
					status: resp.StatusCode,
					err:    fmt.Errorf("Error from play.golang.org: %s", p),
				}
				return "", fmt.Errorf("Error from play.golang.org: %s", p)
			}
			return fmt.Sprintf("http://play.golang.org/p/%s", p), nil
		}
	}
	return "", &httpError{status: http.StatusNotFound}
	return "", errors.New("example not found")
}
diff --git a/gddo-server/server.go b/gddo-server/server.go
index 66967b4..73ce179 100644
--- a/gddo-server/server.go
+++ b/gddo-server/server.go
@@ -117,10 +117,6 @@ func (s *Server) GetDoc(ctx context.Context, importPath string) (*database.Modul

	select {
	case r := <-ch:
		if r.err != nil && !errors.Is(r.err, proxy.ErrNotFound) &&
			!errors.Is(r.err, proxy.ErrInvalidArgument) {
			log.Printf("Error getting doc for %q: %v", importPath, r.err)
		}
		return r.mod, r.pkg, r.doc, r.err
	case <-ctx.Done():
		log.Printf("Serving %q as not found after timeout getting doc", importPath)
diff --git a/internal/httputil/httputil.go b/internal/httputil/httputil.go
deleted file mode 100644
index a03717c..0000000
--- a/internal/httputil/httputil.go
@@ -1,25 +0,0 @@
// Copyright 2013 The Go Authors. All rights reserved.
//
// Use of this source code is governed by a BSD-style
// license that can be found in the LICENSE file or at
// https://developers.google.com/open-source/licenses/bsd.

// Package httputil is a toolkit for the Go net/http package.
package httputil

import (
	"net"
	"net/http"
)

// StripPort removes the port specification from an address.
func StripPort(s string) string {
	if h, _, err := net.SplitHostPort(s); err == nil {
		s = h
	}
	return s
}

// Error defines a type for a function that accepts a ResponseWriter for
// a Request with the HTTP status code and error.
type Error func(w http.ResponseWriter, r *http.Request, status int, err error)
diff --git a/internal/httputil/static.go b/internal/httputil/static.go
index b926ffe..d4414cb 100644
--- a/internal/httputil/static.go
+++ b/internal/httputil/static.go
@@ -32,10 +32,6 @@ type StaticServer struct {
	// headers.
	MaxAge time.Duration

	// Error specifies the function used to generate error responses. If Error
	// is nil, then http.Error is used to generate error responses.
	Error Error

	mu    sync.Mutex
	etags map[string]string
}
-- 
2.31.1
Details
Message ID
<CB22MLJW31WY.2UI9RIRMG8FDJ@taiga>
In-Reply-To
<20210430194316.18382-1-me@adnano.co> (view parent)
DKIM signature
fail
Download raw message
DKIM signature: fail
Thanks!

To git@git.sr.ht:~sircmpwn/gddo
   dfc2f02..24ba5a5  master -> master
Reply to thread Export thread (mbox)