🚀 feat: add Load-Shedding Middleware for Request Timeout Management #3264
Conversation
ErfanMomeniii
requested review from
gaby,
sixcolors,
ReneWerner87 and
efectn
and removed request for
a team
|
Thanks for opening this pull request! 🎉 Please check out our contributing guidelines. If you need help or want to chat with us, join us on Discord https://gofiber.io/discord |
WalkthroughThe pull request introduces a new load-shedding middleware for the Fiber web framework in Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
middleware/loadshedding/loadshedding.go (1)
10-12: Refine the comment phrasing and spacing for clarity.The function’s comment lines could be slightly restated and spaced to enhance readability. Also, consider rephrasing “creates a middleware handler enforces” to maintain proper English grammar.
-// New creates a middleware handler enforces a timeout on request processing to manage server load. -// If a request exceeds the specified timeout, a custom load-shedding handler is executed. +// New creates a middleware handler that enforces a timeout on request processing to help manage server load. +// If a request exceeds the specified timeout, a custom load-shedding handler is invoked.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
middleware/loadshedding/loadshedding.go(1 hunks)middleware/loadshedding/loadshedding_test.go(1 hunks)
🔇 Additional comments (9)
middleware/loadshedding/loadshedding.go (5)
13-17: Check the return value flow for excluded requests.
The exclusion check is handled correctly, ensuring that excluded routes bypass load shedding. Just confirm all necessary route patterns are accounted for in your exclude function.
19-21: Appropriate use of context for the timeout.
Creating a context with the specified timeout is done well here. This approach cleanly provides a timeout boundary for processing the request.
23-25: Double-check concurrency safety for the channel usage.
The buffered channel with capacity 1 is a good approach to avoid potential goroutine leaks. Also, ensure that no other paths close or reuse this channel.
26-29: Potential concurrency concern with fiber.Ctx usage.
Fiber’s Ctx implementation is generally not concurrency-safe. Calling c.Next() in a separate goroutine might pose a risk if the fiber.Ctx gets accessed by multiple goroutines simultaneously. Although this pattern often works in practice, be sure to confirm with Fiber documentation or tests that no concurrency safety issues arise.
31-38: Clear and effective use of select for timeout vs. request completion.
The logic to invoke loadSheddingHandler on timeout vs. returning the normal handler response is correct and straightforward.
middleware/loadshedding/loadshedding_test.go (4)
13-21: Adequate clarity in helper handlers implementation.
The division into discreet handlers (success, timeout, load-shedding) enhances readability. The 2-second sleep for timeout is ingenious for simulating a long-running request.
31-50: Comprehensive test for excluded routes.
Verifies that the middleware works as intended when routes are excluded. This is an essential scenario to ensure safe bypass logic.
51-73: Excellent timeout test approach.
The test methodically confirms that a request exceeding the 1-second cap triggers load-shedding. This thorough check ensures the middleware’s main functionality.
75-92: Valid test for successful requests within the timeout.
This covers the happy path, ensuring the request completes successfully within the allotted time. Including this case rounds out the coverage.
|
@ErfanMomeniii You should be able to replicate the CI using the provided Makefile. Run this: |
|
I believe that bug 1 is the cause of the tests failing. We should also add the improvement 1 from the below review. This review was generated using OpenAI Overall ImpressionThe middleware is straightforward and mostly does what it promises:
Its behavior is reasonably clear, and the accompanying tests show various scenarios (excluded routes, timeouts, and successful requests). However, there are a few issues and potential improvements worth pointing out. Bugs / Potential Issues
Potential Improvements
Missing Features / Wishlist
|
|
Hi @gaby, it should be fixed now. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
middleware/loadshedding/loadshedding.go (2)
10-11: Clarify the documentation header.The comment does not explicitly mention that the timeout is enforced on a per-request basis. Consider clarifying that this middleware applies a deadline to each incoming request.
-// New creates a middleware handler enforces a timeout on request processing to manage server load. -// If a request exceeds the specified timeout, a custom load-shedding handler is executed. +// New creates a middleware handler that enforces a per-request timeout to manage server load. +// If a request exceeds the specified timeout, the custom load-shedding handler is executed.
26-36: Finalize or remove thedonechannel logic.Because
c.Next()blocks until completion, theerr := c.Next()line has already captured any error before the goroutine sends it intodone. Consequently, the channel-based approach offers little concurrency benefit. If you wish to enforce the timeout dynamically, consider running the entire operation in a separate goroutine and listening on the channel in real time. Otherwise, remove the channel to simplify.middleware/loadshedding/loadshedding_test.go (3)
1-2: Use a consistent naming scheme for the test package.Currently, the package is named
loadshedding_test. If your convention is to place middleware tests in the same package, rename the package toloadshedding. Otherwise, ensure the_testsuffix is applied consistently across your test packages for clarity.
23-25: Extend the load-shedding handler for partial data.The
Service Overloadedmessage is concise and predictable. For advanced scenarios (e.g., streaming or partial request completions), consider including more diagnostic information, such as expected wait times or retry intervals, to help consumers handle overload situations.
51-77: Double-check concurrency test coverage.The
Test_LoadSheddingTimeoutensures that a 2-second simulated delay triggers a 503 with a 1-second middleware timeout. However, it relies on Fiber’s internal mechanism for context cancellation. Confirm that all critical concurrency edge cases (e.g., partial content, streaming) are covered. If additional concurrency scenarios exist, create separate tests to avoid conflating multiple concerns in a single test.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
middleware/loadshedding/loadshedding.go(1 hunks)middleware/loadshedding/loadshedding_test.go(1 hunks)
🧰 Additional context used
🪛 golangci-lint (1.62.2)
middleware/loadshedding/loadshedding_test.go
18-18: func timeoutHandler is unused
(unused)
🔇 Additional comments (5)
middleware/loadshedding/loadshedding.go (3)
14-17: Check the exclusion function contract.
When exclude is set, the middleware bypasses timeout enforcement without returning any annotation or log for the excluded path. If debugging load shedding is needed, consider adding optional logging or instrumentation to confirm that certain routes are excluded as intended.
37-45: Note potential partial response issues.
If data has already been written to the response before the context expires, clients might receive partial results without a clear error or an HTTP status code update. Ensure that the load-shedding handler or subsequent logic can convey a proper 503 or alternative response if partial data streaming is possible.
19-25:
Revisit context usage for concurrency and cancellation.
c.Next() is called synchronously, which means the middleware will not reach the select statement until after the handler completes (successfully or otherwise). Ensure that downstream handlers actually respect the context’s cancellation, or this approach will not explicitly interrupt a long-running or blocking operation. Fiber might handle context internally, but confirm that partial responses or streaming are properly aborted when the context is canceled.
Suggestion:
- Spawn
c.Next()in a separate goroutine, and use aselectto cancel if the context times out before completion. - Confirm that your handlers check
c.Context().Err()or similar to exit early.
middleware/loadshedding/loadshedding_test.go (2)
18-21: Confirm necessity of timeoutHandler.
Static analysis warns that timeoutHandler may be unused. Verify whether it is still required, or remove it to keep the code tidy.
🧰 Tools
🪛 golangci-lint (1.62.2)
18-18: func timeoutHandler is unused
(unused)
79-96: Consider adding negative scenarios.
While Test_LoadSheddingSuccessfulRequest verifies normal operation within the timeout, consider including scenarios for boundary conditions—e.g., requests that finish exactly at the timeout boundary or near it—to ensure robust behavior in borderline cases.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3264 +/- ##
==========================================
+ Coverage 84.23% 84.31% +0.08%
==========================================
Files 116 117 +1
Lines 11519 11546 +27
==========================================
+ Hits 9703 9735 +32
+ Misses 1391 1387 -4
+ Partials 425 424 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
Hi again @gaby , I’ve resolved the lint CI issue as well, so I believe everything is ready for merging now. |
|
@ErfanMomeniii Code and Tests look fine to me. Three things missing:
|
Description
This pull request introduces a new Load-Shedding Middleware. This middleware enforces a configurable timeout on request processing to effectively manage server load. If a request exceeds the specified timeout, a custom load-shedding handler is invoked to gracefully handle the overloaded state.
Purpose of the Change:
Changes introduced
This pull request adds the following features and updates:
Load-Shedding Middleware (
loadshedding.New):Unit Tests:
Test_LoadSheddingExcluded: Verifies that excluded routes bypass the middleware.Test_LoadSheddingTimeout: Tests behavior when requests exceed the timeout.Test_LoadSheddingSuccessfulRequest: Confirms successful processing of requests completed within the timeout.Documentation Update: Detailed usage examples for the middleware are added.
Benchmarks: Basic performance benchmarks have been conducted to ensure minimal overhead.
Changelog/What's New: This middleware addition will be documented in the changelog for the next release.
API Longevity: Designed with consistency in mind to align with Fiber's existing middleware and ensure backward compatibility.
Type of change
Checklist
/docs/directory for Fiber's documentation.Commit formatting
Commits follow the recommended format with appropriate emojis for better identification: