compress/gzip: When using gzip with http handlers, panics can't write headers #16830

azr · 2016-08-22T14:24:02Z

Hello there,

Since this :

Line 235 in d0e8d3a

z.Write(nil)

For a lot of libraries that use defer close to Close gzip in http handlers it's going to be impossible to call WriteHeader in the event of a panic, this a call to Write will default the headers to 200.

Found it there : https://github.com/BakedSoftware/go-parameters/pull/6/files

And there : nytimes/gziphandler#15

This issue is just here to warn you guys up.
It would be nice to find a way to warn all the other libs etc.

Cheers !
A.

bradfitz · 2016-08-22T15:32:36Z

I don't see how this is a problem.

The panic rules, defer rules, and HTTP ServeHTTP rules (around headers, content-type sniffing, flushing) are all clearly defined, and nothing precludes all these features from being used together.

This also isn't a recent change as far as I know.

azr · 2016-08-22T15:39:34Z

Here is a snippet : https://play.golang.org/p/3hPGBmN5V8

$ curl -H "Accept-Encoding: gzip" -i localhost:8000/catch
HTTP/1.1 200 OK
Date: Mon, 22 Aug 2016 15:34:38 GMT
Content-Length: 23
Content-Type: application/x-gzip
[...]

$ curl -H "Accept-Encoding: gzip" -i localhost:8000/nocatch                                                                                                                                                                              $ curl -H "Accept-Encoding: gzip" -i localhost:8000/nocatch
curl: (52) Empty reply from server

To me it was clear that ServeHTTP recovered panics and wrote 500 errors.

May be I misread something.

This behavior changed to me, I guess.

bradfitz · 2016-08-22T15:42:35Z

I would need to see code too, not just your curl output. And if it changed, when did it change?

azr · 2016-08-22T15:44:57Z

Follow the link ! :)

Or here it is :

package main

import (
    "compress/gzip"
    "io"
    "net/http"
)

type gzipResponseWriter struct {
    io.Writer
    http.ResponseWriter
}

func (w gzipResponseWriter) Write(b []byte) (int, error) {
    return w.Writer.Write(b)
}

// EnableGZIP will attempt to compress the response if the client has passed a
// header value for Accept-Encoding which allows gzip
func EnableGZIP(fn http.Handler) http.Handler {
    return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
        gz := gzip.NewWriter(w)
        defer gz.Close()
        gzr := gzipResponseWriter{Writer: gz, ResponseWriter: w}
        fn.ServeHTTP(gzr, r)
    })
}

func CatchPanic(next http.Handler) http.Handler {
    return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
        defer func() {
            if err := recover(); err != nil {
                w.WriteHeader(http.StatusInternalServerError)
            }
        }()
        next.ServeHTTP(w, r)
    })
}

func main() {
    handler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
        panic("banana")
    })

    http.Handle("/nocatch", EnableGZIP(handler))

    http.Handle("/catch", CatchPanic(EnableGZIP(handler)))

    http.ListenAndServe("0.0.0.0:8000", nil)
}

It changed when I started using 1.7 sorry I didn't mention that ...

azr · 2016-08-22T15:46:43Z

In fact even a simple handler that panics :

package main

import "net/http"

func main() {
    handler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
        panic("banana")
    })

    http.Handle("/", handler)

    http.ListenAndServe("0.0.0.0:8000", nil)
}

Gets me an empty response, may be bananas are too slipery

Edit: installed go from brew :

$ pwd ; git status
/usr/local/Cellar/go/1.7
On branch master
nothing to commit, working tree clean

bradfitz · 2016-08-22T17:31:50Z

It changed when I started using 1.7 sorry I didn't mention that ...

From what version? The code you reference is not new at all:

c9150003 src/pkg/compress/gzip/gzip.go (Nigel Tao           2010-01-29 11:00:05 +1100 233)      z.closed = true
db12f9d4 src/pkg/compress/gzip/gzip.go (Brad Fitzpatrick    2013-08-30 11:41:12 -0700 234)      if !z.wroteHeader {
c9150003 src/pkg/compress/gzip/gzip.go (Nigel Tao           2010-01-29 11:00:05 +1100 235)              z.Write(nil)
c9150003 src/pkg/compress/gzip/gzip.go (Nigel Tao           2010-01-29 11:00:05 +1100 236)              if z.err != nil {
c9150003 src/pkg/compress/gzip/gzip.go (Nigel Tao           2010-01-29 11:00:05 +1100 237)                      return z.err
c9150003 src/pkg/compress/gzip/gzip.go (Nigel Tao           2010-01-29 11:00:05 +1100 238)              }
c9150003 src/pkg/compress/gzip/gzip.go (Nigel Tao           2010-01-29 11:00:05 +1100 239)      }

And there's no interesting diff from Go 1.6 to Go 1.7 or even from Go 1.5 to Go 1.7.

So, nothing to do here. This behavior has been unmodified for quite some time.

People writing gzip http.Handlers will have to deal with panics.

azr · 2016-08-23T09:37:40Z

On gzip handling:
Okay, sorry for bothering you then, I expected these libs to be working from the beginning; in fact I only started seeing those multiple WriteHeader calls errors from logs only after upgrading servers to 1.7, I got lucky and made wrong assumptions.

On the http server recovering panic:
Sorry again, I also expected to get a header 500 on an panic by default and I was wrong, that's specific behavior.
I expected this since Writing anything would add headers 200 and a default behavior of writing a 500 on an unrecovered panic seems like an idea golang people would also have.

Lessons learned: don't expect anything.

Here is an interesting snippet containing all that wrongitude that behaves exactly the same on 1.6 & 1.7:

package main

import "net/http"

func DeferWriteNil(next http.Handler) http.Handler {
    return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
        defer w.Write(nil)
        next.ServeHTTP(w, r)
    })
}

func CatchPanic(next http.Handler) http.Handler {
    return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
        defer func() {
            if err := recover(); err != nil {
                w.WriteHeader(http.StatusInternalServerError)
            }
        }()
        next.ServeHTTP(w, r)
    })
}

func main() {
    panicker := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
        panic("banana")
    })

    http.Handle("/panic", panicker) // empty http response !!!
    http.Handle("/", CatchPanic(DeferWriteNil(panicker))) // 200 !!!
    http.ListenAndServe("0.0.0.0:8000", nil)
}

bradfitz closed this as completed Aug 22, 2016

golang locked and limited conversation to collaborators Aug 23, 2017

gopherbot added the FrozenDueToAge label Aug 23, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

compress/gzip: When using gzip with http handlers, panics can't write headers #16830

compress/gzip: When using gzip with http handlers, panics can't write headers #16830

azr commented Aug 22, 2016 •

edited

bradfitz commented Aug 22, 2016

azr commented Aug 22, 2016

bradfitz commented Aug 22, 2016

azr commented Aug 22, 2016

azr commented Aug 22, 2016 •

edited

bradfitz commented Aug 22, 2016

azr commented Aug 23, 2016 •

edited

compress/gzip: When using gzip with http handlers, panics can't write headers #16830

compress/gzip: When using gzip with http handlers, panics can't write headers #16830

Comments

azr commented Aug 22, 2016 • edited

bradfitz commented Aug 22, 2016

azr commented Aug 22, 2016

bradfitz commented Aug 22, 2016

azr commented Aug 22, 2016

azr commented Aug 22, 2016 • edited

bradfitz commented Aug 22, 2016

azr commented Aug 23, 2016 • edited

azr commented Aug 22, 2016 •

edited

azr commented Aug 22, 2016 •

edited

azr commented Aug 23, 2016 •

edited