diff --git a/Makefile b/Makefile index 87f230f4..6097ae19 100644 --- a/Makefile +++ b/Makefile @@ -29,9 +29,7 @@ build: ## Build the Gotenberg's Docker image GOTENBERG_GRACEFUL_SHUTDOWN_DURATION=30s API_PORT=3000 API_PORT_FROM_ENV= -API_READ_TIMEOUT=30s -API_PROCESS_TIMEOUT=30s -API_WRITE_TIMEOUT=30s +API_TIMEOUT=30s API_ROOT_PATH=/ API_TRACE_HEADER=Gotenberg-Trace API_DISABLE_HEALTH_CHECK_LOGGING=false @@ -61,6 +59,7 @@ WEBHOOK_ERROR_DENY_LIST= WEBHOOK_MAX_RETRY=4 WEBHOOK_RETRY_MIN_WAIT=1s WEBHOOK_RETRY_MAX_WAIT=30s +WEBHOOK_CLIENT_TIMEOUT=30s WEBHOOK_DISABLE=false .PHONY: run @@ -72,9 +71,7 @@ run: ## Start a Gotenberg container --gotenberg-graceful-shutdown-duration=$(GOTENBERG_GRACEFUL_SHUTDOWN_DURATION) \ --api-port=$(API_PORT) \ --api-port-from-env=$(API_PORT_FROM_ENV) \ - --api-read-timeout=$(API_READ_TIMEOUT) \ - --api-process-timeout=$(API_PROCESS_TIMEOUT) \ - --api-write-timeout=$(API_WRITE_TIMEOUT) \ + --api-timeout=$(API_TIMEOUT) \ --api-root-path=$(API_ROOT_PATH) \ --api-trace-header=$(API_TRACE_HEADER) \ --api-disable-health-check-logging=$(API_DISABLE_HEALTH_CHECK_LOGGING) \ @@ -104,6 +101,7 @@ run: ## Start a Gotenberg container --webhook-max-retry=$(WEBHOOK_MAX_RETRY) \ --webhook-retry-min-wait=$(WEBHOOK_RETRY_MIN_WAIT) \ --webhook-retry-max-wait=$(WEBHOOK_RETRY_MAX_WAIT) \ + --webhook-client-timeout=$(WEBHOOK_CLIENT_TIMEOUT) \ --webhook-disable=$(WEBHOOK_DISABLE) .PHONY: build-tests diff --git a/pkg/modules/api/api.go b/pkg/modules/api/api.go index 27f35db6..62ec06ed 100644 --- a/pkg/modules/api/api.go +++ b/pkg/modules/api/api.go @@ -30,8 +30,8 @@ func init() { type API struct { port int readTimeout time.Duration - processTimeout time.Duration writeTimeout time.Duration + timeout time.Duration rootPath string traceHeader string disableHealthCheckLogging bool @@ -166,10 +166,20 @@ func (API) Descriptor() gotenberg.ModuleDescriptor { fs.Duration("api-read-timeout", time.Duration(30)*time.Second, "Set the maximum duration allowed to read a complete request, including the body") fs.Duration("api-process-timeout", time.Duration(30)*time.Second, "Set the maximum duration allowed to process a request") fs.Duration("api-write-timeout", time.Duration(30)*time.Second, "Set the maximum duration before timing out writes of the response") + fs.Duration("api-timeout", time.Duration(30)*time.Second, "Set the time limit for requests") fs.String("api-root-path", "/", "Set the root path of the API - for service discovery via URL paths") fs.String("api-trace-header", "Gotenberg-Trace", "Set the header name to use for identifying requests") fs.Bool("api-disable-health-check-logging", false, "Disable health check logging") + var err error + err = multierr.Append(err, fs.MarkDeprecated("api-read-timeout", "use api-timeout instead")) + err = multierr.Append(err, fs.MarkDeprecated("api-process-timeout", "use api-timeout instead")) + err = multierr.Append(err, fs.MarkDeprecated("api-write-timeout", "use api-timeout instead")) + + if err != nil { + panic(fmt.Errorf("create deprecated flags for the api module: %v", err)) + } + return fs }(), New: func() gotenberg.Module { return new(API) }, @@ -180,9 +190,9 @@ func (API) Descriptor() gotenberg.ModuleDescriptor { func (a *API) Provision(ctx *gotenberg.Context) error { flags := ctx.ParsedFlags() a.port = flags.MustInt("api-port") - a.readTimeout = flags.MustDuration("api-read-timeout") - a.processTimeout = flags.MustDuration("api-process-timeout") - a.writeTimeout = flags.MustDuration("api-write-timeout") + a.readTimeout = flags.MustDeprecatedDuration("api-read-timeout", "api-timeout") + a.writeTimeout = flags.MustDeprecatedDuration("api-write-timeout", "api-timeout") + a.timeout = flags.MustDeprecatedDuration("api-process-timeout", "api-timeout") a.rootPath = flags.MustString("api-root-path") a.traceHeader = flags.MustString("api-trace-header") a.disableHealthCheckLogging = flags.MustBool("api-disable-health-check-logging") @@ -274,7 +284,7 @@ func (a *API) Provision(ctx *gotenberg.Context) error { } // Grace duration. - a.gcGraceDuration = a.readTimeout + a.processTimeout + a.writeTimeout + a.gcGraceDuration = a.timeout mods, err = ctx.Modules(new(GarbageCollectorGraceDurationIncrementer)) if err != nil { @@ -378,7 +388,9 @@ func (a *API) Start() error { a.srv.HideBanner = true a.srv.HidePort = true a.srv.Server.ReadTimeout = a.readTimeout - a.srv.Server.WriteTimeout = a.writeTimeout + a.srv.Server.IdleTimeout = a.timeout + // See https://github.com/gotenberg/gotenberg/issues/396. + a.srv.Server.WriteTimeout = a.writeTimeout + a.writeTimeout a.srv.HTTPErrorHandler = httpErrorHandler() // Let's prepare the modules' routes. @@ -402,7 +414,6 @@ func (a *API) Start() error { latencyMiddleware(), rootPathMiddleware(a.rootPath), traceMiddleware(a.traceHeader), - timeoutsMiddleware(a.readTimeout, a.processTimeout, a.writeTimeout), loggerMiddleware(a.logger, disableLoggingForPaths), ) @@ -419,14 +430,14 @@ func (a *API) Start() error { } } - hardTimeout := a.processTimeout + (time.Duration(5) * time.Second) + hardTimeout := a.timeout + (time.Duration(5) * time.Second) // Add the modules' routes and their specific middlewares. for _, route := range a.routes { var middlewares []echo.MiddlewareFunc if route.IsMultipart { - middlewares = append(middlewares, contextMiddleware(a.processTimeout)) + middlewares = append(middlewares, contextMiddleware(a.timeout)) for _, externalMultipartMiddleware := range externalMultipartMiddlewares { middlewares = append(middlewares, externalMultipartMiddleware.Handler) @@ -447,7 +458,7 @@ func (a *API) Start() error { a.srv.GET( fmt.Sprintf("%s%s", a.rootPath, "health"), func() echo.HandlerFunc { - checks := append(a.healthChecks, health.WithTimeout(a.processTimeout)) + checks := append(a.healthChecks, health.WithTimeout(a.timeout)) checker := health.NewChecker(checks...) return echo.WrapHandler(health.NewHandler(checker)) diff --git a/pkg/modules/api/api_test.go b/pkg/modules/api/api_test.go index 5a85c8d0..3900cc77 100644 --- a/pkg/modules/api/api_test.go +++ b/pkg/modules/api/api_test.go @@ -386,7 +386,7 @@ func TestAPI_Provision(t *testing.T) { }, ) }(), - expectGraceDuration: time.Duration(93) * time.Second, + expectGraceDuration: time.Duration(33) * time.Second, expectErr: true, }, { diff --git a/pkg/modules/api/middlewares.go b/pkg/modules/api/middlewares.go index fa0d0855..5beb29a0 100644 --- a/pkg/modules/api/middlewares.go +++ b/pkg/modules/api/middlewares.go @@ -120,25 +120,6 @@ func traceMiddleware(header string) echo.MiddlewareFunc { } } -// timeoutsMiddleware sets the read, process and write timeouts in the -// echo.Context under "readTimeout", "processTimeout" and "writeTimeout". -// -// readTimeout := c.Get("readTimeout").(time.Duration) -// processTimeout := c.Get("processTimeout").(time.Duration) -// writeTimeout := c.Get("writeTimeout").(time.Duration) -func timeoutsMiddleware(readTimeout, processTimeout, writeTimeout time.Duration) echo.MiddlewareFunc { - return func(next echo.HandlerFunc) echo.HandlerFunc { - return func(c echo.Context) error { - c.Set("readTimeout", readTimeout) - c.Set("processTimeout", processTimeout) - c.Set("writeTimeout", writeTimeout) - - // Call the next middleware in the chain. - return next(c) - } - } -} - // loggerMiddleware sets the logger in the echo.Context under "logger" and logs // a synchronous request result. // @@ -218,14 +199,14 @@ func loggerMiddleware(logger *zap.Logger, disableLoggingForPaths []string) echo. // // ctx := c.Get("context").(*api.Context) // cancel := c.Get("cancel").(context.CancelFunc) -func contextMiddleware(processTimeout time.Duration) echo.MiddlewareFunc { +func contextMiddleware(timeout time.Duration) echo.MiddlewareFunc { return func(next echo.HandlerFunc) echo.HandlerFunc { return func(c echo.Context) error { logger := c.Get("logger").(*zap.Logger) // We create a context with a timeout so that underlying processes are // able to stop early and handle correctly a timeout scenario. - ctx, cancel, err := newContext(c, logger, processTimeout) + ctx, cancel, err := newContext(c, logger, timeout) if err != nil { cancel() diff --git a/pkg/modules/api/middlewares_test.go b/pkg/modules/api/middlewares_test.go index 578e3e85..486f72d1 100644 --- a/pkg/modules/api/middlewares_test.go +++ b/pkg/modules/api/middlewares_test.go @@ -222,47 +222,6 @@ func TestTraceMiddleware(t *testing.T) { } } -func TestTimeoutsMiddleware(t *testing.T) { - recorder := httptest.NewRecorder() - request := httptest.NewRequest(http.MethodGet, "/foo", nil) - - srv := echo.New() - srv.HideBanner = true - srv.HidePort = true - - c := srv.NewContext(request, recorder) - - expectReadTimeout := time.Duration(1) * time.Second - expectProcessTimeout := time.Duration(2) * time.Second - expectWriteTimeout := time.Duration(3) * time.Second - - err := timeoutsMiddleware(expectReadTimeout, expectProcessTimeout, expectWriteTimeout)( - func(c echo.Context) error { - return nil - }, - )(c) - - if err != nil { - t.Fatalf("expected no error but got: %v", err) - } - - actualReadTimeout := c.Get("readTimeout").(time.Duration) - actualProcessTimeout := c.Get("processTimeout").(time.Duration) - actualWriteTimeout := c.Get("writeTimeout").(time.Duration) - - if actualReadTimeout != expectReadTimeout { - t.Errorf("expected '%s' but got '%s", expectReadTimeout, actualReadTimeout) - } - - if actualProcessTimeout != expectProcessTimeout { - t.Errorf("expected '%s' but got '%s", expectProcessTimeout, actualProcessTimeout) - } - - if actualWriteTimeout != expectWriteTimeout { - t.Errorf("expected '%s' but got '%s", actualWriteTimeout, expectWriteTimeout) - } -} - func TestLoggerMiddleware(t *testing.T) { for i, tc := range []struct { request *http.Request diff --git a/pkg/modules/webhook/middleware.go b/pkg/modules/webhook/middleware.go index 17588efc..6898f649 100644 --- a/pkg/modules/webhook/middleware.go +++ b/pkg/modules/webhook/middleware.go @@ -143,7 +143,7 @@ func webhookMiddleware(w Webhook) api.Middleware { client: &retryablehttp.Client{ HTTPClient: &http.Client{ - Timeout: c.Get("writeTimeout").(time.Duration), + Timeout: w.clientTimeout, }, RetryMax: w.maxRetry, RetryWaitMin: w.retryMinWait, diff --git a/pkg/modules/webhook/middleware_test.go b/pkg/modules/webhook/middleware_test.go index ec736021..7462e92d 100644 --- a/pkg/modules/webhook/middleware_test.go +++ b/pkg/modules/webhook/middleware_test.go @@ -324,6 +324,7 @@ func TestWebhookMiddlewareAsynchronousProcess(t *testing.T) { maxRetry: 0, retryMinWait: 0, retryMaxWait: 0, + clientTimeout: time.Duration(30) * time.Second, disable: false, } } @@ -397,7 +398,6 @@ func TestWebhookMiddlewareAsynchronousProcess(t *testing.T) { c.Set("traceHeader", "Gotenberg-Trace") c.Set("trace", "foo") c.Set("startTime", time.Now()) - c.Set("writeTimeout", time.Duration(10)*time.Second) ctx := &api.MockContext{Context: &api.Context{}} ctx.SetLogger(zap.NewNop()) diff --git a/pkg/modules/webhook/webhook.go b/pkg/modules/webhook/webhook.go index 4138bbc6..d5b271ab 100644 --- a/pkg/modules/webhook/webhook.go +++ b/pkg/modules/webhook/webhook.go @@ -25,6 +25,7 @@ type Webhook struct { maxRetry int retryMinWait time.Duration retryMaxWait time.Duration + clientTimeout time.Duration disable bool } @@ -55,7 +56,7 @@ func (Webhook) Descriptor() gotenberg.ModuleDescriptor { err = multierr.Append(err, fs.MarkDeprecated("api-disable-webhook", "use webhook-disable instead")) if err != nil { - panic(fmt.Errorf("create deprecated flags for webhook module: %v", err)) + panic(fmt.Errorf("create deprecated flags for the webhook module: %v", err)) } // New flags. @@ -66,6 +67,7 @@ func (Webhook) Descriptor() gotenberg.ModuleDescriptor { fs.Int("webhook-max-retry", 4, "Set the maximum number of retries for the webhook feature") fs.Duration("webhook-retry-min-wait", time.Duration(1)*time.Second, "Set the minimum duration to wait before trying to call the webhook again") fs.Duration("webhook-retry-max-wait", time.Duration(30)*time.Second, "Set the maximum duration to wait before trying to call the webhook again") + fs.Duration("webhook-client-timeout", time.Duration(30)*time.Second, "Set the time limit for requests to the webhook") fs.Bool("webhook-disable", false, "Disable the webhook feature") return fs @@ -84,6 +86,7 @@ func (w *Webhook) Provision(ctx *gotenberg.Context) error { w.maxRetry = flags.MustDeprecatedInt("api-webhook-max-retry", "webhook-max-retry") w.retryMinWait = flags.MustDeprecatedDuration("api-webhook-retry-min-wait", "webhook-retry-min-wait") w.retryMaxWait = flags.MustDeprecatedDuration("api-webhook-retry-min-wait", "webhook-retry-max-wait") + w.clientTimeout = flags.MustDuration("webhook-client-timeout") w.disable = flags.MustDeprecatedBool("api-disable-webhook", "webhook-disable") return nil