CompressHandler: Ignore MIME types, re-use writers via sync.Pool

[elithrar]

Note: raising this as an issue half as a reminder to address this, and to provide someone a chance to get to it before I can.

As per this great article on gzip performance: http://blog.klauspost.com/gzip-performance-for-go-webservers/

• We should re-use gzip.NewWriter via a sync.Pool (being very conscious of the need to explicitly Reset() items coming out of the pool)
• We should (which I meant to raise a while ago) ignore already compressed content types by inspecting the Content-Type header here: https://github.com/gorilla/handlers/blob/master/compress.go#L30-L38 - also see here for relevant types: https://support.cloudflare.com/hc/en-us/articles/200168396-What-will-CloudFlare-gzip-

Quick example:

func (w *compressResponseWriter) Write(b []byte) (int, error) {
    // You'll need to *not* write the gzip content-encoding header based on the type.   
    h := w.ResponseWriter.Header()
    var ctype string
    if ctype = h.Get("Content-Type"); ctype == "" {
        ctype  = http.DetectContentType(b)
        h.Set("Content-Type", ctype)
    }

    // compressableTypes checks types against a []string of acceptable Content-Types
    if !compressableTypes(ctype) {
        // Write the uncompressed bytes to the response.
        w.ResponseWriter.Write(b)
        return
    }

    h.Del("Content-Length")
    return w.Writer.Write(b)
}

I'll update here before I begin work in earnest, but it won't be until after the weekend as I need to submit patches to the sessions stores first. If someone wants to pick this up before then (it's fairly straightforward patch) then just say so. Happy to guide you in writing tests as well.

[elithrar]

In addition:

• Tests need to explicitly check that a *gzip.Writer retrieved from the pool is correctly Reset(). This is a potential security issue waiting to happen otherwise.
• Use a base64 encoded PNG to test the "don't gzip" case - http://www.gorillatoolkit.org/static/images/gorilla-icon-64.png is a good candidate (drop the base64 bytes into a var, decode into bytes during the test)
• Update the docs on CompressHandler to indicate the selective gzipping.

[nwidger]

+1

[elithrar]

#48 has likely made this much harder now due to the limitations around defining a global pool:

• If we create a global pool of gzip writers (we must if we want a pool) then we can't pass the "current" level to the pool per-instance of middleware:

var gzipPool = sync.Pool{
    // Can't pass the level when we call New()
    New: func() interface{} {
        return gzip.NewWriter(nil)
    },
}

• We could set a package global level, but that creates implicit behaviour (all CompressHandler instances would need to use it) and also causes race conditions when you have more than one CompressHandlerLevel middleware in play. Effectively a non-starter here.
• The level field on *gzip.Writer isn't exported and therefore you can't modify it after you type-assert the writer out of the pool.

Is there a solution here that I'm missing? I think this PR had more value as changing gzip levels should be done with caution in a HTTP context, but a pooled gzip writer nets everyone a performance gain, but unfortunately that's how things have shaken out.

@kisielk - would appreciate your feedback/input here.

[kisielk]

How about a separate pool for every compression level? There's only 11 of them and the size overhead for a pool is not that large.

[bernardolm]

👍

[stale]

This issue has been automatically marked as stale because it hasn't seen a recent update. It'll be automatically closed in a few days.