fiber icon indicating copy to clipboard operation
fiber copied to clipboard

fix up timeout middleware

Open pjebs opened this issue 2 months ago • 8 comments

Description

Fixed timeout middleware

Needs to be checked

Fixes #3394

Changes introduced

List the new features or adjustments introduced in this pull request. Provide details on benchmarks, documentation updates, changelog entries, and if applicable, the migration guide.

  • [ ] Benchmarks: Describe any performance benchmarks and improvements related to the changes.
  • [ ] Documentation Update: Detail the updates made to the documentation and links to the changed files.
  • [ ] Changelog/What's New: Include a summary of the additions for the upcoming release notes.
  • [ ] Migration Guide: If necessary, provide a guide or steps for users to migrate their existing code to accommodate these changes.
  • [ ] API Alignment with Express: Explain how the changes align with the Express API.
  • [ ] API Longevity: Discuss the steps taken to ensure that the new or updated APIs are consistent and not prone to breaking changes.
  • [ ] Examples: Provide examples demonstrating the new features or changes in action.

Type of change

Please delete options that are not relevant.

  • [ ] New feature (non-breaking change which adds functionality)
  • [ ] Enhancement (improvement to existing features and functionality)
  • [ ] Documentation update (changes to documentation)
  • [ ] Performance improvement (non-breaking change which improves efficiency)
  • [ ] Code consistency (non-breaking change which improves code reliability and robustness)

Checklist

Before you submit your pull request, please make sure you meet these requirements:

  • [ ] Followed the inspiration of the Express.js framework for new functionalities, making them similar in usage.
  • [ ] Conducted a self-review of the code and provided comments for complex or critical parts.
  • [ ] Updated the documentation in the /docs/ directory for Fiber's documentation.
  • [ ] Added or updated unit tests to validate the effectiveness of the changes or new features.
  • [ ] Ensured that new and existing unit tests pass locally with the changes.
  • [ ] Verified that any new dependencies are essential and have been agreed upon by the maintainers/community.
  • [ ] Aimed for optimal performance with minimal allocations in the new code.
  • [ ] Provided benchmarks for the new code to analyze and improve upon.

Commit formatting

Please use emojis in commit messages for an easy way to identify the purpose or intention of a commit. Check out the emoji cheatsheet here: CONTRIBUTING.md

pjebs avatar Oct 28 '25 23:10 pjebs

[!WARNING]

Rate limit exceeded

@pjebs has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 9 minutes and 15 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

📥 Commits

Reviewing files that changed from the base of the PR and between 970c9de4d219dae6a419c436c3eb5a6224d8923f and 9d42576f03f91a83336a9740f932266d34f56c4f.

📒 Files selected for processing (2)
  • middleware/timeout/timeout.go (2 hunks)
  • middleware/timeout/timeout_test.go (2 hunks)

[!NOTE]

Other AI code review bot(s) detected

CodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review.

Walkthrough

The timeout middleware was refactored to run handlers in a goroutine and use a select on the handler result channel vs. time.After. On timeout the middleware returns immediately (optionally invoking OnTimeout) and no longer uses context.WithTimeout or cancels the handler.

Changes

Cohort / File(s) Summary
Async timeout redesign
middleware/timeout/timeout.go
Replaced context.WithTimeout approach with goroutine + channel and select on time.After. Handler result is received asynchronously; on timeout the middleware returns immediately and invokes OnTimeout when configured. Removed context import; added time and utils imports; removed prior runHandler wrapper.
Test updates
middleware/timeout/timeout_test.go
Added/modified runHandler implementation used by tests, updated tests to simulate timeouts via time.Sleep and context.DeadlineExceeded, added timer cleanup (defer timer.Stop()), removed obsolete TestRunHandler_DefaultOnTimeout, adjusted TestTimeout_CustomHandler and TestRunHandler_CustomOnTimeout.

Sequence Diagram(s)

sequenceDiagram
    participant Client
    participant Middleware
    participant Handler
    rect rgb(220,255,220)
    Note over Middleware: New async flow (goroutine + select)
    Client->>Middleware: HTTP request
    Middleware->>Handler: start handler in goroutine
    Middleware->>Middleware: select { handler result | time.After(timeout) }
    alt timeout fires first
        Middleware-->>Client: return fiber.ErrRequestTimeout (invoke OnTimeout if set)
        Note right of Handler: handler continues in background (not awaited)
    else handler completes first
        Handler-->>Middleware: result via channel
        Middleware-->>Client: return handler result
    end
    end

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

  • Review concurrency: goroutine spawning, channel buffering/close, potential leaks.
  • Verify OnTimeout invocation paths and error mapping to fiber.ErrRequestTimeout.
  • Confirm non-positive timeout short-circuit and removed context behavior don't break callers.
  • Check tests for reliable timing (flaky/time-dependent assertions).

Possibly related issues

  • #3671 — The change (removing context.WithTimeout and running handlers in goroutines) matches concerns about timeouts not enforcing cancellation; related for validation of design trade-offs.

Possibly related PRs

  • #3720 — Also edits middleware/timeout and changes context/timeout handling; may overlap or conflict.
  • #3604 — Modifies timeout middleware and runHandler behavior; closely related to error/OnTimeout logic.
  • #3275 — Previously refactored timeout/runHandler logic and custom timeout-error detection; likely touches same areas.

Suggested labels

v3, codex

