From 6c2d3d7fc027d4825c7f03dcf66020981342b095 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sybren=20A=2E=20St=C3=BCvel?= Date: Wed, 26 Jun 2024 10:25:10 +0200 Subject: [PATCH] Manager: avoid logging error on HTTP disconnect on some API calls Improve the error handling on some worker management API calls, to deal with closed HTTP connections better. A new function, `api_impl.handleConnectionClosed()` can now be called when `errors.Is(err, context.Canceled)`. This will only log at debug level, and send a `419 I'm a Teapot` response to the client. This response will very likely never be seen, as the connection was closed. However, in case this function is called by mistake, this response is unlikely to be accepted by the HTTP client. --- internal/manager/api_impl/api_impl.go | 10 ++++++ internal/manager/api_impl/worker_mgt.go | 41 ++++++++++++++++--------- 2 files changed, 36 insertions(+), 15 deletions(-) diff --git a/internal/manager/api_impl/api_impl.go b/internal/manager/api_impl/api_impl.go index b923d3f8..4349ef01 100644 --- a/internal/manager/api_impl/api_impl.go +++ b/internal/manager/api_impl/api_impl.go @@ -12,6 +12,7 @@ import ( "time" "github.com/labstack/echo/v4" + "github.com/rs/zerolog" "projects.blender.org/studio/flamenco/pkg/api" ) @@ -134,3 +135,12 @@ func sendAPIErrorDBBusy(e echo.Context, message string, args ...interface{}) err e.Response().Header().Set("Retry-After", strconv.FormatInt(seconds, 10)) return e.JSON(code, apiErr) } + +// handleConnectionClosed logs a message and sends a "418 I'm a teapot" response +// to the HTTP client. The response is likely to be seen, as the connection was +// closed. But just in case this function was called by mistake, it's a response +// code that is unlikely to be accepted by the client. +func handleConnectionClosed(e echo.Context, logger zerolog.Logger, logMessage string) error { + logger.Debug().Msg(logMessage) + return e.NoContent(http.StatusTeapot) +} diff --git a/internal/manager/api_impl/worker_mgt.go b/internal/manager/api_impl/worker_mgt.go index a2412fa4..f36411f3 100644 --- a/internal/manager/api_impl/worker_mgt.go +++ b/internal/manager/api_impl/worker_mgt.go @@ -3,6 +3,7 @@ package api_impl // SPDX-License-Identifier: GPL-3.0-or-later import ( + "context" "errors" "net/http" @@ -16,7 +17,10 @@ import ( func (f *Flamenco) FetchWorkers(e echo.Context) error { logger := requestLogger(e) dbWorkers, err := f.persist.FetchWorkers(e.Request().Context()) - if err != nil { + switch { + case errors.Is(err, context.Canceled): + return handleConnectionClosed(e, logger, "fetching all workers") + case err != nil: logger.Error().Err(err).Msg("fetching all workers") return sendAPIError(e, http.StatusInternalServerError, "error fetching workers: %v", err) } @@ -42,18 +46,23 @@ func (f *Flamenco) FetchWorker(e echo.Context, workerUUID string) error { ctx := e.Request().Context() dbWorker, err := f.persist.FetchWorker(ctx, workerUUID) - if errors.Is(err, persistence.ErrWorkerNotFound) { + switch { + case errors.Is(err, persistence.ErrWorkerNotFound): logger.Debug().Msg("non-existent worker requested") return sendAPIError(e, http.StatusNotFound, "worker %q not found", workerUUID) - } - if err != nil { + case errors.Is(err, context.Canceled): + return handleConnectionClosed(e, logger, "fetching task assigned to worker") + case err != nil: logger.Error().Err(err).Msg("fetching worker") return sendAPIError(e, http.StatusInternalServerError, "error fetching worker: %v", err) } dbTask, err := f.persist.FetchWorkerTask(ctx, dbWorker) - if err != nil { - logger.Error().Err(err).Msg("fetching task assigned to worker") + switch { + case errors.Is(err, context.Canceled): + return handleConnectionClosed(e, logger, "fetching task assigned to worker") + case err != nil: + logger.Error().AnErr("cause", err).Msg("fetching task assigned to worker") return sendAPIError(e, http.StatusInternalServerError, "error fetching task assigned to worker: %v", err) } @@ -86,11 +95,11 @@ func (f *Flamenco) DeleteWorker(e echo.Context, workerUUID string) error { // Fetch the worker in order to re-queue its tasks. worker, err := f.persist.FetchWorker(ctx, workerUUID) - if errors.Is(err, persistence.ErrWorkerNotFound) { + switch { + case errors.Is(err, persistence.ErrWorkerNotFound): logger.Debug().Msg("deletion of non-existent worker requested") return sendAPIError(e, http.StatusNotFound, "worker %q not found", workerUUID) - } - if err != nil { + case err != nil: logger.Error().Err(err).Msg("fetching worker for deletion") return sendAPIError(e, http.StatusInternalServerError, "error fetching worker for deletion: %v", err) @@ -105,11 +114,11 @@ func (f *Flamenco) DeleteWorker(e echo.Context, workerUUID string) error { // Actually delete the worker. err = f.persist.DeleteWorker(ctx, workerUUID) - if errors.Is(err, persistence.ErrWorkerNotFound) { + switch { + case errors.Is(err, persistence.ErrWorkerNotFound): logger.Debug().Msg("deletion of non-existent worker requested") return sendAPIError(e, http.StatusNotFound, "worker %q not found", workerUUID) - } - if err != nil { + case err != nil: logger.Error().Err(err).Msg("deleting worker") return sendAPIError(e, http.StatusInternalServerError, "error deleting worker: %v", err) } @@ -145,11 +154,13 @@ func (f *Flamenco) RequestWorkerStatusChange(e echo.Context, workerUUID string) // Fetch the worker. dbWorker, err := f.persist.FetchWorker(e.Request().Context(), workerUUID) - if errors.Is(err, persistence.ErrWorkerNotFound) { + switch { + case errors.Is(err, context.Canceled): + return handleConnectionClosed(e, logger, "fetching worker") + case errors.Is(err, persistence.ErrWorkerNotFound): logger.Debug().Msg("non-existent worker requested") return sendAPIError(e, http.StatusNotFound, "worker %q not found", workerUUID) - } - if err != nil { + case err != nil: logger.Error().Err(err).Msg("fetching worker") return sendAPIError(e, http.StatusInternalServerError, "error fetching worker: %v", err) }