🐛 bug: close BodyStream in adaptor FiberHandler streaming path#4267
Conversation
|
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughThis PR adds resource cleanup for HTTP response body streaming in the fasthttp adaptor middleware. A deferred call now closes the response body stream after request handling completes, preventing stream leaks. Two new tests verify closure in both successful send and write-failure paths. ChangesStream Body Closure
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.1)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" 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. Comment |
There was a problem hiding this comment.
Pull request overview
This PR fixes a resource leak in the middleware/adaptor net/http adapter by ensuring that fasthttp response body streams are always closed when the adapter takes the streaming copy path, preventing leaked stream resources on EOF or downstream write failures.
Changes:
- Add a deferred
fctx.Response.CloseBodyStream()in theFiberHandlerstreaming branch so the underlying body stream is released on all exit paths. - Add regression tests to assert streamed
io.ReadCloserinstances are closed both on normal completion and when the downstreamResponseWriterreturns a write error.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| middleware/adaptor/adaptor.go | Adds a deferred body-stream close in the streaming response forwarding path. |
| middleware/adaptor/adaptor_test.go | Adds regression tests to verify body-stream closure on success and on write failure. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4267 +/- ##
==========================================
+ Coverage 91.18% 91.27% +0.08%
==========================================
Files 129 130 +1
Lines 12757 12756 -1
==========================================
+ Hits 11633 11643 +10
+ Misses 709 701 -8
+ Partials 415 412 -3
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 ensures that the response body stream is closed in the adaptor middleware by adding a deferred call to CloseBodyStream. It also includes new unit tests to verify that the stream is correctly closed during both successful operations and write errors. I have no feedback to provide.
Motivation
fasthttpBodyStream()data to anet/httpResponseWriterbut did not close the underlying fasthttp body stream, which can leak resources and enable DoS via repeated disconnects.Description
fctx.Response.CloseBodyStream()is always called when the streaming branch is taken by adding a deferred close inmiddleware/adaptor/adaptor.goso streams are released on EOF, read errors, or downstream write errors.middleware/adaptor/adaptor_test.gothat assert streamedio.ReadCloserinstances are closed after normal completion and after a simulated downstream write error using a close-tracking reader and a failingResponseWriter.middleware/adaptor/adaptor.goandmiddleware/adaptor/adaptor_test.go.