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

Implement the http.CloseNotifier interface #10

Closed jpillora closed 6 years ago

jpillora commented 8 years ago

I've got a chain of middleware and the outer layer is doing w.(http.CloserNotifier).CloseNotify() - basically assuming a stdlib http.ResponseWriter. I was thinking about making it return nil if !ok though I think a panic is more appropriate here.

adammck commented 8 years ago

Thanks for the patch! I'm not familiar with the CloseNotifier interface-- can you clarify for me how/why you're using this?

jpillora commented 8 years ago

No worries, I'm using https://github.com/donovanhide/eventsource (SSE/EventSource) which inturn uses https://golang.org/pkg/net/http/#CloseNotifier. The eventsource package requires it to monitor the state of the underlying TCP connection.

jpillora commented 8 years ago

I just realised, in rare situations, this could break existing programs. There are three options I can see:

  1. Close the PR, don't support.
  2. If underlying response writer isn't a CloseNotifier, return a new channel that never returns.
  3. If underlying response writer isn't a CloseNotifier, return a new channel that we close after we've closed off the gzip writer.
chuyeow commented 7 years ago

If underlying response writer isn't a CloseNotifier, return a new channel that never returns.

I think returning a channel that never returns is closest to the "default" behavior. http.Handlers that do not use the http.CloseNotifier interface are effectively saying that they do not care when the client disconnects early, so a channel that never returns models that behavior.

jpillora commented 6 years ago

This can now be closed in favour of ctx.Done() (https://golang.org/pkg/context/#Context)