🐛 bug: fix compress middleware's shouldSkip method to avoid memory growth#4284
Conversation
…esponses The shouldSkip method in the compress middleware was calling c.Response().Body() which materializes any active bodyStream into memory. This defeats the purpose of streaming and causes unbounded memory growth for large streamed responses. The fix checks if the response is using a body stream via IsBodyStream() before calling Body() to check if the body is empty. This allows streaming responses set via c.SendStream/Response.SetBodyStream to pass through without being materialized into memory. Added tests to verify: - Streaming responses without compression work correctly - Streaming responses with compression are properly compressed by fasthttp Fixes #4294 Agent-Logs-Url: https://github.com/gofiber/fiber/sessions/47dff18e-6997-459d-a63d-4d83aca1b48d Co-authored-by: gaby <835733+gaby@users.noreply.github.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4284 +/- ##
==========================================
- Coverage 91.35% 91.33% -0.02%
==========================================
Files 132 132
Lines 12977 12978 +1
==========================================
- Hits 11855 11854 -1
- Misses 708 709 +1
- Partials 414 415 +1
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.
Pull request overview
This PR addresses a streaming-related memory growth bug in Fiber’s compress middleware by avoiding response body materialization during the shouldSkip decision logic when the response is a stream.
Changes:
- Update
shouldSkipto checkResponse.IsBodyStream()before callingResponse.Body()to prevent buffering streamed responses in memory. - Add streaming-focused middleware tests for requests with and without
Accept-Encoding.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| middleware/compress/compress.go | Avoids calling Response.Body() for streamed responses during skip checks, preventing stream materialization. |
| middleware/compress/compress_test.go | Adds tests covering streaming responses with/without Accept-Encoding (though current assertions don’t directly detect the regression). |
|
@copilot apply changes based on the comments in this thread |
Agent-Logs-Url: https://github.com/gofiber/fiber/sessions/0c53e070-8a1e-492d-8242-bc29f34fb492 Co-authored-by: gaby <835733+gaby@users.noreply.github.com>
Applied the review feedback in 29ebe66. The streaming regression coverage is now a unit-level |
|
Congrats on merging your first pull request! 🎉 We here at Fiber are proud of you! If you need help or want to chat with us, join us on Discord https://gofiber.io/discord |
Thanks for asking me to work on this. I will get started on it and keep this PR's description up to date as I form a plan and make progress.