gzip ignores HTTP error codes

[transkatgirl]

I am using go version go1.8 linux/amd64, and I have been trying to be able to send some data through gzip. The problem is that even if I send an error code using w.WriteHeader, this library will simply ignore it and send HTTP 200 no matter what. Is there any way that you could fix this?

[adammck]

Can you provide an example? I'd very much like to fix this.

[transkatgirl]

Sure thing! I am a bit lazy (and I have no idea what is causing the problem), so I think I will just send you the entire file. Main.go.txt

[transkatgirl]

I'm not very sure if this is an actual bug or me just misusing the library, I am kinda new to Golang still :P Also, the code I sent you is a very early work in progress, and is nowhere near done.

[adammck]

It looks like http.ServeFile sends its own status header, which overwrites yours. Does this work as expected when you remove the gzip middlware? The stdlib response logs an error (http: multiple response.WriteHeader calls) when this is the case, but our implementation silently overwrites the previous status, which is rather unhelpful. Sorry about that. I expect you'll see that error in your log.

[transkatgirl]

Yup, this works as expected when I remove the gzip middleware. When I add gzip back, it goes back to silently removing the 404 status code. I do still see the 404 in my log, of course. But, the browser doesn't see it, and that can be a problem.

[tmthrgd]

This is a bug in gziphandler, although you could almost say this is undefined behaviour of Golang's net/http.

The net/http WriteHeader functions all ignore multiple calls to WriteHeader and only use the status code from the first call. Unfortunately this behaviour is not explicitly documented anywhere.

src/net/http/server.go:

func (w *response) WriteHeader(code int) {
    ...
    if w.wroteHeader {
        w.conn.server.logf("http: multiple response.WriteHeader calls")
        return
    }
    w.wroteHeader = true
    w.status = code
    ...

src/net/http/h2_bundle.go:

func (rws *http2responseWriterState) writeHeader(code int) {
    if !rws.wroteHeader {
        rws.wroteHeader = true
        rws.status = code
        ...

I've fixed this with tmthrgd@c3e45bd8a0785df13a521b09086779133a73f15b in my fork. A similar fix could be introduced into this repository, care has to be taken to ensure that buffering of the response (for min size) doesn't cause future calls to WriteHeader to succeed.

[EmielM]

I think https://github.com/NYTimes/gziphandler/pull/49 has fixed this too. Can you confirm, @kittyhacker101 / @tmthrgd ?

[tmthrgd]

@EmielM I don't know how I missed your comment for all this time, sorry. I don't seem to have gotten a notification for it 😕

I just ran this repo against the test case I introduced in tmthrgd@c3e45bd and it still fails with:

--- FAIL: TestStatusCodes (0.00s)
        gzip_test.go:287: StatusCode should have been 404 but was 500

The failing test code is (snippet):

    handler = GzipHandler(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
        http.NotFound(w, r)
        w.WriteHeader(http.StatusInternalServerError)
    }))
    w = httptest.NewRecorder()
    handler.ServeHTTP(w, r)

    result = w.Result()
    if result.StatusCode != 404 {
        t.Errorf("StatusCode should have been 404 but was %d", result.StatusCode)
    }

49 wasn't enough to fix this unfortunately.

[EmielM]

Yep you're right. Added some code in PR #59, don't know how attribution works on github but perhaps the repo owner can add you as well?