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

GzipResponseWriter must implement CloseNotifier if ResponseWriter implement it #62

Closed juliens closed 6 years ago

juliens commented 6 years ago

GzipResponseWriter doesn't implement CloseNotifier.

We test if the underlying ResponseWriter implement it and if it does, we use it. If it doesn't, we do not implement it in order to leave the management of the problem to the user.

jprobinson commented 6 years ago

Hi there, @juliens!

Do you have a specific use case that requires this? We’ve discussed the CloseNotifier interface before here: https://github.com/NYTimes/gziphandler/pull/10

juliens commented 6 years ago

Hi,

I use it with https://golang.org/pkg/net/http/httputil/#NewSingleHostReverseProxy. The fact that GzipWriter doesn't implement CloseNotify break the chain, If I have a long connection and if the client break the connection, the server connection is not close correctly.

jprobinson commented 6 years ago

Sorry for the delay. After reading the docs, it looks like things have changed a bit since the discussion in #10 so I'm fine with this coming in.

Would you mind adding a test for your changes?

juliens commented 6 years ago

Done 😄

jprobinson commented 6 years ago

Thanks! 🌯 🎆