Http2 support for middleware, fixes #123

[pkieltyka]

cc: @lclarkmichalek

[lclarkmichalek]

Sorry for being so slow to get to this! Looks good, I like the changes to reduce the amount of stuff in the 1.8 specific files. I've added a couple of comments. This doesn't handle the compression middleware atm I think?

[lclarkmichalek]

This is much nicer

[lclarkmichalek]

I don't think HTTP2 requests have ReaderFrom

[lclarkmichalek]

This handler should be wrapped in a middleware to have this test check that the response writer still has the interfaces expected after wrapping.

[lclarkmichalek]

Now I think about it, these messages should probably all read ResponseWriter should have been ...

[pkieltyka]

Yea it's still missing support for compress18 I realize it makes sense for chi to support the current and one back version going forward. So 18 and 17 now, and eventually just 19 and 18, which should reduce these nuances as http2 will be mature by then

…

On Jan 5, 2017, at 6:00 PM, Laurie Clark-Michalek ***@***.***> wrote: @lclarkmichalek commented on this pull request. Looks good, I like the changes to reduce the amount of stuff in the 1.8 specific files. I've added a couple of comments. This doesn't handle the compression middleware atm I think? In middleware/wrap_writer17.go: > +package middleware + +import ( + "io" + "net/http" +) + +// NewWrapResponseWriter wraps an http.ResponseWriter, returning a proxy that allows you to +// hook into various parts of the response process. +func NewWrapResponseWriter(w http.ResponseWriter, protoMajor int) WrapResponseWriter { + _, cn := w.(http.CloseNotifier) + _, fl := w.(http.Flusher) + _, rf := w.(io.ReaderFrom) + + bw := basicWriter{ResponseWriter: w} + if protoMajor == 2 { This is much nicer In middleware/wrap_writer18.go: > +import ( + "io" + "net/http" +) + +// NewWrapResponseWriter wraps an http.ResponseWriter, returning a proxy that allows you to +// hook into various parts of the response process. +func NewWrapResponseWriter(w http.ResponseWriter, protoMajor int) WrapResponseWriter { + _, cn := w.(http.CloseNotifier) + _, fl := w.(http.Flusher) + _, rf := w.(io.ReaderFrom) + + bw := basicWriter{ResponseWriter: w} + if protoMajor == 2 { + _, ps := w.(http.Pusher) + if cn && fl && ps && rf { I don't think HTTP2 requests have ReaderFrom In middleware/middleware18_test.go: > + } + _, rf := w.(io.ReaderFrom) + if rf { + t.Fatal("request should not have been a io.ReaderFrom") + } + _, ps := w.(http.Pusher) + if !ps { + t.Fatal("request should have been a http.Pusher") + } + + w.Write([]byte("OK")) + }) + + server := http.Server{ + Addr: ":7072", + Handler: handler, This handler should be wrapped in a middleware to have this test check that the response writer still has the interfaces expected after wrapping. In middleware/middleware18_test.go: > + "net/http" + "testing" + "time" + + "golang.org/x/net/http2" +) + +// NOTE: we must import `golang.org/x/net/http2` in order to explicitly enable +// http2 transports for certain tests. The runtime pkg does not have this dependency +// though as the transport configuration happens under the hood on go 1.7+. + +func TestWrapWriterHTTP2(t *testing.T) { + handler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + _, cn := w.(http.CloseNotifier) + if !cn { + t.Fatal("request should have been a http.CloseNotifier") Now I think about it, these messages should probably all read ResponseWriter should have been ... — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or mute the thread.

[pkieltyka]

all done. thanks for the feedback