replace checkErr with return (#1496)

errors.go

@@ -7,6 +7,19 @@ import (
 	"github.com/imgproxy/imgproxy/v3/ierrors"
 )
 
+// Monitoring error categories
+const (
+	categoryTimeout       = "timeout"
+	categoryImageDataSize = "image_data_size"
+	categoryPathParsing   = "path_parsing"
+	categorySecurity      = "security"
+	categoryQueue         = "queue"
+	categoryDownload      = "download"
+	categoryProcessing    = "processing"
+	categoryIO            = "IO"
+	categoryStreaming     = "streaming"
+)
+
 type (
 	ResponseWriteError   struct{ error }
 	InvalidURLError      string

processing_handler.go

@@ -1,7 +1,6 @@
 package main
 
 import (
-	"context"
 	"errors"
 	"fmt"
 	"io"
@@ -122,17 +121,19 @@ func setCanonical(rw http.ResponseWriter, originURL string) {
 	}
 }
 
-func writeOriginContentLengthDebugHeader(ctx context.Context, rw http.ResponseWriter, originData imagedata.ImageData) {
+func writeOriginContentLengthDebugHeader(rw http.ResponseWriter, originData imagedata.ImageData) error {
 	if !config.EnableDebugHeaders {
-		return
+		return nil
 	}
 
 	size, err := originData.Size()
 	if err != nil {
-		checkErr(ctx, "image_data_size", err)
+		return ierrors.Wrap(err, 0, ierrors.WithCategory(categoryImageDataSize))
 	}
 
 	rw.Header().Set(httpheaders.XOriginContentLength, strconv.Itoa(size))
+
+	return nil
 }
 
 func writeDebugHeaders(rw http.ResponseWriter, result *processing.Result) {
@@ -146,13 +147,13 @@ func writeDebugHeaders(rw http.ResponseWriter, result *processing.Result) {
 	rw.Header().Set(httpheaders.XResultHeight, strconv.Itoa(result.ResultHeight))
 }
 
-func respondWithImage(reqID string, r *http.Request, rw http.ResponseWriter, statusCode int, resultData imagedata.ImageData, po *options.ProcessingOptions, originURL string, originData imagedata.ImageData, originHeaders http.Header) {
+func respondWithImage(reqID string, r *http.Request, rw http.ResponseWriter, statusCode int, resultData imagedata.ImageData, po *options.ProcessingOptions, originURL string, originData imagedata.ImageData, originHeaders http.Header) error {
 	// We read the size of the image data here, so we can set Content-Length header.
 	// This indireclty ensures that the image data is fully read from the source, no
 	// errors happened.
 	resultSize, err := resultData.Size()
 	if err != nil {
-		checkErr(r.Context(), "image_data_size", err)
+		return ierrors.Wrap(err, 0, ierrors.WithCategory(categoryImageDataSize))
 	}
 
 	contentDisposition := httpheaders.ContentDispositionValue(
@@ -183,8 +184,7 @@ func respondWithImage(reqID string, r *http.Request, rw http.ResponseWriter, sta
 		ierr = newResponseWriteError(err)
 
 		if config.ReportIOErrors {
-			sendErr(r.Context(), "IO", ierr)
-			errorreport.Report(ierr, r)
+			return ierrors.Wrap(ierr, 0, ierrors.WithCategory(categoryIO), ierrors.WithShouldReport(true))
 		}
 	}
 
@@ -195,6 +195,8 @@ func respondWithImage(reqID string, r *http.Request, rw http.ResponseWriter, sta
 			"processing_options": po,
 		},
 	)
+
+	return nil
 }
 
 func respondWithNotModified(reqID string, r *http.Request, rw http.ResponseWriter, po *options.ProcessingOptions, originURL string, originHeaders http.Header) {
@@ -211,36 +213,6 @@ func respondWithNotModified(reqID string, r *http.Request, rw http.ResponseWrite
 	)
 }
 
-func sendErr(ctx context.Context, errType string, err error) {
-	send := true
-
-	if ierr, ok := err.(*ierrors.Error); ok {
-		switch ierr.StatusCode() {
-		case http.StatusServiceUnavailable:
-			errType = "timeout"
-		case 499:
-			// Don't need to send a "request cancelled" error
-			send = false
-		}
-	}
-
-	if send {
-		metrics.SendError(ctx, errType, err)
-	}
-}
-
-func sendErrAndPanic(ctx context.Context, errType string, err error) {
-	sendErr(ctx, errType, err)
-	panic(err)
-}
-
-func checkErr(ctx context.Context, errType string, err error) {
-	if err == nil {
-		return
-	}
-	sendErrAndPanic(ctx, errType, err)
-}
-
 func handleProcessing(reqID string, rw http.ResponseWriter, r *http.Request) error {
 	stats.IncRequestsInProgress()
 	defer stats.DecRequestsInProgress()
@@ -263,19 +235,22 @@ func handleProcessing(reqID string, rw http.ResponseWriter, r *http.Request) err
 		signature = path[:signatureEnd]
 		path = path[signatureEnd:]
 	} else {
-		sendErrAndPanic(ctx, "path_parsing", newInvalidURLErrorf(
-			http.StatusNotFound, "Invalid path: %s", path),
+		return ierrors.Wrap(
+			newInvalidURLErrorf(http.StatusNotFound, "Invalid path: %s", path), 0,
+			ierrors.WithCategory(categoryPathParsing),
 		)
 	}
 
 	path = fixPath(path)
 
 	if err := security.VerifySignature(signature, path); err != nil {
-		sendErrAndPanic(ctx, "security", err)
+		return ierrors.Wrap(err, 0, ierrors.WithCategory(categorySecurity))
 	}
 
 	po, imageURL, err := options.ParsePath(path, r.Header)
-	checkErr(ctx, "path_parsing", err)
+	if err != nil {
+		return ierrors.Wrap(err, 0, ierrors.WithCategory(categoryPathParsing))
+	}
 
 	var imageOrigin any
 	if u, uerr := url.Parse(imageURL); uerr == nil {
@@ -295,19 +270,20 @@ func handleProcessing(reqID string, rw http.ResponseWriter, r *http.Request) err
 	metrics.SetMetadata(ctx, metricsMeta)
 
 	err = security.VerifySourceURL(imageURL)
-	checkErr(ctx, "security", err)
+	if err != nil {
+		return ierrors.Wrap(err, 0, ierrors.WithCategory(categorySecurity))
+	}
 
 	if po.Raw {
-		streamOriginImage(ctx, reqID, r, rw, po, imageURL)
-		return nil
+		return streamOriginImage(ctx, reqID, r, rw, po, imageURL)
 	}
 
 	// SVG is a special case. Though saving to svg is not supported, SVG->SVG is.
 	if !vips.SupportsSave(po.Format) && po.Format != imagetype.Unknown && po.Format != imagetype.SVG {
-		sendErrAndPanic(ctx, "path_parsing", newInvalidURLErrorf(
+		return ierrors.Wrap(newInvalidURLErrorf(
 			http.StatusUnprocessableEntity,
 			"Resulting image format is not supported: %s", po.Format,
-		))
+		), 0, ierrors.WithCategory(categoryPathParsing))
 	}
 
 	imgRequestHeader := make(http.Header)
@@ -339,7 +315,7 @@ func handleProcessing(reqID string, rw http.ResponseWriter, r *http.Request) err
 	}
 
 	// The heavy part starts here, so we need to restrict worker number
-	func() {
+	err = func() error {
 		defer metrics.StartQueueSegment(ctx)()
 
 		err = processingSem.Acquire(ctx, 1)
@@ -347,12 +323,21 @@ func handleProcessing(reqID string, rw http.ResponseWriter, r *http.Request) err
 			// We don't actually need to check timeout here,
 			// but it's an easy way to check if this is an actual timeout
 			// or the request was canceled
-			checkErr(ctx, "queue", server.CheckTimeout(ctx))
+			if terr := server.CheckTimeout(ctx); terr != nil {
+				return ierrors.Wrap(terr, 0, ierrors.WithCategory(categoryTimeout))
+			}
+
 			// We should never reach this line as err could be only ctx.Err()
 			// and we've already checked for it. But beter safe than sorry
-			sendErrAndPanic(ctx, "queue", err)
+
+			return ierrors.Wrap(err, 0, ierrors.WithCategory(categoryQueue))
 		}
+
+		return nil
 	}()
+	if err != nil {
+		return err
+	}
 	defer processingSem.Release(1)
 
 	stats.IncImagesInProgress()
@@ -375,7 +360,9 @@ func handleProcessing(reqID string, rw http.ResponseWriter, r *http.Request) err
 
 		if config.CookiePassthrough {
 			downloadOpts.CookieJar, err = cookies.JarFromRequest(r)
-			checkErr(ctx, "download", err)
+			if err != nil {
+				return nil, nil, ierrors.Wrap(err, 0, ierrors.WithCategory(categoryDownload))
+			}
 		}
 
 		return imagedata.DownloadAsync(ctx, imageURL, "source image", downloadOpts)
@@ -398,21 +385,23 @@ func handleProcessing(reqID string, rw http.ResponseWriter, r *http.Request) err
 	default:
 		// This may be a request timeout error or a request cancelled error.
 		// Check it before moving further
-		checkErr(ctx, "timeout", server.CheckTimeout(ctx))
+		if terr := server.CheckTimeout(ctx); terr != nil {
+			return ierrors.Wrap(terr, 0, ierrors.WithCategory(categoryTimeout))
+		}
 
 		ierr := ierrors.Wrap(err, 0)
 		if config.ReportDownloadingErrors {
 			ierr = ierrors.Wrap(ierr, 0, ierrors.WithShouldReport(true))
 		}
 
-		sendErr(ctx, "download", ierr)
-
 		if imagedata.FallbackImage == nil {
-			panic(ierr)
+			return ierr
 		}
 
-		// We didn't panic, so the error is not reported.
-		// Report it now
+		// Just send error
+		metrics.SendError(ctx, categoryDownload, ierr)
+
+		// We didn't return, so we have to report error
 		if ierr.ShouldReport() {
 			errorreport.Report(ierr, r)
 		}
@@ -433,27 +422,33 @@ func handleProcessing(reqID string, rw http.ResponseWriter, r *http.Request) err
 		}
 	}
 
-	checkErr(ctx, "timeout", server.CheckTimeout(ctx))
+	if terr := server.CheckTimeout(ctx); terr != nil {
+		return ierrors.Wrap(terr, 0, ierrors.WithCategory(categoryTimeout))
+	}
 
 	if config.ETagEnabled && statusCode == http.StatusOK {
-		imgDataMatch, terr := etagHandler.SetActualImageData(originData, originHeaders)
-		if terr == nil {
-			rw.Header().Set("ETag", etagHandler.GenerateActualETag())
+		imgDataMatch, eerr := etagHandler.SetActualImageData(originData, originHeaders)
+		if eerr != nil && config.ReportIOErrors {
+			return ierrors.Wrap(eerr, 0, ierrors.WithCategory(categoryIO))
+		}
 
-			if imgDataMatch && etagHandler.ProcessingOptionsMatch() {
-				respondWithNotModified(reqID, r, rw, po, imageURL, originHeaders)
-				return nil
-			}
+		rw.Header().Set("ETag", etagHandler.GenerateActualETag())
+
+		if imgDataMatch && etagHandler.ProcessingOptionsMatch() {
+			respondWithNotModified(reqID, r, rw, po, imageURL, originHeaders)
+			return nil
 		}
 	}
 
-	checkErr(ctx, "timeout", server.CheckTimeout(ctx))
+	if terr := server.CheckTimeout(ctx); terr != nil {
+		return ierrors.Wrap(terr, 0, ierrors.WithCategory(categoryTimeout))
+	}
 
 	if !vips.SupportsLoad(originData.Format()) {
-		sendErrAndPanic(ctx, "processing", newInvalidURLErrorf(
+		return ierrors.Wrap(newInvalidURLErrorf(
 			http.StatusUnprocessableEntity,
 			"Source image format is not supported: %s", originData.Format(),
-		))
+		), 0, ierrors.WithCategory(categoryProcessing))
 	}
 
 	result, err := func() (*processing.Result, error) {
@@ -468,20 +463,31 @@ func handleProcessing(reqID string, rw http.ResponseWriter, r *http.Request) err
 		defer result.OutData.Close()
 	}
 
-	if err != nil {
-		// First, check if the processing error wasn't caused by an image data error
-		checkErr(ctx, "download", originData.Error())
+	// First, check if the processing error wasn't caused by an image data error
+	if originData.Error() != nil {
+		return ierrors.Wrap(originData.Error(), 0, ierrors.WithCategory(categoryDownload))
+	}
 
-		// If it wasn't, than it was a processing error
-		sendErrAndPanic(ctx, "processing", err)
+	// If it wasn't, than it was a processing error
+	if err != nil {
+		return ierrors.Wrap(err, 0, ierrors.WithCategory(categoryProcessing))
 	}
 
-	checkErr(ctx, "timeout", server.CheckTimeout(ctx))
+	if terr := server.CheckTimeout(ctx); terr != nil {
+		return ierrors.Wrap(terr, 0, ierrors.WithCategory(categoryTimeout))
+	}
 
 	writeDebugHeaders(rw, result)
-	writeOriginContentLengthDebugHeader(ctx, rw, originData)
 
-	respondWithImage(reqID, r, rw, statusCode, result.OutData, po, imageURL, originData, originHeaders)
+	err = writeOriginContentLengthDebugHeader(rw, originData)
+	if err != nil {
+		return ierrors.Wrap(err, 0, ierrors.WithCategory(categoryImageDataSize))
+	}
+
+	err = respondWithImage(reqID, r, rw, statusCode, result.OutData, po, imageURL, originData, originHeaders)
+	if err != nil {
+		return err
+	}
 
 	return nil
 }

server/errors.go

@@ -1,6 +1,7 @@
 package server
 
 import (
+	"context"
 	"fmt"
 	"net/http"
 	"time"
@@ -34,11 +35,16 @@ func newRequestCancelledError(after time.Duration) *ierrors.Error {
 		ierrors.WithStatusCode(499),
 		ierrors.WithPublicMessage("Cancelled"),
 		ierrors.WithShouldReport(false),
+		ierrors.WithCategory(categoryTimeout),
 	)
 }
 
 func (e RequestCancelledError) Error() string { return string(e) }
 
+func (e RequestCancelledError) Unwrap() error {
+	return context.Canceled
+}
+
 func newRequestTimeoutError(after time.Duration) *ierrors.Error {
 	return ierrors.Wrap(
 		RequestTimeoutError(fmt.Sprintf("Request was timed out after %v", after)),
@@ -46,11 +52,16 @@ func newRequestTimeoutError(after time.Duration) *ierrors.Error {
 		ierrors.WithStatusCode(http.StatusServiceUnavailable),
 		ierrors.WithPublicMessage("Gateway Timeout"),
 		ierrors.WithShouldReport(false),
+		ierrors.WithCategory(categoryTimeout),
 	)
 }
 
 func (e RequestTimeoutError) Error() string { return string(e) }
 
+func (e RequestTimeoutError) Unwrap() error {
+	return context.DeadlineExceeded
+}
+
 func newInvalidSecretError() error {
 	return ierrors.Wrap(
 		InvalidSecretError{},

server/middlewares.go

@@ -1,7 +1,9 @@
 package server
 
 import (
+	"context"
 	"crypto/subtle"
+	"errors"
 	"fmt"
 	"net/http"
 
@@ -11,6 +13,10 @@ import (
 	"github.com/imgproxy/imgproxy/v3/metrics"
 )
 
+const (
+	categoryTimeout = "timeout"
+)
+
 // WithMetrics wraps RouteHandler with metrics handling.
 func (r *Router) WithMetrics(h RouteHandler) RouteHandler {
 	if !metrics.Enabled() {
@@ -56,6 +62,35 @@ func (r *Router) WithSecret(h RouteHandler) RouteHandler {
 	}
 }
 
+// WithPanic recovers panic and converts it to normal error
+func (r *Router) WithPanic(h RouteHandler) RouteHandler {
+	return func(reqID string, rw http.ResponseWriter, r *http.Request) (retErr error) {
+		defer func() {
+			// try to recover from panic
+			rerr := recover()
+			if rerr == nil {
+				return
+			}
+
+			// abort handler is an exception of net/http, we should simply repanic it.
+			// it will supress the stack trace
+			if rerr == http.ErrAbortHandler {
+				panic(rerr)
+			}
+
+			// let's recover error value from panic if it has panicked with error
+			err, ok := rerr.(error)
+			if !ok {
+				err = fmt.Errorf("panic: %v", err)
+			}
+
+			retErr = err
+		}()
+
+		return h(reqID, rw, r)
+	}
+}
+
 // WithReportError handles error reporting.
 // It should be placed after `WithMetrics`, but before `WithPanic`.
 func (r *Router) WithReportError(h RouteHandler) RouteHandler {
@@ -77,9 +112,13 @@ func (r *Router) WithReportError(h RouteHandler) RouteHandler {
 		// Get the error category
 		errCat := ierr.Category()
 
-		// Report error to metrics (if metrics are disabled, it will be a no-op)
-		// Skip context closed error
-		if ierr.StatusCode() != 499 {
+		// Exception: any context.DeadlineExceeded error is timeout
+		if errors.Is(ierr, context.DeadlineExceeded) {
+			errCat = categoryTimeout
+		}
+
+		// We do not need to send any canceled context
+		if !errors.Is(ierr, context.Canceled) {
 			metrics.SendError(ctx, errCat, err)
 		}
 
@@ -104,32 +143,3 @@ func (r *Router) WithReportError(h RouteHandler) RouteHandler {
 		return nil
 	}
 }
-
-// WithPanic recovers panic and converts it to normal error
-func (r *Router) WithPanic(h RouteHandler) RouteHandler {
-	return func(reqID string, rw http.ResponseWriter, r *http.Request) (retErr error) {
-		defer func() {
-			// try to recover from panic
-			rerr := recover()
-			if rerr == nil {
-				return
-			}
-
-			// abort handler is an exception of net/http, we should simply repanic it.
-			// it will supress the stack trace
-			if rerr == http.ErrAbortHandler {
-				panic(rerr)
-			}
-
-			// let's recover error value from panic if it has panicked with error
-			err, ok := rerr.(error)
-			if !ok {
-				err = fmt.Errorf("panic: %v", err)
-			}
-
-			retErr = err
-		}()
-
-		return h(reqID, rw, r)
-	}
-}

server/timer.go

@@ -44,7 +44,7 @@ func CheckTimeout(ctx context.Context) error {
 		case context.DeadlineExceeded:
 			return newRequestTimeoutError(d)
 		default:
-			return ierrors.Wrap(err, 0)
+			return ierrors.Wrap(err, 0, ierrors.WithCategory(categoryTimeout))
 		}
 	default:
 		return nil

stream.go

@@ -12,6 +12,7 @@ import (
 	"github.com/imgproxy/imgproxy/v3/config"
 	"github.com/imgproxy/imgproxy/v3/cookies"
 	"github.com/imgproxy/imgproxy/v3/httpheaders"
+	"github.com/imgproxy/imgproxy/v3/ierrors"
 	"github.com/imgproxy/imgproxy/v3/imagedata"
 	"github.com/imgproxy/imgproxy/v3/metrics"
 	"github.com/imgproxy/imgproxy/v3/metrics/stats"
@@ -44,7 +45,7 @@ var (
 	}
 )
 
-func streamOriginImage(ctx context.Context, reqID string, r *http.Request, rw http.ResponseWriter, po *options.ProcessingOptions, imageURL string) {
+func streamOriginImage(ctx context.Context, reqID string, r *http.Request, rw http.ResponseWriter, po *options.ProcessingOptions, imageURL string) error {
 	stats.IncImagesInProgress()
 	defer stats.DecImagesInProgress()
 
@@ -65,18 +66,24 @@ func streamOriginImage(ctx context.Context, reqID string, r *http.Request, rw ht
 
 	if config.CookiePassthrough {
 		cookieJar, err = cookies.JarFromRequest(r)
-		checkErr(ctx, "streaming", err)
+		if err != nil {
+			return ierrors.Wrap(err, 0, ierrors.WithCategory(categoryStreaming))
+		}
 	}
 
 	req, err := imagedata.Fetcher.BuildRequest(r.Context(), imageURL, imgRequestHeader, cookieJar)
 	defer req.Cancel()
-	checkErr(ctx, "streaming", err)
+	if err != nil {
+		return ierrors.Wrap(err, 0, ierrors.WithCategory(categoryStreaming))
+	}
 
 	res, err := req.Send()
 	if res != nil {
 		defer res.Body.Close()
 	}
-	checkErr(ctx, "streaming", err)
+	if err != nil {
+		return ierrors.Wrap(err, 0, ierrors.WithCategory(categoryStreaming))
+	}
 
 	for _, k := range streamRespHeaders {
 		vv := res.Header.Values(k)
@@ -127,4 +134,6 @@ func streamOriginImage(ctx context.Context, reqID string, r *http.Request, rw ht
 	if copyerr != nil {
 		panic(http.ErrAbortHandler)
 	}
+
+	return nil
 }