Skip to content

fix up timeout middleware #3826

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

fix up timeout middleware #3826

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
73a6df4
fix up timeout middleware
pjebs Oct 28, 2025
3f2e551
remove for loop
pjebs Oct 28, 2025
970c9de
remove should not reach
pjebs Oct 28, 2025
15fd3ea
fix channel
pjebs Oct 28, 2025
9d42576
Merge branch 'main' into fix-timeout-middleware
pjebs Oct 28, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 22 additions & 29 deletions middleware/timeout/timeout.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,11 @@
package timeout

import (
"context"
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot Oct 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

The context package was removed, but it's essential for correctly handling request timeouts and cancellations in Go. The new implementation without it introduces a critical bug (leaked goroutines). Please re-add the context import as it's required for the proper fix I've suggested in my other comment.

"errors"
"time"

"github.com/gofiber/fiber/v3"
utils "github.com/gofiber/utils/v2"
)

// New enforces a timeout for each incoming request. It replaces the request's
Expand All @@ -21,42 +22,34 @@ func New(h fiber.Handler, config ...Config) fiber.Handler {

timeout := cfg.Timeout
if timeout <= 0 {
return runHandler(ctx, h, cfg)
return h(ctx)
}

parent := ctx.Context()
tCtx, cancel := context.WithTimeout(parent, timeout)
ctx.SetContext(tCtx)
defer func() {
cancel()
ctx.SetContext(parent)
err := make(chan error, 1)
go func() {
Copy link
Member

@gaby gaby Oct 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why?

Copy link
Contributor Author

@pjebs pjebs Oct 29, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the timeout occurs first, the go routine will otherwise block forever because there is nothing to receive from the channel. Hence the buffered channel.

err <- h(ctx)
}()

err := runHandler(ctx, h, cfg)

if errors.Is(tCtx.Err(), context.DeadlineExceeded) && err == nil {
select {
case err := <-err:
if err != nil && (len(cfg.Errors) > 0 && isCustomError(err, cfg.Errors)) {
if cfg.OnTimeout != nil {
if toErr := cfg.OnTimeout(ctx); toErr != nil {
return toErr
}
}
return fiber.ErrRequestTimeout
}
return err
case <-time.After(timeout):
if cfg.OnTimeout != nil {
return cfg.OnTimeout(ctx)
err := cfg.OnTimeout(ctx)
ctx.RequestCtx().TimeoutErrorWithResponse(&ctx.RequestCtx().Response)
Copy link
Member

@gaby gaby Oct 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a problem, you have to create a brand new response object not the one from the ctx since it gets recycle.

Copy link
Contributor Author

@pjebs pjebs Oct 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The function TimeoutErrorWithResponse already creates a new response inside

Copy link
Contributor Author

@pjebs pjebs Oct 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://github.com/valyala/fasthttp/blob/v1.68.0/server.go#L1632

return err
}
ctx.RequestCtx().TimeoutErrorWithCode(utils.StatusMessage(fiber.StatusRequestTimeout), fiber.StatusRequestTimeout)
return fiber.ErrRequestTimeout
}
return err
}
}

// runHandler executes the handler and returns fiber.ErrRequestTimeout if it
// sees a deadline exceeded error or one of the custom "timeout-like" errors.
func runHandler(c fiber.Ctx, h fiber.Handler, cfg Config) error {
err := h(c)
if err != nil && (errors.Is(err, context.DeadlineExceeded) || (len(cfg.Errors) > 0 && isCustomError(err, cfg.Errors))) {
if cfg.OnTimeout != nil {
if toErr := cfg.OnTimeout(c); toErr != nil {
return toErr
}
}
return fiber.ErrRequestTimeout
}
return err
}

// isCustomError checks whether err matches any error in errList using errors.Is.
Expand Down
36 changes: 18 additions & 18 deletions middleware/timeout/timeout_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,21 @@ var (
errUnrelated = errors.New("unmatched error")
)

// runHandler executes the handler and returns fiber.ErrRequestTimeout if it
// sees a deadline exceeded error or one of the custom "timeout-like" errors.
func runHandler(c fiber.Ctx, h fiber.Handler, cfg Config) error {
err := h(c)
if err != nil && (len(cfg.Errors) > 0 && isCustomError(err, cfg.Errors)) {
if cfg.OnTimeout != nil {
if toErr := cfg.OnTimeout(c); toErr != nil {
return toErr
}
}
return fiber.ErrRequestTimeout
}
return err
}

// sleepWithContext simulates a task that takes `d` time, but returns `te` if the context is canceled.
func sleepWithContext(ctx context.Context, d time.Duration, te error) error {
timer := time.NewTimer(d)
Expand Down Expand Up @@ -155,11 +170,9 @@ func TestTimeout_CustomHandler(t *testing.T) {
app := fiber.New()
called := 0

app.Get("/custom-handler", New(func(c fiber.Ctx) error {
if err := sleepWithContext(c.Context(), 100*time.Millisecond, context.DeadlineExceeded); err != nil {
return err
}
return c.SendString("should not reach")
app.Get("/custom-handler", New(func(_ fiber.Ctx) error {
time.Sleep(100 * time.Millisecond)
return context.DeadlineExceeded
}, Config{
Timeout: 20 * time.Millisecond,
OnTimeout: func(c fiber.Ctx) error {
Expand All @@ -175,19 +188,6 @@ func TestTimeout_CustomHandler(t *testing.T) {
require.Equal(t, 1, called)
}

// TestRunHandler_DefaultOnTimeout ensures context.DeadlineExceeded triggers ErrRequestTimeout.
func TestRunHandler_DefaultOnTimeout(t *testing.T) {
app := fiber.New()
ctx := app.AcquireCtx(&fasthttp.RequestCtx{})
defer app.ReleaseCtx(ctx)

err := runHandler(ctx, func(_ fiber.Ctx) error {
return context.DeadlineExceeded
}, Config{})

require.Equal(t, fiber.ErrRequestTimeout, err)
}

// TestRunHandler_CustomOnTimeout verifies that a custom error and OnTimeout handler are used.
func TestRunHandler_CustomOnTimeout(t *testing.T) {
app := fiber.New()
Expand Down
Loading