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

encode: ignore flushing until after first write #4318

Merged
merged 2 commits into from
Aug 31, 2021

Conversation

ueffel
Copy link
Contributor

@ueffel ueffel commented Aug 31, 2021

The first write will determine if encoding has to be done and will add an Content-Encoding. Until then Flushing has to be delayed so the Content-Encoding header can be added before headers and status code is written. (A passthrough flush would write header and status code)

Fixes #4314

As of now, no integration test included

@francislavoie francislavoie added the bug 🐞 Something isn't working label Aug 31, 2021
@francislavoie francislavoie added this to the v2.4.5 milestone Aug 31, 2021
The first write will determine if encoding has to be done and will add an Content-Encoding. Until then Flushing has to be delayed so the Content-Encoding header can be added before headers and status code is written. (A passthrough flush would write header and status code)
francislavoie
francislavoie previously approved these changes Aug 31, 2021
Copy link
Member

@francislavoie francislavoie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've tested this fix the same way that I tried it last night according to your report #4314 (comment), and I can confirm it fixed that case.

LGTM!

If you find the time later to add an integration test, that would be great, but I don't think we need to wait on merging this for the tests.

Copy link
Member

@mholt mholt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This LGTM, thanks so much for identifying the problem and implementing a solid fix.

I just request that we link to the bug in the comment since it may not be obvious why this could be a problem. 👍

modules/caddyhttp/encode/encode.go Outdated Show resolved Hide resolved
@mholt mholt merged commit 4ebf100 into caddyserver:master Aug 31, 2021
@ueffel ueffel deleted the issue-4314 branch March 12, 2022 21:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐞 Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Content-Encoding header missing with reverse-proxied chunked response
3 participants