defer Close your gzip handler will prevent panics from writing http headers
azr opened this issue · 7 comments
Hello there !
https://github.com/NYTimes/gziphandler/blob/master/gzip.go#L115
This causes StatusCode to be 200 on panics.
Defer will close the gzip handler before the http handler to catch the panic
We had the same issue here : https://github.com/BakedSoftware/go-parameters/pull/6/files
Sorry I was totally wrong for you :)
Whoops I actually found a way to reproduce: https://play.golang.org/p/zDqhbl4Nnn
log outputs : 2016/08/22 17:24:16 http: multiple response.WriteHeader calls
$ curl -H "Accept-Encoding: gzip" -i localhost:8000
HTTP/1.1 200 OK
Content-Encoding: gzip
Vary: Accept-Encoding
Date: Mon, 22 Aug 2016 15:24:18 GMT
Content-Length: 23
Content-Type: application/x-gzip
� n����%
Ouch! That's a nasty bug. Thank you very much for reporting it, and especially for including a POC. The solution isn't immediately obvious to me, but so long as we call Reset before attempting to re-use the gzip writers (which we do), I see no harm in simply not calling Close when a panic occurs. Perhaps we should (partially) revert #8?
You could also just call Reset explicitly instead of Close (as a
precaution).
On Mon, Aug 22, 2016 at 8:46 AM Adam Mckaig notifications@github.com
wrote:
Ouch! That's a nasty bug. Thank you very much for reporting it, and
especially for including a POC. The solution isn't immediately obvious to
me, but so long as we call Reset before attempting to re-use the gzip
writers (which we do), I see no harm in simply not calling Close when a
panic occurs. Perhaps we should (partially) revert #8
#8?—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#15 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AABIcLfhMi0aryIkpJ0M0fZPHXhWvisGks5qicRQgaJpZM4Jp5xq
.
We already call Reset immediately after grabbing a gzw from the pool, so I think omitting the Close should be okay.
Hey, you're welcome :)