🚀 feat: Add support for Timeout middleware configuration #3518
Conversation
WalkthroughThe timeout middleware has been refactored to use a new Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant FiberApp
participant TimeoutMiddleware
participant Handler
Client->>FiberApp: Sends HTTP Request
FiberApp->>TimeoutMiddleware: Passes request
TimeoutMiddleware->>TimeoutMiddleware: Check SkipPaths, Next, Routes in Config
alt Path/Next skips timeout
TimeoutMiddleware->>Handler: Call handler directly
else Timeout applies
TimeoutMiddleware->>Handler: Run handler with context timeout
alt Handler completes in time
Handler-->>TimeoutMiddleware: Response
TimeoutMiddleware-->>FiberApp: Pass response
else Timeout occurs
TimeoutMiddleware->>TimeoutMiddleware: Call OnTimeout (if set)
TimeoutMiddleware-->>FiberApp: Return timeout error/response
end
end
FiberApp-->>Client: Responds
Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @gaby, I'm Gemini Code Assist1! 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 refactors the timeout middleware to be fully configurable using a new Config struct. This significantly enhances its flexibility by allowing per-route timeouts, excluding specific paths from timeout enforcement, and providing a custom handler for timeout events. The changes maintain backward compatibility, ensuring existing middleware usage remains functional while offering powerful new customization options.
Highlights
- Configurable Timeout: Introduced a
Configstruct (middleware/timeout/config.go) to provide flexible configuration options for the timeout middleware. - Per-Route Timeouts: Added support for specifying different timeout durations for individual routes using the
Routesfield in theConfigstruct. - Path Exclusion: Implemented the ability to skip the timeout middleware for specific paths using the
SkipPathsfield in theConfigstruct. - Custom Timeout Handler: Allowed users to define a custom
fiber.Handler(OnTimeoutfield inConfig) to be executed when a request times out, instead of the defaultfiber.ErrRequestTimeout. - Backward Compatibility: The
Newfunction (middleware/timeout/timeout.go) was updated to accept theConfigstruct but retains backward compatibility with the previous signature by using variadic arguments and default configuration handling. - Documentation & Testing: Updated documentation (
docs/middleware/timeout.md,docs/whats_new.md) to reflect the new configuration options and added new tests (middleware/timeout_test.go) to cover the new features.
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 in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| 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 issue 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 is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
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.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configureGemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
gaby
changed the title
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3518 +/- ##
==========================================
+ Coverage 83.82% 83.91% +0.08%
==========================================
Files 120 121 +1
Lines 12286 12331 +45
==========================================
+ Hits 10299 10347 +48
+ Misses 1561 1559 -2
+ Partials 426 425 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Code Review
This pull request introduces a configurable timeout middleware, a valuable addition that enhances control over timeout behavior. However, the documentation in whats_new.md needs revision for accuracy regarding backward compatibility, and additional test coverage is suggested for combined configurations.
gaby
changed the title
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
⚠️ Performance Alert ⚠️
Possible performance regression was detected for benchmark.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.50.
| Benchmark suite | Current: 25f2986 | Previous: 670fbd5 | Ratio |
|---|---|---|---|
Benchmark_GenericParseTypeBytes/benchmark_genericParseTypeBytes |
35.06 ns/op 8 B/op 1 allocs/op |
21.74 ns/op 8 B/op 1 allocs/op |
1.61 |
Benchmark_GenericParseTypeBytes/benchmark_genericParseTypeBytes - ns/op |
35.06 ns/op |
21.74 ns/op |
1.61 |
Benchmark_GenericParseTypeString/benchmark_genericParseTypeString#01 |
13.99 ns/op 0 B/op 0 allocs/op |
8.867 ns/op 0 B/op 0 allocs/op |
1.58 |
Benchmark_GenericParseTypeString/benchmark_genericParseTypeString#01 - ns/op |
13.99 ns/op |
8.867 ns/op |
1.58 |
This comment was automatically generated by workflow using github-action-benchmark.
There was a problem hiding this comment.
@gaby. I reviewed your timeout middleware implementation and identified a few small but impactful improvements.
fix: Apply default timeout on negative values and optimize skip path …
gaby
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
middleware/timeout/timeout.go (2)
22-29: Exact-match path skipping only – verify expectations
skipis amap[string]struct{}keyed by the raw request path.
Because no normalisation (e.g.path.Clean, trailing-slash handling, wildcards) is performed,/health/will not match/health,%2Fhealthetc.If the intent is prefix / wildcard skipping, consider using
strings.HasPrefixor a radix tree.
If exact match is deliberate, please document it in the README to avoid surprises.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 24-25: middleware/timeout/timeout.go#L24-L25
Added lines #L24 - L25 were not covered by tests
30-36: Map lookup every request could be avoided
cfg.Routesis consulted for every request (if cfg.Routes != nil { ... }).
For large route maps this is an O(1) lookup but still costs one hash each time even when the majority of routes share the default timeout.If memory allows, you can pre-split the middleware into two instances (default / custom timeout) at registration time or cache the resolved duration inside a
middlewareCtx.
Not critical, just a perf note.middleware/timeout/timeout_test.go (1)
133-145: Test name & comment mismatchThe test is called
TestTimeout_NegativeDurationbut the comment copy-pastes the “zero duration” explanation.
Consider updating the comment so future readers understand what is really being checked.-// TestTimeout_NegativeDuration tests the edge case where the timeout is set to zero. -// Usually this means the request can never exceed a 'deadline' – effectively no timeout. +// TestTimeout_NegativeDuration verifies that a negative timeout is treated the same +// as zero – i.e. no timeout is applied.docs/middleware/timeout.md (1)
7-17: Typo – “passed executions”“If the context passed executions … takes longer”
Perhaps meant “If the wrapped executions” or “If the operations executed within the context”.
Minor wording issue, but docs are user-facing.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~13-~13: The abbreviation “e.g.” (= for example) requires two periods.
Context: ...()`. If the context passed executions (eg. DB ops, Http calls) takes longer than t...(E_G)
[misspelling] ~15-~15: This word is normally spelled with a hyphen.
Context: ...zedErrorHandler. It does not cancel long running executions. Underlying executions must ...(EN_COMPOUNDS_LONG_RUNNING)
docs/whats_new.md (1)
1240-1243: Phrase could be clearer on breaking changeThe note says “existing code must be updated accordingly”, which is correct, but earlier marketing material in the same file mentions “retaining backward compatibility”.
Consider explicitly calling out that thetimeout.Newsignature change is breaking to prevent confusion during migration.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
docs/middleware/timeout.md(4 hunks)docs/whats_new.md(1 hunks)middleware/timeout/config.go(1 hunks)middleware/timeout/timeout.go(1 hunks)middleware/timeout/timeout_test.go(6 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
middleware/timeout/config.go (1)
ctx_interface_gen.go (1)
Ctx(17-379)
🪛 GitHub Check: codecov/patch
middleware/timeout/timeout.go
[warning] 24-25: middleware/timeout/timeout.go#L24-L25
Added lines #L24 - L25 were not covered by tests
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Compare
- GitHub Check: repeated
🔇 Additional comments (1)
middleware/timeout/timeout_test.go (1)
151-160: Missing test forNextskip functionLines 23-25 in
timeout.goare uncovered (Codecov warning).
Add a unit test that setscfg.Nextto returntruefor a specific request and assert that the wrapped handler executes without timeout enforcement.This will both improve coverage and protect against regressions on the
Nextlogic.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
docs/middleware/timeout.md
Outdated
| | Timeout | `time.Duration` | Default timeout for routes. | `0` | | ||
| | OnTimeout | `fiber.Handler` | Handler executed when a timeout occurs. | `nil` | | ||
| | SkipPaths | `[]string` | Paths that should not have a timeout enforced. | `nil` | | ||
| | Routes | `map[string]time.Duration` | Specific timeout values per route path. | `nil` | |
There was a problem hiding this comment.
not sure if we should allow this feature
since the middleware hangs on a certain route, the route is already handled with a timeout config
to have further configurations of the routes in there I see as wrong
also people will want the routes to work with wildcard and other route specific matchings
don't know if we should allow this feature in the middleware config which itself is bound to a route if it already works
f.e.
app.Get("/reports", timeout.New(handler, timeout.Config{
Timeout: 5 * time.Second,
Routes: map[string]time.Duration{
"/reports": 30 * time.Second,
},
}))what added value does the timeout value have if the actual value is in the routes map
same with the skip feature
for this we have the skip middleware
https://docs.gofiber.io/next/middleware/skip
There was a problem hiding this comment.
@ReneWerner87 I understand your points, but I believe the value of this feature lies in the convenience it provides. I prefer configuring everything in a single place rather than spreading it across multiple routes. If the framework itself offered this support natively, even better.
As for the Timeout property, the intention was for it to serve as the default value for routes that don’t specify one. If we decide to move forward with this, we can simply rename it to DefaultTimeout if you think that would make it clearer.
There was a problem hiding this comment.
@ReneWerner87 What if we remove the handler param, and require the middleware to be used with app.Use() ?
There was a problem hiding this comment.
Now I understand better. I thought we were already planning to use it in app.Use() instead of on individual routes, based on the suggested idea:
app.Use(timeout.NewWithConfig(timeout.Config{
Timeout: 5 * time.Second,
PerRoute: map[string]time.Duration{
"/api/reports": 30 * time.Second,
"/api/uploads": 60 * time.Second,
},
}))
@gaby, wouldn’t it be better to keep the original usage below unchanged and just create a separate method, preserving backward compatibility?
app.Get("/foo/:sleepTime", timeout.New(h, 2*time.Second))
There was a problem hiding this comment.
@Andrei-hub11 The main branch is tracking the new major version of GoFiber. Breaking changes are allowed.
There was a problem hiding this comment.
@ReneWerner87, I think using timeout.NewWithConfig() in the same place where a route is defined would feel a bit awkward, as it creates a conceptual ambiguity.
Since changes can be made, and if we ever need specific adjustments for individual route usage, we can simply refactor timeout.New() so that timeout.NewWithConfig() can be used independently with app.Use().
Clarity of purpose:
timeout.New() = "This specific route needs X seconds"
app.Get("/api/reports", timeout.New(30*time.Second))
timeout.NewWithConfig() = "I'm configuring timeouts for the entire application"
app.Use(timeout.NewWithConfig(timeout.Config{
DefaultTimeout: 5 * time.Second,
Routes: map[string]time.Duration{
"/api/videos": 30 * time.Second,
"/api/uploads": 60 * time.Second,
},
SkipPaths: []string{"/health"},
}))
There was a problem hiding this comment.
the one with the routes will not work well, people will then think they can also define dynamic routes not just completely static ones like “/api/user/:userId”
There was a problem hiding this comment.
@ReneWerner87 Man, how about making it handle /api/user/ as matching /api/user/123 and similar paths? That would solve it and be intuitive, as long as it's documented.
app.Use(timeout.NewWithConfig(timeout.Config{
DefaultTimeout: 5 * time.Second,
RoutePrefixes: map[string]time.Duration{
"/api/users/": 15 * time.Second, // matches: /api/users/123, /api/users/456/profile
"/api/reports/": 30 * time.Second, // matches: /api/reports/daily, /api/reports/monthly/2024
"/api/uploads/": 60 * time.Second, // matches: /api/uploads/images, /api/uploads/documents/pdf
"/admin/": 10 * time.Second, // matches: /admin/dashboard, /admin/users/edit
},
SkipPaths: []string{"/health", "/metrics"},
}))
There was a problem hiding this comment.
It is not that simple to implement, it requires using the router within the handler.
There was a problem hiding this comment.
Apologies — I think I may have misunderstood something. Are you saying that something like strings.HasPrefix(c.Path(), prefix) wouldn't be sufficient?
|
Note: Add support for |
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
middleware/timeout/config.go (1)
59-61: Fix missing timeout validation.Based on past review comments, there should be validation for negative timeout values to ensure consistency with route-specific timeouts.
Apply this diff to add timeout validation:
+ if cfg.Timeout < 0 { + cfg.Timeout = ConfigDefault.Timeout + } if cfg.Errors == nil { cfg.Errors = ConfigDefault.Errors }middleware/timeout/timeout.go (1)
27-37: Critical: Timeout context is not propagated to the handler.The timeout context
tCtxis created but the handler is still executed with the originalctx. This means the handler and any downstream code (DB drivers, HTTP clients, etc.) never receive the timeout signal, defeating the main purpose of the middleware.Apply this diff to fix the timeout context propagation:
tCtx, cancel := context.WithTimeout(ctx, timeout) +old := ctx.Context() +ctx.SetContext(tCtx) defer cancel() +defer ctx.SetContext(old) -err := runHandler(ctx, h, cfg) +err := runHandler(ctx, h, cfg)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
docs/middleware/timeout.md(4 hunks)docs/whats_new.md(1 hunks)middleware/timeout/config.go(1 hunks)middleware/timeout/timeout.go(1 hunks)middleware/timeout/timeout_test.go(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- docs/whats_new.md
- docs/middleware/timeout.md
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: sixcolors
PR: gofiber/fiber#3016
File: middleware/session/middleware_test.go:190-191
Timestamp: 2024-10-12T10:01:44.206Z
Learning: When testing session `IdleTimeout` expiration, it's acceptable to use `time.Sleep` to simulate the passage of time in tests.
Learnt from: sixcolors
PR: gofiber/fiber#3016
File: middleware/session/middleware_test.go:190-191
Timestamp: 2024-09-25T17:05:06.991Z
Learning: When testing session `IdleTimeout` expiration, it's acceptable to use `time.Sleep` to simulate the passage of time in tests.
Learnt from: sixcolors
PR: gofiber/fiber#3016
File: middleware/session/config.go:16-26
Timestamp: 2024-10-08T19:06:06.583Z
Learning: In the session middleware `Config` struct, `Store` is backed by `fiber.Storage`; they are different entities serving distinct purposes in session management.
Learnt from: sixcolors
PR: gofiber/fiber#3016
File: middleware/session/config.go:16-26
Timestamp: 2024-09-25T16:17:00.969Z
Learning: In the session middleware `Config` struct, `Store` is backed by `fiber.Storage`; they are different entities serving distinct purposes in session management.
middleware/timeout/timeout_test.go (21)
Learnt from: sixcolors
PR: gofiber/fiber#3016
File: middleware/session/middleware_test.go:190-191
Timestamp: 2024-10-12T10:01:44.206Z
Learning: When testing session `IdleTimeout` expiration, it's acceptable to use `time.Sleep` to simulate the passage of time in tests.
Learnt from: sixcolors
PR: gofiber/fiber#3016
File: middleware/session/middleware_test.go:190-191
Timestamp: 2024-09-25T17:05:06.991Z
Learning: When testing session `IdleTimeout` expiration, it's acceptable to use `time.Sleep` to simulate the passage of time in tests.
Learnt from: sixcolors
PR: gofiber/fiber#3051
File: middleware/session/session.go:215-216
Timestamp: 2024-10-08T19:06:06.583Z
Learning: Parallel tests for `Session.Save` already exist in the `middleware/session/session_test.go` file, specifically in the `Test_Session_Save` and `Test_Session_Save_Expiration` functions.
Learnt from: sixcolors
PR: gofiber/fiber#3051
File: middleware/session/session.go:215-216
Timestamp: 2024-06-30T00:38:06.580Z
Learning: Parallel tests for `Session.Save` already exist in the `middleware/session/session_test.go` file, specifically in the `Test_Session_Save` and `Test_Session_Save_Expiration` functions.
Learnt from: efectn
PR: gofiber/fiber#3162
File: app_test.go:893-895
Timestamp: 2024-11-29T12:37:27.581Z
Learning: In the `Test_App_ShutdownWithContext` function in `app_test.go`, the `clientDone` channel is used to synchronize the client's request completion before proceeding, eliminating the need for additional `time.Sleep` calls.
Learnt from: ReneWerner87
PR: gofiber/fiber#3161
File: app.go:923-932
Timestamp: 2024-11-15T07:56:21.623Z
Learning: In the Fiber framework, breaking changes are acceptable when moving from version 2 to version 3, including modifications to method signatures such as in the `Test` method in `app.go`.
Learnt from: gaby
PR: gofiber/fiber#3193
File: middleware/adaptor/adaptor.go:111-111
Timestamp: 2024-11-10T23:44:13.704Z
Learning: In the `middleware/adaptor/adaptor.go` file of the Fiber framework, when updating context handling, replacing `c.Context()` with `c.RequestCtx()` is appropriate to access the `fasthttp.RequestCtx`.
Learnt from: sixcolors
PR: gofiber/fiber#3016
File: middleware/session/middleware_test.go:400-407
Timestamp: 2024-09-25T17:09:03.756Z
Learning: In the `Test_Session_Next` function in `middleware/session/middleware_test.go`, the variable `doNext` is properly synchronized with the `muNext` mutex for both read and write access.
Learnt from: sixcolors
PR: gofiber/fiber#3016
File: middleware/csrf/csrf_test.go:164-165
Timestamp: 2024-09-25T17:08:07.693Z
Learning: In the `Test_CSRF_WithSession_Middleware` function, calling `session.NewWithStore()` without arguments is acceptable, as the default configuration is sufficient.
Learnt from: sixcolors
PR: gofiber/fiber#3016
File: middleware/csrf/csrf_test.go:164-165
Timestamp: 2024-10-08T19:06:06.583Z
Learning: In the `Test_CSRF_WithSession_Middleware` function, calling `session.NewWithStore()` without arguments is acceptable, as the default configuration is sufficient.
Learnt from: efectn
PR: gofiber/fiber#3162
File: hooks_test.go:228-228
Timestamp: 2024-12-13T08:14:22.851Z
Learning: In Go test files, prefer using the `require` methods from the `testify` package for assertions instead of manual comparisons and calls to `t.Fatal` or `t.Fatalf`.
Learnt from: sixcolors
PR: gofiber/fiber#3016
File: middleware/session/store.go:164-167
Timestamp: 2024-10-02T23:03:31.727Z
Learning: Unit tests in this project use testify require.
Learnt from: sixcolors
PR: gofiber/fiber#3016
File: middleware/session/store.go:164-167
Timestamp: 2024-10-08T19:06:06.583Z
Learning: Unit tests in this project use testify require.
Learnt from: sixcolors
PR: gofiber/fiber#2922
File: middleware/cors/utils.go:63-71
Timestamp: 2024-07-26T21:00:12.902Z
Learning: The project uses the testify/assert package for assertions in unit tests.
Learnt from: sixcolors
PR: gofiber/fiber#2922
File: middleware/cors/utils.go:63-71
Timestamp: 2024-10-08T19:06:06.583Z
Learning: The project uses the testify/assert package for assertions in unit tests.
Learnt from: ckoch786
PR: gofiber/fiber#3230
File: docs/whats_new.md:944-951
Timestamp: 2024-12-15T19:56:45.935Z
Learning: Detailed usage examples and explanations for new methods like `RemoveRoute` and `RemoveRouteByName` are documented in `docs/api/app.md`, so it's unnecessary to duplicate them in `docs/whats_new.md`.
Learnt from: sixcolors
PR: gofiber/fiber#3016
File: middleware/csrf/csrf_test.go:188-193
Timestamp: 2024-09-25T15:57:10.221Z
Learning: In the Fiber framework tests, using `ctx.Response.Header.Cookie` may not be suitable for parsing cookies from the response header, as it requires a `*Cookie` and fills it rather than returning a string value; thus, manual parsing of the `Set-Cookie` header may be necessary.
Learnt from: sixcolors
PR: gofiber/fiber#3016
File: middleware/csrf/csrf_test.go:188-193
Timestamp: 2024-10-08T19:06:06.583Z
Learning: In the Fiber framework tests, using `ctx.Response.Header.Cookie` may not be suitable for parsing cookies from the response header, as it requires a `*Cookie` and fills it rather than returning a string value; thus, manual parsing of the `Set-Cookie` header may be necessary.
Learnt from: mdelapenya
PR: gofiber/fiber#3434
File: app.go:623-636
Timestamp: 2025-05-08T08:14:37.302Z
Learning: In the gofiber/fiber framework, service startup failures should panic rather than allowing the application to continue running with degraded functionality, as this is the agreed-upon design decision.
Learnt from: sixcolors
PR: gofiber/fiber#3016
File: middleware/session/config.go:122-122
Timestamp: 2024-10-08T19:06:06.583Z
Learning: In `DefaultErrorHandler(c *fiber.Ctx, err error)`, since `c` is a pointer to an interface, we need to dereference `*c` when calling interface methods like `SendStatus`.
Learnt from: sixcolors
PR: gofiber/fiber#3016
File: middleware/session/config.go:122-122
Timestamp: 2024-09-25T16:18:34.719Z
Learning: In `DefaultErrorHandler(c *fiber.Ctx, err error)`, since `c` is a pointer to an interface, we need to dereference `*c` when calling interface methods like `SendStatus`.
middleware/timeout/config.go (5)
Learnt from: gaby
PR: gofiber/fiber#3193
File: middleware/adaptor/adaptor.go:111-111
Timestamp: 2024-11-10T23:44:13.704Z
Learning: In the `middleware/adaptor/adaptor.go` file of the Fiber framework, when updating context handling, replacing `c.Context()` with `c.RequestCtx()` is appropriate to access the `fasthttp.RequestCtx`.
Learnt from: sixcolors
PR: gofiber/fiber#3016
File: middleware/session/config.go:16-26
Timestamp: 2024-10-08T19:06:06.583Z
Learning: In the session middleware `Config` struct, `Store` is backed by `fiber.Storage`; they are different entities serving distinct purposes in session management.
Learnt from: sixcolors
PR: gofiber/fiber#3016
File: middleware/session/config.go:16-26
Timestamp: 2024-09-25T16:17:00.969Z
Learning: In the session middleware `Config` struct, `Store` is backed by `fiber.Storage`; they are different entities serving distinct purposes in session management.
Learnt from: sixcolors
PR: gofiber/fiber#3016
File: middleware/session/middleware_test.go:190-191
Timestamp: 2024-10-12T10:01:44.206Z
Learning: When testing session `IdleTimeout` expiration, it's acceptable to use `time.Sleep` to simulate the passage of time in tests.
Learnt from: sixcolors
PR: gofiber/fiber#3016
File: middleware/session/middleware_test.go:190-191
Timestamp: 2024-09-25T17:05:06.991Z
Learning: When testing session `IdleTimeout` expiration, it's acceptable to use `time.Sleep` to simulate the passage of time in tests.
middleware/timeout/timeout.go (7)
Learnt from: gaby
PR: gofiber/fiber#3193
File: middleware/adaptor/adaptor.go:111-111
Timestamp: 2024-11-10T23:44:13.704Z
Learning: In the `middleware/adaptor/adaptor.go` file of the Fiber framework, when updating context handling, replacing `c.Context()` with `c.RequestCtx()` is appropriate to access the `fasthttp.RequestCtx`.
Learnt from: gaby
PR: gofiber/fiber#3193
File: middleware/cache/cache_test.go:897-897
Timestamp: 2024-11-08T04:10:42.990Z
Learning: In the Fiber framework, `Context()` is being renamed to `RequestCtx()`, and `UserContext()` to `Context()` to improve clarity and align with Go's context conventions.
Learnt from: sixcolors
PR: gofiber/fiber#3016
File: middleware/session/config.go:122-122
Timestamp: 2024-10-08T19:06:06.583Z
Learning: In `DefaultErrorHandler(c *fiber.Ctx, err error)`, since `c` is a pointer to an interface, we need to dereference `*c` when calling interface methods like `SendStatus`.
Learnt from: sixcolors
PR: gofiber/fiber#3016
File: middleware/session/config.go:122-122
Timestamp: 2024-09-25T16:18:34.719Z
Learning: In `DefaultErrorHandler(c *fiber.Ctx, err error)`, since `c` is a pointer to an interface, we need to dereference `*c` when calling interface methods like `SendStatus`.
Learnt from: sixcolors
PR: gofiber/fiber#3016
File: middleware/session/middleware_test.go:190-191
Timestamp: 2024-10-12T10:01:44.206Z
Learning: When testing session `IdleTimeout` expiration, it's acceptable to use `time.Sleep` to simulate the passage of time in tests.
Learnt from: sixcolors
PR: gofiber/fiber#3016
File: middleware/session/middleware_test.go:190-191
Timestamp: 2024-09-25T17:05:06.991Z
Learning: When testing session `IdleTimeout` expiration, it's acceptable to use `time.Sleep` to simulate the passage of time in tests.
Learnt from: efectn
PR: gofiber/fiber#3162
File: app_test.go:893-895
Timestamp: 2024-11-29T12:37:27.581Z
Learning: In the `Test_App_ShutdownWithContext` function in `app_test.go`, the `clientDone` channel is used to synchronize the client's request completion before proceeding, eliminating the need for additional `time.Sleep` calls.
🧬 Code Graph Analysis (1)
middleware/timeout/timeout.go (2)
middleware/timeout/config.go (1)
Config(10-34)constants.go (1)
ErrRequestTimeout(124-124)
🪛 GitHub Check: lint
middleware/timeout/config.go
[failure] 33-33:
expected declaration, found 'return' (typecheck)
[failure] 32-32:
expected '}', found 'if' (typecheck)
[failure] 23-23:
expected type, found ':' (typecheck)
[failure] 23-23:
syntax error: unexpected :, expected type (typecheck)
🪛 GitHub Check: govulncheck-check
middleware/timeout/config.go
[failure] 4-4:
"time" imported and not used
[failure] 33-33:
expected declaration, found 'return'
[failure] 32-32:
expected ';', found len
[failure] 32-32:
expected '}', found 'if'
[failure] 23-23:
expected type, found ':'
[failure] 23-23:
expected ';', found ':'
middleware/timeout/timeout.go
[failure] 14-14:
undefined: configDefault
🪛 GitHub Actions: Run govulncheck
middleware/timeout/config.go
[error] 23-23: Syntax error: expected ';', found ':' at line 23.
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: Compare
- GitHub Check: unit (1.24.x, macos-13)
- GitHub Check: Analyse
- GitHub Check: unit (1.24.x, windows-latest)
- GitHub Check: repeated
- GitHub Check: unit (1.24.x, macos-latest)
🔇 Additional comments (3)
middleware/timeout/timeout_test.go (1)
49-124: Excellent test coverage for new configuration options.The tests comprehensively cover the new Config struct functionality including route-specific timeouts, custom handlers, and skip paths. The test structure is well-organized and follows good testing practices.
middleware/timeout/timeout.go (2)
13-25: Excellent refactoring to use Config struct.The middleware refactoring to use a Config struct instead of positional parameters is well-implemented. The conditional logic for skipping middleware and handling zero timeouts is correct.
41-51: Well-implemented timeout error handling.The
runHandlerfunction properly handles both deadline exceeded errors and custom timeout errors, with appropriate fallback to the default timeout error when no custom handler is provided.
| if cfg.Routes == nil { | ||
| cfg.Routes = ConfigDefault.Routes | ||
| } |
There was a problem hiding this comment.
Remove duplicate Routes check.
The code checks cfg.Routes == nil twice (lines 38 and 50-52).
Apply this diff to remove the duplicate check:
- if cfg.Routes == nil {
- cfg.Routes = ConfigDefault.Routes
- }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if cfg.Routes == nil { | |
| cfg.Routes = ConfigDefault.Routes | |
| } |
🤖 Prompt for AI Agents
In middleware/timeout/config.go around lines 50 to 52, there is a duplicate
check for cfg.Routes being nil that also occurs at line 38. Remove the redundant
if statement at lines 50-52 to avoid checking cfg.Routes == nil twice and keep
only the first occurrence at line 38.
| Timeout: 0, | ||
| OnTimeout: nil, | ||
| SkipPaths: nil, | ||
| Routes: nil, | ||
| Errors: nil, | ||
| } |
There was a problem hiding this comment.
Fix critical syntax errors in Config struct and ConfigDefault definition.
The code has several syntax errors that prevent compilation:
- Lines 23-28 contain struct literal syntax inside the Config struct definition
- The
ConfigDefaultvariable is not properly defined
Apply this diff to fix the syntax errors:
+ // Routes defines per-route timeout overrides.
+ // Optional. Default: nil
+ Routes map[string]time.Duration
+
+ // SkipPaths defines paths to skip timeout enforcement.
+ // Optional. Default: nil
+ SkipPaths []string
+
+ // Timeout defines the default timeout duration.
+ // Optional. Default: 0 (no timeout)
+ Timeout time.Duration
+}
+
+// ConfigDefault is the default configuration.
+var ConfigDefault = Config{
+ Next: nil,
- Timeout: 0,
- OnTimeout: nil,
- SkipPaths: nil,
- Routes: nil,
- Errors: nil,
+ OnTimeout: nil,
+ Errors: nil,
+ Routes: nil,
+ SkipPaths: nil,
+ Timeout: 0,
}
<details>
<summary>🧰 Tools</summary>
<details>
<summary>🪛 GitHub Check: lint</summary>
[failure] 23-23:
expected type, found ':' (typecheck)
---
[failure] 23-23:
syntax error: unexpected :, expected type (typecheck)
</details>
<details>
<summary>🪛 GitHub Check: govulncheck-check</summary>
[failure] 23-23:
expected type, found ':'
---
[failure] 23-23:
expected ';', found ':'
</details>
<details>
<summary>🪛 GitHub Actions: Run govulncheck</summary>
[error] 23-23: Syntax error: expected ';', found ':' at line 23.
</details>
</details>
<details>
<summary>🤖 Prompt for AI Agents</summary>
In middleware/timeout/config.go around lines 23 to 28, fix the syntax errors by
removing the struct literal syntax from inside the Config struct definition and
properly define the ConfigDefault variable as a separate variable with a struct
literal. Add missing field declarations with types inside the Config struct,
then define ConfigDefault as a variable of type Config with fields initialized
in a struct literal outside the struct definition.
</details>
<!-- This is an auto-generated comment by CodeRabbit -->
<!-- fingerprinting:phantom:poseidon:panther -->
| app := fiber.New() | ||
|
|
There was a problem hiding this comment.
Remove duplicate app declaration.
There's a duplicate app := fiber.New() declaration that will cause a compilation error.
Apply this diff to fix the duplicate declaration:
- app := fiber.New()
-📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| app := fiber.New() |
🤖 Prompt for AI Agents
In middleware/timeout/timeout_test.go around lines 108 to 109, there is a
duplicate declaration of `app := fiber.New()` which causes a compilation error.
Remove the redundant `app := fiber.New()` declaration so that the variable is
only declared once in the test function.
|
@gaby can you solve the syntax errors |
Summary
Fixes #3514