Suggested reviewers

  • sixcolors
  • efectn
  • gaby
  • ReneWerner87

Poem

🐇 I hopped in quick with channels bright and keen,
Goroutines running where contexts had been.
When time runs out I shout, "Return!" with cheer,
OnTimeout nudges — the client gets answer near.
Hop, skip, and code — our requests are swift and seen.

Pre-merge checks and finishing touches

❌ Failed checks (1 warning, 1 inconclusive)
Check name Status Explanation Resolution
Description Check ⚠️ Warning The pull request description is largely incomplete and fails to meet the requirements of the provided template. While the description correctly references the linked issue (#3394), it provides only minimal information ("Fixed timeout middleware" and "Needs to be checked"). The substantive sections of the template—including a detailed description of changes, type of change selection, and all checklist items—remain unfilled. None of the change categories (benchmarks, documentation, changelog, migration guide, API alignment, examples) are addressed, and the type of change classification is not selected. The description appears to be submitted as a template without meaningful content added by the author.
Title Check ❓ Inconclusive The pull request title "fix up timeout middleware" does reference the main component being modified (timeout middleware) and is directly related to the changeset. However, the title uses vague language that lacks specificity about what is actually being fixed. The significant redesign involves changing from synchronous to asynchronous timeout handling with immediate response on timeout expiry, but this fundamental shift is not conveyed in the title. The phrasing "fix up" is generic and doesn't clearly communicate the nature of the change to someone scanning the project history.
✅ Passed checks (3 passed)
Check name Status Explanation
Linked Issues Check ✅ Passed The code changes directly implement the objectives specified in linked issue #3394. The issue requests that the timeout middleware be redesigned to return a timeout error immediately when the deadline expires, rather than waiting for the handler to complete. The changes accomplish this by replacing the synchronous context.WithTimeout approach with an asynchronous model where the handler runs in a goroutine and a select statement waits for either the handler result or a timeout signal. When the timeout occurs, the middleware can now return fiber.ErrRequestTimeout immediately to the client without waiting for handler completion, which is the core objective of the linked issue.
Out of Scope Changes Check ✅ Passed All changes in this pull request are directly scoped to the linked issue #3394. The modifications are limited to two files in the timeout middleware package: timeout.go (implementing the asynchronous redesign) and timeout_test.go (updating tests for the new implementation). No changes appear in unrelated packages, utilities, or features. The changes are focused exclusively on addressing the specific architectural problem identified in the issue, with no extraneous modifications or scope creep detected.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

coderabbitai[bot] avatar Oct 28 '25 23:10 coderabbitai[bot]

Summary of Changes

Hello @pjebs, I'm Gemini Code Assist[^1]! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request significantly refactors the timeout middleware to enhance its robustness and control over request processing. The changes replace the previous context.WithTimeout approach with a more explicit goroutine and select statement, allowing for precise termination and handling of requests that exceed the configured timeout duration. This ensures that Fiber applications can manage request timeouts more reliably, providing consistent error responses and status codes.

Highlights

  • Timeout Mechanism Refactor: The core timeout logic in the middleware has been completely rewritten, moving from context.WithTimeout to a goroutine and select statement with time.After for more explicit and direct timeout management.
  • Dependency Update: The context package import has been removed from middleware/timeout/timeout.go, and time and github.com/gofiber/utils/v2 packages have been added to support the new timeout implementation.
  • Error Handling Improvement: Timeout error handling now directly interacts with the underlying fasthttp.RequestCtx using ctx.RequestCtx().TimeoutErrorWithResponse and ctx.RequestCtx().TimeoutErrorWithCode for more precise control over timeout responses.
  • Code Structure and Testing: The runHandler helper function has been moved from the main middleware file to the test file, indicating it's no longer part of the core middleware logic. Test cases have also been simplified to directly simulate timeouts.
Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help /gemini help Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with :thumbsup: and :thumbsdown: on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

[^1]: Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution.

gemini-code-assist[bot] avatar Oct 28 '25 23:10 gemini-code-assist[bot]

@pjebs There's a race condition happening in the implementation.

gaby avatar Oct 29 '25 00:10 gaby

@gaby I'm not sure if it can be fixed without knowing fasthttp better. Or even betterm the custom error functionality should be removed. It doesn't belong with this implementation.

pjebs avatar Oct 29 '25 01:10 pjebs

When the timeout occurs the handler should not have influence over the response or anything for that matter. The only question is if the handler is permitted to continue or is instantly cancelled (that can be sorted at a later day).

pjebs avatar Oct 29 '25 01:10 pjebs

@pjebs one problem is the response, you have to create a new object.

Look at the fasthttp test case for doing this for example https://github.com/valyala/fasthttp/blob/master/server_test.go#L3148

gaby avatar Oct 29 '25 03:10 gaby

The middleware should work like this:

  1. If handler returns before timeout => operate as per normal.
  2. If timeout occurs before handler returns => return a timeout http response (or custom timeout response). Totally ignore the handler. (Ideally, the handler should also be notified perhaps by context cancellation that it's no longer needed).

The handler returning errors influencing the final response part is creating race conditions.

pjebs avatar Oct 29 '25 12:10 pjebs

there is another timeout middleware PR https://github.com/gofiber/fiber/pull/3680 and this should solve https://github.com/gofiber/fiber/issues/3671

maybe we can combine both in one

ReneWerner87 avatar Oct 29 '25 14:10 ReneWerner87