feat: Add Drop method to DefaultCtx for silent connection termination #3257
Conversation
Introduced a Drop method in DefaultCtx for closing connections, enabling easier resource management. Removed unnecessary nil-checks for headers in manager_msgp to simplify code logic. Added a unit test to ensure the new Drop method behaves as expected.
The `Drop` method allows silently terminating client connections without sending HTTP headers or a response body. This is useful for scenarios like mitigating DDoS attacks or blocking unauthorized access to sensitive endpoints. Example usage and function signature are included in the updated documentation.
ryanbekhen
requested review from
gaby,
sixcolors,
ReneWerner87 and
efectn
and removed request for
a team
WalkthroughThis pull request introduces a new Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 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)
ctx_test.go (1)
5850-5863: LGTM! The test case verifies the basic Drop() functionality.The test properly verifies that Drop() terminates the connection without sending a response. The test is well structured with parallel execution enabled.
Consider adding additional test cases to verify:
- Calling Drop() after writing some response data
- Calling Drop() from different middleware positions
- Verifying Drop() behavior with different HTTP methods
- Testing Drop() with concurrent requests
Here's a suggested test case structure:
func Test_Ctx_Drop_AfterWrite(t *testing.T) { t.Parallel() app := New() app.Get("/drop-after-write", func(c Ctx) error { c.Write([]byte("partial response")) return c.Drop() }) // Test and verify behavior }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
ctx.go(1 hunks)ctx_interface_gen.go(1 hunks)ctx_test.go(1 hunks)docs/api/ctx.md(1 hunks)middleware/cache/manager_msgp.go(0 hunks)
💤 Files with no reviewable changes (1)
- middleware/cache/manager_msgp.go
🧰 Additional context used
🪛 Markdownlint (0.37.0)
docs/api/ctx.md
487-487: Expected: 1; Actual: 2
Multiple consecutive blank lines
(MD012, no-multiple-blanks)
🪛 GitHub Check: markdownlint
docs/api/ctx.md
[failure] 487-487: Multiple consecutive blank lines
docs/api/ctx.md:487 MD012/no-multiple-blanks Multiple consecutive blank lines [Expected: 1; Actual: 2] https://github.com/DavidAnson/markdownlint/blob/v0.36.1/doc/md012.md
🔇 Additional comments (4)
ctx_interface_gen.go (1)
353-353: LGTM! Interface addition is clean and well-placed.
The new Drop() method follows the existing interface patterns with appropriate error handling.
docs/api/ctx.md (1)
466-486: Documentation is comprehensive and well-structured.
The documentation clearly explains the purpose, provides a practical example, and follows the established documentation style.
ctx.go (2)
1982-1986: Implementation is efficient and correct.
The method efficiently terminates the connection by directly accessing the underlying connection. The nolint directive is appropriately used as error wrapping would not add value in this case.
1982-1986: Consider logging dropped connections in production.
While the implementation is correct, it's recommended to add logging when connections are dropped to help monitor potential abuse or issues in production.
Consider implementing rate limiting or connection tracking to prevent abuse of this feature.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3257 +/- ##
==========================================
+ Coverage 82.72% 82.77% +0.04%
==========================================
Files 115 115
Lines 11365 11362 -3
==========================================
+ Hits 9402 9405 +3
+ Misses 1557 1551 -6
Partials 406 406
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Eliminated an unnecessary blank line in the API context documentation for improved readability and formatting consistency. No functional changes were made to the content.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
docs/api/ctx.md (1)
466-472: Enhance the method description with more details.The description could be more explicit about the connection closure mechanism and its implications.
Consider adding:
- Mention that it closes the underlying TCP connection
- Note about potential client behavior (connection reset)
- Warning about proper error handling in client code
Revised the example code in the API documentation to return a generic "Hello World!" string instead of a dynamic response. This improves consistency and simplifies the example for easier understanding.
Simplified the Drop method by inlining the connection close call. Added new test cases to ensure proper handling of no-response scenarios and improved overall test coverage.
|
@gaby I have pushed the changes. please check. |
Fix the golangci-lint issue, and it should be good. |
|
@grivera64 Added you to reviewers since you did the initial research on this. |
|
@gaby done |
|
@coderabbitai what is the name of the method in expressjs which is the same as this feature |
|
Tip For best results, initiate chat on the files or code changes. @ReneWerner87 In Express.js, there isn’t a direct equivalent named method for silently dropping a connection like Fiber’s “Drop()”. The closest approach is to forcibly close the underlying socket by calling something like “req.socket.destroy()” (or “res.destroy()” in newer Node versions). This abruptly ends the connection without sending a response body or headers. |
|
@coderabbitai suggest good method names for this feature |
|
Tip For best results, initiate chat on the files or code changes. @ReneWerner87 Here are a few method name suggestions that clearly convey the intent of silently terminating the connection:
|
|
should we use a different name? what do you say @gofiber/maintainers ? |
There was a problem hiding this comment.
Thank you for the contribution! The Drop method is a valuable addition to the Ctx interface, and the accompanying documentation and tests are solid. I have a couple of minor suggestions for further improvements:
-
Error Handling:
- The
Dropmethod includes anolint:wrapcheckdirective. Adding a comment to explain why error wrapping is intentionally avoided could be helpful for maintainers and future contributors.
- The
-
Test Coverage:
- While the test for
Dropis thorough, edge cases like attempting to Drop an already-closed connection could be tested to ensure robust behavior. Also, While I verified that middleware works as expected withDrop, it might be beneficial to include explicit test cases verifying middleware behavior whenDropis called. This ensures future changes do not inadvertently break compatibility.
For example:
// Middleware that sets a header after the handler is called app.Use(func(c Ctx) error { err := c.Next() c.Response().Header.Set("X-Test", "test") return err })
- While the test for
The name If we want alternatives for clarity or consistency with other frameworks, we could consider:
Appending "Connection" isn't necessary since this method is part of the connection context. That said, |
|
Ok then let's stick to drop |
|
@sixcolors ok, I'll make the changes later |
This test ensures the correct behavior of the Ctx.Drop method when used with middleware, including response handling and error scenarios. It verifies that the middleware and handler properly handle the Drop call and its resulting effects.
The Drop method allows closing connections without sending a response, improving control over connection handling. Also updated a test assertion to use StatusOK for improved readability and consistency.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
ctx.go (1)
1982-1986: Add documentation comment for Drop method.The implementation looks good, but please add a documentation comment explaining the method's purpose and behavior.
+// Drop closes the underlying connection without sending any response headers or body. +// This can be useful for silently terminating client connections, such as in DDoS mitigation +// or when blocking access to sensitive endpoints. func (c *DefaultCtx) Drop() error {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
ctx.go(1 hunks)ctx_test.go(1 hunks)
🔇 Additional comments (1)
ctx_test.go (1)
5850-5905: LGTM! Comprehensive test coverage.
The test cases thoroughly verify the Drop method's functionality, including:
- Basic Drop behavior
- Middleware interaction
- Error handling
- Response validation
Explain the rationale for not wrapping errors in the Drop method. Emphasize that the returned error is solely for logging and not for further propagation or processing.
Clarified the `Drop` method's behavior, specifying that it closes the connection without sending headers or a body. Added examples of use cases, such as DDoS mitigation and blocking sensitive endpoints.
done, please check again. |
Replaced the direct header setting with the `Set` method for consistency and improved clarity. Removed a test case checking for a panic on closed response body as it is no longer applicable.
|
done @grivera64 |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
ctx_test.go (1)
5850-5877: LGTM! Good test coverage.The test effectively verifies the Drop functionality by checking both error return and nil response. It also includes a good comparison with normal handler behavior.
Consider adding a test case to verify that no response headers are sent when Drop is called, as this is a key feature of silent termination.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
ctx.go(1 hunks)ctx_test.go(1 hunks)
🔇 Additional comments (2)
ctx.go (1)
1981-1988: LGTM! Clean and efficient implementation.
The Drop method provides a simple and effective way to silently terminate client connections. The error handling is appropriately lightweight, and the code is well-documented.
ctx_test.go (1)
5879-5901: LGTM! Good middleware interaction test.
The test effectively verifies that Drop behavior is maintained even when used with middleware, ensuring that middleware operations don't interfere with the connection termination.
Description
This pull request introduces the Drop method to the DefaultCtx structure. The Drop method allows developers to silently terminate client connections without sending any HTTP headers or response body. This functionality is particularly useful for scenarios such as:
This addition aligns with Fiber’s goal of providing high-performance web server functionalities with fine-grained control.
Fixes #3252
Changes introduced
Type of change
Checklist
Before you submit your pull request, please make sure you meet these requirements:
/docs/directory for Fiber's documentation.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