go-chi
[compress] Create compress writer only in case the content is compressible (fixes potential memory leak)
Description
Recently, there was an attempt to fix the potential memory leak in the compress middleware. For details, check the following PR: https://github.com/go-chi/chi/pull/919
Although the fix worked for that particular case, it introduced a bug (https://github.com/go-chi/chi/issues/923) that caused the unnecessary write of the gzip header even in case the content is not compressible.
After discussing this with @VojtechVitek it was decided to try to create a compressed writer only when the content is compressible.
This PR also adds more test cases to test the raw output in the case of no compression to make sure we are not changing the output.
Related
- https://github.com/go-chi/chi/issues/923
- https://github.com/go-chi/chi/pull/919
Tested this PR with both gzip and brotli compression and everything looks 👌
cc: @VojtechVitek
@VojtechVitek Anything I can do to move this forward? I would love to see an end to this issue, let me know if the PR needs more updates 🙏
Hey, I'll get back to this once I have some free time. I'm swamped with product work now :)
@VojtechVitek The PR has been open for a long time already. Is there anything that can be done to merge it? I understand this part is sensitive. Please, let me know if there are any additional tests need to be performed to make sure it is safe to merge it 🙏
@VojtechVitek The PR has been rebased on the fresh source branch. Please, check it out when you have time 🙏
@VojtechVitek Regular bump here 😅 Please, let me know if there is anything I can do to move thisPR forward 🙏
@VojtechVitek Regular follow-up on this PR 😅 Still interested in merging this one to the main branch in case the team sees the value. Please, let me know if there is anything I can do to push this forward 🙏
Hello, I'm sorry. I'm too busy with other work right now -- but eventually I'll get back to this PR.
Since this feature caused regressions last time we merged it, I'm very cautious. I feel like it will require proper review & significant time for testing.
Note for anyone reading this: Feel free to sponsor the project author if you want to bump up the priority 👍
@VojtechVitek
Since this feature caused regressions last time we merged it, I'm very cautious.
Fair enough. I am extremely interested in making this change as robust as possible. We have been running this change in production for around a year already and haven't noticed any issues.
Would you need any sort of additional tests, like benchmarks, etc? Or maybe you would need to review the Go profile reports. Let me know if there is something else I can do to make this change set ready to be merged.
I feel like it will require proper review & significant time for testing.
What does a "proper review" look like? Should we request a review from folks familiar with the compress middleware? I was thinking of tagging a few folks who recently contributed to the middleware. Please let me know if that sounds good.
For posterity, I posted this PR in the Gophers #go-chi Slack channel to attract more reviewers/sponsors.