fix: do not panic on client disconnect #650
Conversation
WalkthroughThe pull request introduces enhancements to the error handling logic in the Changes
Assessment against linked issues
Possibly related PRs
Poem
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
🧹 Outside diff range and nitpick comments (2)
huma.go (1)
515-521: LGTM! Consider adding a comment about status code 499.The implementation correctly handles client disconnections by checking for context cancellation and setting an appropriate status code. This change effectively prevents panics when clients disconnect during response writing.
Consider adding a comment explaining that status code 499 is a non-standard code (originated from nginx) used to indicate that the client closed the connection before the server could send the response:
if errors.Is(ctx.Context().Err(), context.Canceled) { + // Status code 499 is a non-standard code (originated from nginx) that indicates + // the client closed the connection before the server could send the response ctx.SetStatus(499) return nil }huma_test.go (1)
2124-2126: Consider using errors.New for better performanceSince the error message doesn't require any formatting, using
errors.Newwould be more efficient thanfmt.Errorf.func (br *BrokenWriter) Write(p []byte) (n int, err error) { - return 0, fmt.Errorf("failed writing") + return 0, errors.New("failed writing") }🧰 Tools
🪛 GitHub Check: Build & Test (1.23)
[failure] 2125-2125:
fmt.Errorf can be replaced with errors.New (perfsprint)🪛 golangci-lint
2125-2125: fmt.Errorf can be replaced with errors.New
(perfsprint)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
huma.go(1 hunks)huma_test.go(1 hunks)
🧰 Additional context used
🪛 GitHub Check: Build & Test (1.23)
huma_test.go
[failure] 2125-2125:
fmt.Errorf can be replaced with errors.New (perfsprint)
🪛 golangci-lint
huma_test.go
2125-2125: fmt.Errorf can be replaced with errors.New
(perfsprint)
🔇 Additional comments (2)
huma_test.go (2)
2120-2123: LGTM!
The BrokenWriter struct is well-designed for testing purposes, properly embedding http.ResponseWriter.
2128-2151: LGTM! Well-structured test case
The test effectively simulates a client disconnection scenario by:
- Using context cancellation to simulate the disconnect
- Using BrokenWriter to simulate write failures
- Verifying that the handler gracefully handles these failures without panicking
The test is well-documented and serves as a good regression test for the fix.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #650 +/- ##
==========================================
+ Coverage 92.97% 93.04% +0.06%
==========================================
Files 22 22
Lines 4895 4901 +6
==========================================
+ Hits 4551 4560 +9
+ Misses 300 298 -2
+ Partials 44 43 -1 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
huma_test.go (1)
2128-2151: Consider adding more assertions for comprehensive testing.The test effectively simulates client disconnection and write failures. However, it could be enhanced with additional assertions:
- Verify that the response status code is set to 499 (client closed request)
- Add assertions to verify the error handling behavior
Consider adding these assertions:
assert.NotPanics(t, func() { api.Adapter().ServeHTTP(resp, req) }) +assert.Equal(t, 499, recorder.Code, "Expected status code 499 for client closed request") +assert.Empty(t, recorder.Body.String(), "Expected empty response body")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
huma_test.go(1 hunks)
🔇 Additional comments (1)
huma_test.go (1)
2120-2127: LGTM! Clean implementation of the error simulation.
The BrokenWriter struct and its implementation are well-designed for simulating write failures in tests. The code is clean, focused, and follows Go conventions.
This fix detects when a client has disconnected via a canceled context and prevents throwing and error/panic in that case.
Fixes #632.
Summary by CodeRabbit
New Features
Tests