code review 4245064: http: add Hijacker type; remove Hijack from ResponseWriter

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(..)
    }
}

[bradfitz, 2011-03-07 02:07:35]

Hello rsc (cc: golang-dev@googlegroups.com),

I'd like you to review this change to
https://go.googlecode.com/hg/

[bradfitz, 2011-03-07 02:07:47]

Hello rsc (cc: golang-dev@googlegroups.com),

Please take another look.

[rsc, 2011-03-07 02:36:53]

LGTM

s/HiJack/Hijack/ in the CL description.

[bradfitz, 2011-03-07 02:59:54]

*** Submitted as http://code.google.com/p/go/source/detail?r=06fb9544cf52 ***

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(..)
    }
}

R=rsc
CC=golang-dev
http://codereview.appspot.com/4245064

[jnw, 2011-03-07 17:35:03]

This feels a whole lot less kludgy (somehow) than the current method and works
well with my framework. Just a few minor comments:

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#newcode86
src/pkg/http/server.go:86: // A Hijacker is an HTTP request which be taken over
by an HTTP handler.
A Hijacker is not a request, which already has quite clear meaning in HTTP-land,
but an interface that enables the hijacking of an HTTP connection.

Really what it is doing is providing a way to break the encapsulation of an
abstract "http connection", providing access to the underlying rwc/rw.
"A Hijacker provides a way to break encapsulation of an HTT

http://codereview.appspot.com/4245064/diff/6002/src/pkg/http/server.go#newcod...
src/pkg/http/server.go:475: // Hijack impements the ResponseWriter.Hijack
method.
Hijack implements the Hijacker.Hijack method

[jnw, 2011-03-07 17:36:03]

On 2011/03/07 17:35:03, jnw wrote:
> This feels a whole lot less kludgy (somehow) than the current method and works
> well with my framework. Just a few minor comments:
> 
> 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#newcode86
> src/pkg/http/server.go:86: // A Hijacker is an HTTP request which be taken
over
> by an HTTP handler.
> A Hijacker is not a request, which already has quite clear meaning in
HTTP-land,
> but an interface that enables the hijacking of an HTTP connection.
> 
> Really what it is doing is providing a way to break the encapsulation of an
> abstract "http connection", providing access to the underlying rwc/rw.
> "A Hijacker provides a way to break encapsulation of an HTT
> 
>
http://codereview.appspot.com/4245064/diff/6002/src/pkg/http/server.go#newcod...
> src/pkg/http/server.go:475: // Hijack impements the ResponseWriter.Hijack
> method.
> Hijack implements the Hijacker.Hijack method

And that's what I get for having a cached version of the CL up.. I didn't even
know it had already been submitted O.o!

- Jim

[bradfitz, 2011-03-07 17:40:07]

I'm happy with any documentation changes if Russ & others are cool with
them.  I'm still learning Go documentation style.  :-)


On Mon, Mar 7, 2011 at 9:36 AM, <jnwhiteh@gmail.com> wrote:

> On 2011/03/07 17:35:03, jnw wrote:
>
>> This feels a whole lot less kludgy (somehow) than the current method
>>
> and works
>
>> well with my framework.
>
>
I had trouble parsing that sentence.  You like Hijacker or not?

[jnw, 2011-03-07 18:02:17]

On Mon, Mar 7, 2011 at 5:40 PM, Brad Fitzpatrick <bradfitz@golang.org> wrote:
> I'm happy with any documentation changes if Russ & others are cool with
> them.  I'm still learning Go documentation style.  :-)
>
> On Mon, Mar 7, 2011 at 9:36 AM, <jnwhiteh@gmail.com> wrote:
>>
>> On 2011/03/07 17:35:03, jnw wrote:
>>>
>>> This feels a whole lot less kludgy (somehow) than the current method
>>
>> and works
>>>
>>> well with my framework.
>
> I had trouble parsing that sentence.  You like Hijacker or not?

It still feels a bit kludgy to me, but its much better than the
previous method. I like it, since something like hijacking an HTTP
connection should feel a bit unsavoury. As for the documentation, I'm
not very good with it either, but I want to make sure that we don't
confuse anyone by making claims that just aren't true =)

- Jim

[gburd, 2011-03-07 19:02:08]

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, bufferedData []byte, os.Error) ?  The
bufferedData is data that was read by the HTTP server, but not yet processed.

This approach allows the caller to make it's own decisions about how the
connection is buffered. It also makes it clear that the caller is getting a
network connection.

[bradfitzgoog, 2011-03-07 19:06:39]

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:
> How about returning (conn net.Conn, bufferedData []byte, os.Error) ?  The
> bufferedData is data that was read by the HTTP server, but not yet processed.
> 
> This approach allows the caller to make it's own decisions about how the
> connection is buffered. It also makes it clear that the caller is getting a
> network connection.

Works for me.  Russ?

[rsc, 2011-03-07 19:08:18]

I'm okay with the conn net.Conn.  I'm less okay with
changing bufio.Reader to []byte.  Among other things
there's no API to do that, and I'm not sure how it would
play out in the uses I have.

If you're doing sizable writes bufio bypasses the buffer
anyway.  It's not a big deal and this is a rare use case.
I'd rather just leave the bufio there.

Russ

[gburd, 2011-03-07 19:16:55]

On 2011/03/07 19:08:18, rsc wrote:
>  Among other things  there's no API to do that

    buf, err = br.Peek(br.Buffered())
 
> If you're doing sizable writes bufio bypasses the buffer
> anyway.  It's not a big deal and this is a rare use case.
> I'd rather just leave the bufio there.

I have the reverse problem. Because my application sends small messages, I want
to reduce the memory used per connection by dropping down to smaller buffers.

> this is a rare use case I'd rather just leave the bufio there.

Good point. It will probably be more convenient for most apps to get the bufio.
I can unwind the bufio in my app.

[adg, 2011-03-07 21:59:10]

On 8 March 2011 06:02,  <gary.burd@gmail.com> wrote:
> This approach allows the caller to make it's own decisions about how the
> connection is buffered. It also makes it clear that the caller is
> getting a network connection.

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?)

Andrew

[rsc, 2011-03-07 22:01:56]

> 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.

Russ

[adg, 2011-03-07 22:33:01]

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.

Andrew

[bradfitzgoog, 2011-03-07 23:23:13]

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.