Gzip based on response content type

[sajal]

I see now GzipResponseWriter can decide if it wants to gzip the response based on response size.

I suggest also filtering on content-types. The types could be user configured or hardcoded to sane defaults

if _, ok := w.Header()[contentType]; !ok {
    // It infer it from the uncompressed body.
    w.Header().Set(contentType, http.DetectContentType(b))
}
//Use the content-type information to decide between `w.startGzip()` and `w.startNoGzip()`

[flimzy]

This sounds like a nice idea, but the combinatorial explosion of config options might be getting out of hand.

[sajal]

How about a config struct with sane defaults?

On Mon, 3 Apr 2017, 06:33 Jonathan Hall, notifications@github.com wrote:

This sounds like a nice idea, but the combinatorial explosion of config options might be getting out of hand.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/NYTimes/gziphandler/issues/40#issuecomment-291023115, or mute the thread https://github.com/notifications/unsubscribe-auth/AABhr-7QaFDFeUd4xAYFX_q-ST5d2KyVks5rsDBDgaJpZM4Muq3B .

[adammck]

I like config structs, but I don't think this functionality belongs in this middleware. I'm not strongly against it, and the implementation would be trivial, but I'd like to hear some concrete use-cases first. I rarely encounter endpoints which may or may not want their responses compressed.

[sajal]

For API servers, sending content uncompressed is not needed. For this use-case the current behaviour is perfect. This is the primary way I too use this middleware.

But if I am writing a server that serves full website (images, videos, binary, etc) then gziping the payload is often redundant. Practically no full-feature webserver would compress jpg, png, mp4, etc since these formats dont have enough gain. This is especially true for videos... running on-the-fly gzip on 1 GB video file to save few KBs is quite wasteful. In this case, I had to write my own filter based on file-extension to enable/disable compression, but I would rather do it based off response Content-Type.

[romainmenke]

On config / options : functional options

Personally I really like this approach.

[adammck]

@romainmenke: me, too :smile:

[ConradIrwin]

I'd like this — we proxy assets through go right now (which we arguably shouldn't), and it'd be nice to compress the javascript files but not the pngs.

[holygeek]

Another point to take into consideration is to not gzip an already gzipped content, something like this:

$ git diff
diff --git gzip.go gzip.go
index c47039f..36ccd1b 100644
--- gzip.go
+++ gzip.go
@@ -102,10 +102,11 @@ func (w *GzipResponseWriter) Write(b []byte) (int, error) {
        // On the first write, w.buf changes from nil to a valid slice
        w.buf = append(w.buf, b...)

+       _, alreadyEncoded := w.Header()[contentEncoding]
        // If the global writes are bigger than the minSize and we're about to write
        // a response containing a content type we want to handle, enable
        // compression.
-       if len(w.buf) >= w.minSize && handleContentType(w.contentTypes, w) {
+       if !alreadyEncoded && len(w.buf) >= w.minSize && handleContentType(w.contentTypes, w) {
                err := w.startGzip()
                if err != nil {
                        return 0, err

I'm currently using gziphandler in a server that serves content either "locally" from my server, or reverse proxied off from other servers. Currently I'm using a stack of http handlers that includes gziphandler, and for all requests that are to be proxied I do r.Header.Del("Accept-Encoding") which means the content will be gzipped "locally" by gziphandler. To avoid this (and to reduce CPU usage on my server) I would have to create two stacks of http handlers -one with gziphandler and another without so that all the gzipping is done at the proxied servers and my server would then just pass the response through without modification. If gziphalder had the above code then I don't have to create two stacks for http handlers, which could simplify my code.

[jprobinson]

You all win :grin: This has been resolved with #55

[flimzy]

That PR resolves the issue of not recompressing responses with the content-encoding header, but this issue is about not compressing responses based on the content-type header.

[tmthrgd]

This was fixed with the merging of #51.

[dolmen]

If this is fixed, it might be time for close of this issue, isn't it? I'm discovering this project and most open issues seem to be in fact done. Discovering that after the read of a long discussion is unexpected.