support for H2 Push

[romainmenke]

add support for H2 Push without removing support for previous Go versions.

28

[jprobinson]

Hi there, @romainmenke. Thanks for getting things started on the H2 Push!

This looks pretty clean, but I'm not sure we need the entire docs from the net/http package on there. A reference similar to what we have for http.Hijacker is probably enough.

Also, please create a test for these changes.

[alexandrestein]

Hi,

I'm not sure this would works as expected. The push are sent to clients, but I would expect that the pushed resources to be compressed if the first request is. It looks like in this implementation, only the original request is GZIP.

Am'I right?

[romainmenke]

@alexandreStein that is not how H2 Push works in Go.

Push does not handle the second request. It sends an H2 Push frame to the client. It also creates the second request for the server to handle. This second request would go through the gziphandler just like the first. Otherwise routing and middleware would all have to be implemented twice for H2 Push.

This is not immediately clear at first as it all happens "auto-magically". A good way to verify this behaviour is to setup a test server with a hardcoded Push.

Set a custom header value in PushOptions

pusher.Push("/relative-path/some-resource", &http.PushOptions{
    Header: http.Header{
        "Go-H2-Push": "X",
    },
})

Verify that the header value appears.

func someHandler(w http.ResponseWriter, r *http.Request) {
        if r.Header.Get("Go-H2-Push") == "X" {
        fmt.Println("this did not come from the client")
    }
}

---

@jprobinson Haven't had time yet to write a descent test for this, but I have already shortened the docs.

[alexandrestein]

I made a test server to check this.

I didn't try to call the server from go because I believe the target is more probably the web browsers over direct language requests. I don't really see the point of doing this except for web pages.

The result is that Push are working, but not gziped.

• With Chrome (I'm using Chromium on Linux) no headers are sent.
• With Fire Fox headers are set but the response still not compressed.

The source of the server can be find here and the working server is here.

[romainmenke]

@alexandreStein You haven't passed any headers from the original request to the Push call. It is not the responsibility of this package to add these headers, it is the responsibility of the original caller.

If you add these PushOptions it will work as expected. (hard-coded to illustrate)

pusher.Push(link, &http.PushOptions{
    Header: http.Header{
        "Accept-Encoding": []string{"gzip"},
    },
})

[alexandrestein]

Your are right. This work has expected with the header.

But I would personally expect the package to do that.

Is it an option to make the GzipResponseWriter.Push look like this:

func (w *GzipResponseWriter) Push(target string, opts *http.PushOptions) error {
    // Try to get the pusher interface.
    pusher, ok := w.ResponseWriter.(http.Pusher)
    if ok && pusher != nil {
        if opts == nil {
            opts = &http.PushOptions{Header: getHeader()}
        } else {
            if opts.Header != nil {
                opts.Header = getHeader()
            } else {
                opts.Header.Set(acceptEncoding, "gzip")
            }
        }
        return pusher.Push(target, opts)
    }
    return http.ErrNotSupported
}

func getHeader() http.Header {
    return http.Header{
        acceptEncoding: []string{"gzip"},
    }
}

That way, it's transparent for caller.

Thanks for your tips.

[romainmenke]

I agree that such behaviour would be helpful to many people implementing H2 Push. How about this :

func (w *GzipResponseWriter) Push(target string, opts *http.PushOptions) error {
    // Try to get the pusher interface.
    pusher, ok := w.ResponseWriter.(http.Pusher)
    if !ok || pusher == nil {
        return http.ErrNotSupported
    }

    if opts == nil {
        opts = &http.PushOptions{
            Header: http.Header{
                acceptEncoding: []string{"gzip"},
            },
        }
        return pusher.Push(target, opts)
    }

    if opts.Header == nil {
        opts.Header = http.Header{
            acceptEncoding: []string{"gzip"},
        }
        return pusher.Push(target, opts)
    }

    return pusher.Push(target, opts)
}

If the http.PushOptions or http.PushOptions.Header is nil the Accept-Encoding header is added. However if there are Headers then nothing is altered. This still fixes the issue, but doesn't force compression on pushes.

Or maybe restrict it to not overriding the Accept-Encoding header?

[alexandrestein]

This is even better. What about add an other condition:

if encoding := opts.Header.Get(acceptEncoding); encoding == "" {
    opts.Header.Add(acceptEncoding, "gzip")
    return pusher.Push(target, opts)
}

That way caller can add headers without "accidentally" disable compression.

[romainmenke]

@alexandreStein I moved the logic for adding the Accept-Encoding header to a separate func to facilitate tests. This also made it possible to rewrite Push to a more idiomatic form.

@jprobinson I looked into writing a test for the Push func. However H2 Push is not yet supported in the http Client. This means it is not possible at this point to create a test that checks both the original request/response and the pushed request/response.

It is possible to create a test that checks if the server receives the Push request.

[alexandrestein]

@romainmenke this looks really good to me; but I'm not the one to convince. :smile:

I'm thinking about a edge case. On the premise that Push is supported, it's very unlikely probable that client don't support GZIP.

But maybe save the "Accept-Encoding" value of the initial caller into the GzipResponseWriter struct can prevent the possible issue of sending GZIP content to a client which not support it.

A more programatic and less hardcodeed solution would maybe looks like this:

func (w *GzipResponseWriter) setAcceptEncodingForPushOptions(opts *http.PushOptions) *http.PushOptions {

    if opts == nil {
        opts = &http.PushOptions{
            Header: http.Header{
                acceptEncoding: w.clientAcceptEncoding,
            },
        }
        return opts
    }

    if opts.Header == nil {
        opts.Header = http.Header{
            acceptEncoding: w.clientAcceptEncoding,
        }
        return opts
    }

    if encoding := opts.Header.Get(acceptEncoding); encoding == "" {
        opts.Header[acceptEncoding] = w.clientAcceptEncoding
        return opts
    }

    return opts
 }

Is it a good idea?

[romainmenke]

@alexandreStein I believe it is impossible for a client that doesn't support GZIP to end up in this flow. https://github.com/NYTimes/gziphandler/blob/master/gzip.go#L175-L185

So we can safely assume that the client will always support GZIP. We however cannot assume anything about other middleware, which is why I would not override headers, but adding them seems perfectly safe to me.

[alexandrestein]

OK, it looks like you are right. Sorry about that.

[jprobinson]

Sorry for the delay, gang. This LGTM. Pulling in.