Server: Write internal server error on missing write (#57813)

pkg/util/errutil/errhttp/writer.go

@@ -8,12 +8,11 @@ import (
 	"reflect"
 
 	"github.com/grafana/grafana/pkg/infra/log"
-	"github.com/grafana/grafana/pkg/services/contexthandler"
 	"github.com/grafana/grafana/pkg/util/errutil"
 )
 
 var ErrNonGrafanaError = errutil.NewBase(errutil.StatusInternal, "core.MalformedError")
-var defaultLogger = log.New("request-errors")
+var defaultLogger = log.New("requestErrors")
 
 // ErrorOptions is a container for functional options passed to [Write].
 type ErrorOptions struct {
@@ -23,9 +22,9 @@ type ErrorOptions struct {
 
 // Write writes an error to the provided [http.ResponseWriter] with the
 // appropriate HTTP status and JSON payload from [errutil.Error].
-// Write also logs the provided error to either the contextlogger,
+// Write also logs the provided error to either the "request-errors"
-// the "request-errors" logger, or the logger provided as a functional
+// logger, or the logger provided as a functional option using
-// option using [WithLogger].
+// [WithLogger].
 // When passing errors that are not [errors.As] compatible with
 // [errutil.Error], [ErrNonGrafanaError] will be used to create a
 // generic 500 Internal Server Error payload by default, this is
@@ -45,8 +44,8 @@ func Write(ctx context.Context, err error, w http.ResponseWriter, opts ...func(E
 	logError(ctx, gErr, opt)
 
 	pub := gErr.Public()
-	w.WriteHeader(pub.StatusCode)
 	w.Header().Add("Content-Type", "application/json")
+	w.WriteHeader(pub.StatusCode)
 	err = json.NewEncoder(w).Encode(pub)
 	if err != nil {
 		defaultLogger.FromContext(ctx).Error("error while writing error", "error", err)
@@ -68,9 +67,6 @@ func WithLogger(opt ErrorOptions, logger log.Logger) ErrorOptions {
 
 func logError(ctx context.Context, e errutil.Error, opt ErrorOptions) {
 	var logger log.Logger = defaultLogger
-	if reqCtx := contexthandler.FromContext(ctx); reqCtx != nil && reqCtx.Logger != nil {
-		logger = reqCtx.Logger
-	}
 	if opt.logger != nil {
 		logger = opt.logger
 	}

pkg/web/context.go

@@ -22,6 +22,9 @@ import (
 	"net/url"
 	"strconv"
 	"strings"
+
+	"github.com/grafana/grafana/pkg/util/errutil"
+	"github.com/grafana/grafana/pkg/util/errutil/errhttp"
 )
 
 // Context represents the runtime context of current request of Macaron instance.
@@ -34,6 +37,8 @@ type Context struct {
 	template *template.Template
 }
 
+var errMissingWrite = errutil.NewBase(errutil.StatusInternal, "web.missingWrite")
+
 func (ctx *Context) run() {
 	h := http.Handler(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {}))
 	for i := len(ctx.mws) - 1; i >= 0; i-- {
@@ -47,7 +52,11 @@ func (ctx *Context) run() {
 	// This indicates nearly always that a middleware is misbehaving and not calling its next.ServeHTTP().
 	// In rare cases where a blank http.StatusOK without any body is wished, explicitly state that using w.WriteStatus(http.StatusOK)
 	if !rw.Written() {
-		panic("chain did not write HTTP response")
+		errhttp.Write(
+			ctx.Req.Context(),
+			errMissingWrite.Errorf("chain did not write HTTP response: %s", ctx.Req.URL.Path),
+			rw,
+		)
 	}
 }
 

pkg/web/context_test.go

@@ -2,8 +2,11 @@ package web
 
 import (
 	"net/http"
+	"net/http/httptest"
 	"testing"
 
+	"github.com/stretchr/testify/assert"
+
 	"github.com/grafana/grafana/pkg/infra/log"
 )
 
@@ -94,3 +97,17 @@ func TestContext_RemoteAddr(t *testing.T) {
 		})
 	}
 }
+
+func TestContext_noHandler(t *testing.T) {
+	recorder := httptest.NewRecorder()
+
+	method := http.MethodGet
+	c := &Context{
+		Req:  httptest.NewRequest(method, "/", nil),
+		Resp: NewResponseWriter(method, recorder),
+	}
+
+	c.run()
+
+	assert.Equal(t, http.StatusInternalServerError, recorder.Code)
+}