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

Fix issue on status code when writer flush #58

Merged
merged 1 commit into from
Nov 8, 2017

Conversation

juliens
Copy link
Contributor

@juliens juliens commented Nov 8, 2017

In the stdlib, when we call Flush on a response writer, if the status code header was never wrote, it write a 200 status code

As the WriteHeader is buffered in GzipWriter, if we call Flush in the handler or in a previous middleware, the status code 200 is send even if we have call WriteHeader before flushing.

Copy link
Contributor

@jprobinson jprobinson left a comment

Choose a reason for hiding this comment

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

😳 thanks!

@jprobinson jprobinson merged commit 0f67f3f into nytimes:master Nov 8, 2017
tmthrgd added a commit to tmthrgd/gziphandler that referenced this pull request Nov 9, 2017
This test case is currently failing but will be fixed in the
following commit. The fix will not follow nytimes#58.
tmthrgd added a commit to tmthrgd/gziphandler that referenced this pull request Nov 9, 2017
This fixes nytimes#58 and the failing test case from
23770e4. This prevents the underlying Flusher from writing the
wrong status code or writing headers before Content-Encoding has
been set.
@tmthrgd
Copy link
Contributor

tmthrgd commented Nov 9, 2017

Correct me if I'm wrong but this actually highlights another bug that is left unfixed. If Flush is called before startGzip is called (because the body hasn't reached the minSize, i.e. Write hasn't been called), then the Content-Encoding header will fail to be set once the body is written (see tmthrgd/gziphandler@574da8f).

I think a far better, and more logical, fix is making Flush a no-op until startGzip has been called (see tmthrgd/gziphandler@cb0f3d9). Such a fix should also be simpler and reduce the size of the per-request GzipResponseWriter struct.

I'm able to confirm that this repo fails the expanded test linked above.

@jprobinson
Copy link
Contributor

@tmthrgd nice catch! mind making a PR with your changes?

@juliens
Copy link
Contributor Author

juliens commented Nov 9, 2017

I don't really agree.

If you intentionly Flush this is because you really want to flush, and you don't want your content to be buffered by a middleware.

If you Flush before having reached the minSize , your response will not be compressed and I think it's normal

@tmthrgd
Copy link
Contributor

tmthrgd commented Nov 9, 2017

@jprobinson I'll take a look tomorrow (it's 3am here, whoops 💤).

@juliens The current behaviour causes gzip compressed data to be written without the Content-Encoding header set. It doesn't skip buffering in any way. There is an argument to be had about what the correct semantics of Flush should be – I'd argue it's more of a hint – but as it stands, this is broken.

@juliens @jprobinson I'll prepare a pull-request to fix the current behaviour (which I think will need to revert 0f67f3f), and if there is a desire to change the semantics of Flush regarding buffering that can be dealt with afterwards.

tmthrgd added a commit to tmthrgd/gziphandler that referenced this pull request Nov 10, 2017
This fixes nytimes#58 and prevents the underlying Flusher
from writing the wrong status code or writing
headers before Content-Encoding has been set.

This is cb0f3d94c6 with the test
case taken from 574da8f22d.
tmthrgd added a commit to tmthrgd/gziphandler that referenced this pull request Nov 10, 2017
This fixes nytimes#58 and prevents the underlying Flusher
from writing the wrong status code or writing
headers before Content-Encoding has been set.

This is cb0f3d94c6 with the test
case taken from 574da8f22d.
tmthrgd added a commit to tmthrgd/gziphandler that referenced this pull request Nov 10, 2017
This fixes nytimes#58 and prevents the underlying Flusher
from writing the wrong status code or writing
headers before Content-Encoding has been set.

This is cb0f3d94c6 with the test
case taken from 574da8f22d.
jprobinson pushed a commit that referenced this pull request Nov 20, 2017
This fixes #58 and prevents the underlying Flusher
from writing the wrong status code or writing
headers before Content-Encoding has been set.

This is tmthrgd/gziphandler@cb0f3d94c6 with the test
case taken from tmthrgd/gziphandler@574da8f22d.
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