Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(395)

Issue 4245064: code review 4245064: http: add Hijacker type; remove Hijack from ResponseWriter (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
10 years, 9 months ago by bradfitz
Modified:
10 years, 9 months ago
Reviewers:
bradfitzgoog, gburd, jnw, adg
CC:
rsc, golang-dev
Visibility:
Public.

Description

http: add Hijacker type; remove Hijack from ResponseWriter The Hijack functionality wasn't removed, but now you have to test if your ResponseWriter is also a Hijacker: func ServeHTTP(rw http.ResponseWriter, req *http.Request) { if hj, ok := rw.(http.Hijacker); ok { hj.Hijack(..) } }

Patch Set 1 #

Patch Set 2 : diff -r 0789311c2e60 https://go.googlecode.com/hg/ #

Patch Set 3 : diff -r 0789311c2e60 https://go.googlecode.com/hg/ #

Patch Set 4 : diff -r 0789311c2e60 https://go.googlecode.com/hg/ #

Patch Set 5 : diff -r 1ba6a90e1545 https://go.googlecode.com/hg/ #

Total comments: 4
Unified diffs Side-by-side diffs Delta from patch set Stats (+6 lines, -12 lines) Patch
M src/pkg/http/httptest/recorder.go View 1 2 2 chunks +0 lines, -9 lines 0 comments Download
M src/pkg/http/server.go View 1 2 3 4 1 chunk +3 lines, -0 lines 4 comments Download
M src/pkg/rpc/server.go View 1 1 chunk +1 line, -1 line 0 comments Download
M src/pkg/websocket/server.go View 1 2 chunks +2 lines, -2 lines 0 comments Download

Messages

Total messages: 16
bradfitz
Hello rsc (cc: golang-dev@googlegroups.com), I'd like you to review this change to https://go.googlecode.com/hg/
10 years, 9 months ago (2011-03-07 02:07:35 UTC) #1
bradfitz
Hello rsc (cc: golang-dev@googlegroups.com), Please take another look.
10 years, 9 months ago (2011-03-07 02:07:47 UTC) #2
rsc
LGTM s/HiJack/Hijack/ in the CL description.
10 years, 9 months ago (2011-03-07 02:36:53 UTC) #3
bradfitz
*** Submitted as http://code.google.com/p/go/source/detail?r=06fb9544cf52 *** http: add Hijacker type; remove Hijack from ResponseWriter The Hijack ...
10 years, 9 months ago (2011-03-07 02:59:54 UTC) #4
jnw
This feels a whole lot less kludgy (somehow) than the current method and works well ...
10 years, 9 months ago (2011-03-07 17:35:03 UTC) #5
jnw
On 2011/03/07 17:35:03, jnw wrote: > This feels a whole lot less kludgy (somehow) than ...
10 years, 9 months ago (2011-03-07 17:36:03 UTC) #6
bradfitz
I'm happy with any documentation changes if Russ & others are cool with them. I'm ...
10 years, 9 months ago (2011-03-07 17:40:07 UTC) #7
jnw
On Mon, Mar 7, 2011 at 5:40 PM, Brad Fitzpatrick <bradfitz@golang.org> wrote: > I'm happy ...
10 years, 9 months ago (2011-03-07 18:02:17 UTC) #8
gburd
http://codereview.appspot.com/4245064/diff/6002/src/pkg/http/server.go File src/pkg/http/server.go (right): http://codereview.appspot.com/4245064/diff/6002/src/pkg/http/server.go#newcode93 src/pkg/http/server.go:93: Hijack() (io.ReadWriteCloser, *bufio.ReadWriter, os.Error) How about returning (conn net.Conn, ...
10 years, 9 months ago (2011-03-07 19:02:08 UTC) #9
bradfitzgoog
http://codereview.appspot.com/4245064/diff/6002/src/pkg/http/server.go File src/pkg/http/server.go (right): http://codereview.appspot.com/4245064/diff/6002/src/pkg/http/server.go#newcode93 src/pkg/http/server.go:93: Hijack() (io.ReadWriteCloser, *bufio.ReadWriter, os.Error) On 2011/03/07 19:02:08, gburd wrote: ...
10 years, 9 months ago (2011-03-07 19:06:39 UTC) #10
rsc
I'm okay with the conn net.Conn. I'm less okay with changing bufio.Reader to []byte. Among ...
10 years, 9 months ago (2011-03-07 19:08:18 UTC) #11
gburd
On 2011/03/07 19:08:18, rsc wrote: > Among other things there's no API to do that ...
10 years, 9 months ago (2011-03-07 19:16:55 UTC) #12
adg
On 8 March 2011 06:02, <gary.burd@gmail.com> wrote: > This approach allows the caller to make ...
10 years, 9 months ago (2011-03-07 21:59:10 UTC) #13
rsc
> Will they always be getting a network connection, though? If you want > to ...
10 years, 9 months ago (2011-03-07 22:01:56 UTC) #14
adg
On 8 March 2011 09:01, Russ Cox <rsc@golang.org> wrote: >> Will they always be getting ...
10 years, 9 months ago (2011-03-07 22:33:01 UTC) #15
bradfitzgoog
10 years, 9 months ago (2011-03-07 23:23:13 UTC) #16
On Mon, Mar 7, 2011 at 2:32 PM, Andrew Gerrand <adg@golang.org> wrote:

> On 8 March 2011 09:01, Russ Cox <rsc@golang.org> wrote:
> >> Will they always be getting a network connection, though? If you want
> >> to access the connection you can always do a type assertion (which
> >> might fail). I prefer the more general ReadWriteCloser. (but maybe we
> >> could call it 'conn' in the function signature?)
> >
> > I thought through that but found that http.Serve only takes
> > a net.Listener.  I think it's fine to assume that if you're hijacking
> > the connection it is a network connection.  We're not in the
> > realm of generality anymore.  There's already been an
> > interface test to see if w implements Hijacker at all.
>
> Makes sense. And I suppose if you wanted to implement a mock Hijacker
> implementing a mock net.Conn is not that big a deal.
>

And if you're talking about actual mocking in the testing definition of the
term, your mock library should automatically generate mocks from interface
types.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b