Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[compress] Create compress writer only in case the content is compressible #928

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

Neurostep
Copy link
Contributor

Description

Recently, there was an attempt to fix the potential memory leak in the compress middleware. For details, check the following PR: #919

Although the fix worked for that particular case, it introduced a bug (#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

@Neurostep Neurostep force-pushed the Neurostep/compression-middleware-refactor-writer-wrapper branch from 625b26f to e57b2d6 Compare July 2, 2024 13:48
@Neurostep Neurostep marked this pull request as ready for review July 3, 2024 11:10
@Neurostep
Copy link
Contributor Author

Tested this PR with both gzip and brotli compression and everything looks 👌
cc: @VojtechVitek

@Neurostep
Copy link
Contributor Author

@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 🙏

@VojtechVitek
Copy link
Contributor

Hey, I'll get back to this once I have some free time. I'm swamped with product work now :)

@tahirmurata
Copy link

@VojtechVitek Any updates?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants