search

nytimes / gziphandler

Go middleware to gzip HTTP responses
https://godoc.org/github.com/NYTimes/gziphandler
Apache License 2.0
868 stars 129 forks source link

Fix issue on status code when writer flush #58

Closed juliens closed 6 years ago

juliens commented 6 years ago

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.

tmthrgd commented 6 years ago

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@574da8f22dc227d501a8b569a6e24ae76cba7ac6).

I think a far better, and more logical, fix is making Flush a no-op until startGzip has been called (see tmthrgd/gziphandler@cb0f3d94c6f51f5bd574431a4eaf2f8b9f6bbf0d). 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 commented 6 years ago

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

juliens commented 6 years ago

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 commented 6 years ago

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