🔥 feat: Add End() method to Ctx #3280
Conversation
grivera64
requested review from
gaby,
sixcolors,
ReneWerner87 and
efectn
and removed request for
a team
WalkthroughThe pull request introduces a new Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Poem
Finishing Touches
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 (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3280 +/- ##
==========================================
+ Coverage 84.08% 84.14% +0.05%
==========================================
Files 116 116
Lines 11541 11553 +12
==========================================
+ Hits 9704 9721 +17
+ Misses 1406 1402 -4
+ Partials 431 430 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
![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 (6)
ctx.go (1)
1989-2005: LGTM! Clean implementation with proper buffering.The implementation correctly uses buffered writing and handles the response flushing sequence.
Consider wrapping errors with more context for better debugging:
func (c *DefaultCtx) End() error { ctx := c.RequestCtx() conn := ctx.Conn() bw := bufio.NewWriter(conn) if err := ctx.Response.Write(bw); err != nil { - return err + return fmt.Errorf("failed to write response: %w", err) } if err := bw.Flush(); err != nil { - return err //nolint:wrapcheck // unnecessary to wrap it + return fmt.Errorf("failed to flush response: %w", err) } - return conn.Close() //nolint:wrapcheck // unnecessary to wrap it + if err := conn.Close(); err != nil { + return fmt.Errorf("failed to close connection: %w", err) + } + return nil }🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 1997-1998: ctx.go#L1997-L1998
Added lines #L1997 - L1998 were not covered by testsctx_test.go (1)
5934-5994: Add test coverage for additional scenarios.The current tests cover basic functionality, but consider adding tests for:
- Error cases (e.g., when Write/Flush/Close fails)
- Concurrent End() calls
- End() with different response types (JSON, streaming, etc.)
- End() with various middleware combinations
Would you like me to help generate these additional test cases?
🧰 Tools
🪛 golangci-lint (1.62.2)
5939-5939: Error return value of
c.SendStringis not checked(errcheck)
5946-5946: expected-actual: need to reverse actual and expected values
(testifylint)
5949-5949: expected-actual: need to reverse actual and expected values
(testifylint)
5959-5959: Error return value of
c.Nextis not checked(errcheck)
5965-5965: Error return value of
c.SendStatusis not checked(errcheck)
5972-5972: expected-actual: need to reverse actual and expected values
(testifylint)
🪛 GitHub Check: lint
[failure] 5939-5939:
Error return value ofc.SendStringis not checked (errcheck)
[failure] 5946-5946:
expected-actual: need to reverse actual and expected values (testifylint)
[failure] 5949-5949:
expected-actual: need to reverse actual and expected values (testifylint)
[failure] 5959-5959:
Error return value ofc.Nextis not checked (errcheck)
[failure] 5965-5965:
Error return value ofc.SendStatusis not checked (errcheck)
[failure] 5972-5972:
expected-actual: need to reverse actual and expected values (testifylint)🪛 GitHub Actions: golangci-lint
[error] 5939-5939: Error return value of
c.SendStringis not checked (errcheck)docs/whats_new.md (2)
407-420: Enhance the documentation with additional details.While the documentation provides a good overview, consider adding:
- A note about error handling when the connection is already closed
- Clarification that
End()returns an error- Mention that this is particularly useful for scenarios involving streaming responses
### End In v3, we introduced a new method to match the Express.js API's `res.end()` method. ```go -func (c Ctx) End() +func (c Ctx) End() errorWith this method, you can:
- Stop middleware from controlling the connection after a handler further up the method chain
by immediately flushing the current response and closing the connection.- Use
return c.End()as an alternative toreturn nil
+- Handle streaming responses by explicitly closing the connection
+Note: If the connection is already closed, End() will return an error.
--- `421-440`: **Improve the example to demonstrate error handling.** The example should show proper error handling and a more realistic use case. ```diff app.Use(func (c fiber.Ctx) error { err := c.Next() if err != nil { log.Println("Got error: %v", err) - return c.SendString(err.Error()) // Will be unsuccessful since the response ended below + // The response has already ended, so this error should be logged but not sent + log.Printf("Cannot send error to client: %v", err) + return nil } return nil }) app.Get("/hello", func (c fiber.Ctx) error { query := c.Query("name", "") if query == "" { - c.SendString("You don't have a name?") - c.End() // Closes the underlying connection - return errors.New("No name provided") + if err := c.SendString("You don't have a name?"); err != nil { + return err + } + if err := c.End(); err != nil { + return fmt.Errorf("failed to end connection: %v", err) + } + return errors.New("no name provided") } return c.SendString("Hello, " + query + "!") })docs/api/ctx.md (2)
495-500: Add error handling to the example.The basic example should demonstrate proper error handling.
```go title="Example" app.Get("/", func(c fiber.Ctx) error { - c.SendString("Hello World!") - return c.End() + if err := c.SendString("Hello World!"); err != nil { + return err + } + return c.End() })--- `509-534`: **Enhance the middleware example with better practices.** The example should demonstrate better error handling and logging practices. ```diff ```go title="Example" // Error Logging/Responding middleware app.Use(func(c fiber.Ctx) error { err := c.Next() // Log errors & write the error to the response if err != nil { - log.Printf("Got error in middleware: %v", err) - return c.Writef("(got error %v)", err) + // Log the error with context + log.Printf("Error in %s %s: %v", c.Method(), c.Path(), err) + // The response might have already ended + if writeErr := c.Writef("(got error %v)", err); writeErr != nil { + log.Printf("Failed to write error response: %v", writeErr) + } + return err } // No errors occurred return nil }) // Handler with simulated error app.Get("/", func(c fiber.Ctx) error { // Closes the connection instantly after writing from this handler // and disallow further modification of its response - defer c.End() + defer func() { + if err := c.End(); err != nil { + log.Printf("Failed to end connection: %v", err) + } + }() c.SendString("Hello, ... I forgot what comes next!") return errors.New("some error") })</blockquote></details> </blockquote></details> <details> <summary>📜 Review details</summary> **Configuration used: CodeRabbit UI** **Review profile: CHILL** **Plan: Pro** <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 4e5fea1d7a830a52cbe736d2da88660a2e959549 and a901e8ff19e43243d5188d721dad10dc7024f431. </details> <details> <summary>📒 Files selected for processing (5)</summary> * `ctx.go` (1 hunks) * `ctx_interface_gen.go` (2 hunks) * `ctx_test.go` (1 hunks) * `docs/api/ctx.md` (1 hunks) * `docs/whats_new.md` (2 hunks) </details> <details> <summary>🧰 Additional context used</summary> <details> <summary>🪛 GitHub Check: codecov/patch</summary> <details> <summary>ctx.go</summary> [warning] 1997-1998: ctx.go#L1997-L1998 Added lines #L1997 - L1998 were not covered by tests </details> </details> <details> <summary>🪛 golangci-lint (1.62.2)</summary> <details> <summary>ctx_test.go</summary> 5939-5939: Error return value of `c.SendString` is not checked (errcheck) --- 5946-5946: expected-actual: need to reverse actual and expected values (testifylint) --- 5949-5949: expected-actual: need to reverse actual and expected values (testifylint) --- 5959-5959: Error return value of `c.Next` is not checked (errcheck) --- 5965-5965: Error return value of `c.SendStatus` is not checked (errcheck) --- 5972-5972: expected-actual: need to reverse actual and expected values (testifylint) </details> </details> <details> <summary>🪛 GitHub Check: lint</summary> <details> <summary>ctx_test.go</summary> [failure] 5939-5939: Error return value of `c.SendString` is not checked (errcheck) --- [failure] 5946-5946: expected-actual: need to reverse actual and expected values (testifylint) --- [failure] 5949-5949: expected-actual: need to reverse actual and expected values (testifylint) --- [failure] 5959-5959: Error return value of `c.Next` is not checked (errcheck) --- [failure] 5965-5965: Error return value of `c.SendStatus` is not checked (errcheck) --- [failure] 5972-5972: expected-actual: need to reverse actual and expected values (testifylint) </details> </details> <details> <summary>🪛 GitHub Actions: golangci-lint</summary> <details> <summary>ctx_test.go</summary> [error] 5939-5939: Error return value of `c.SendString` is not checked (errcheck) </details> </details> </details> <details> <summary>⏰ Context from checks skipped due to timeout of 90000ms (2)</summary> * GitHub Check: Compare * GitHub Check: repeated </details> <details> <summary>🔇 Additional comments (4)</summary><blockquote> <details> <summary>ctx_interface_gen.go (1)</summary> `357-358`: **LGTM! Clean interface addition.** The new `End()` method is well documented and follows the existing interface pattern. </details> <details> <summary>docs/whats_new.md (1)</summary> `344-344`: **LGTM! Clear and concise introduction of the End method.** The documentation correctly introduces the `End` method as a new feature in v3, aligning with Express.js API. </details> <details> <summary>docs/api/ctx.md (2)</summary> `487-494`: **LGTM! Clear method signature and description.** The API documentation correctly shows the method signature and provides a clear description of its purpose. --- `502-508`: **LGTM! Important caution about connection closure.** The caution note clearly warns users about the implications of calling `c.End()`. </details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
|
There appears to be a decrease in test coverage due to using fashttp's ctx.Response.Write() method error checking. From the looks of it, this is because there is no checking whether or not Should we consider adding a |
|
@grivera64 I think it's fine as it is. |
For sure, I'll mark the PR as ready to review again. |
gaby
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
ctx_test.go (1)
5952-5965: Make timeout test more deterministic.The test uses a hardcoded sleep duration which could be flaky in CI environments. Consider using a context with timeout or a more deterministic mechanism to test timeout behavior.
app.Get("/", func(c Ctx) error { - time.Sleep(2 * time.Second) + ctx, cancel := context.WithTimeout(context.Background(), 100*time.Millisecond) + defer cancel() + <-ctx.Done() return c.End() })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
ctx_test.go(1 hunks)docs/api/ctx.md(1 hunks)docs/whats_new.md(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/api/ctx.md
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: unit (1.23.x, windows-latest)
- GitHub Check: Compare
- GitHub Check: repeated
🔇 Additional comments (3)
docs/whats_new.md (1)
Line range hint
1-1000: Ensure consistent indentation in code examples.All code examples should use 4 spaces for indentation to maintain consistency throughout the documentation.
ctx_test.go (2)
5967-5988: 🛠️ Refactor suggestionAdd error handling for SendStatus and Next calls.
The test should check errors returned by
c.SendStatus()andc.Next()to ensure proper error propagation.app.Use(func(c Ctx) error { - c.Next() //nolint:errcheck // unnecessary to check error + if err := c.Next(); err != nil { + return err + } return c.Drop() }) app.Get("/", func(c Ctx) error { - c.SendStatus(StatusOK) //nolint:errcheck // unnecessary to check error + if err := c.SendStatus(StatusOK); err != nil { + return err + } return c.End() })Likely invalid or redundant comment.
5934-5950: 🛠️ Refactor suggestionAdd error handling for SendString call.
The test should check the error returned by
c.SendString()to ensure the response was set correctly before callingEnd().app.Get("/", func(c Ctx) error { - c.SendString("Hello, World!") //nolint:errcheck // unnecessary to check error + if err := c.SendString("Hello, World!"); err != nil { + return err + } return c.End() })Likely invalid or redundant comment.
Description
This PR adds the End() method to Ctx in v3 to allow instant flushing and closing of an active connection.
This feature is useful for stopping middleware from controlling the connection after a handler further up the method chain. Some middleware (like fiberprometheus) will work with a response after calling c.Next(). In addition, Fiber's default error handler may also write to the response, while a user may want to handle an error in different ways from within specific handlers and only log detailed errors internally.
It also can be used as an alternative to simply using return nil at the end of a handler, as done in ExpressJS (see below).
Fixes #3276
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.
res.end()functionalityType 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