code review 4082044: Added pipelining to http.ServerConn and http.ClientConn,

Added pipelining to http.ServerConn and http.ClientConn,
using textproto.

[petar-m, 2011-01-21 00:17:12]

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

I'd like you to review this change.

[rsc, 2011-01-21 20:05:40]

It seems like the ClientConn should be able to be told
not to pipeline a request.  Wikipedia claims that 
non-idempotent requests like POST should not be pipelined,
for example, but in general I think the calling code would
know best.  Don't need to do anything in this CL but maybe
something to think about.  I'm not sure what the API would
look like to enable that control.

http://codereview.appspot.com/4082044/diff/2001/src/pkg/http/persist.go
File src/pkg/http/persist.go (right):

http://codereview.appspot.com/4082044/diff/2001/src/pkg/http/persist.go#newco...
src/pkg/http/persist.go:68: // Ensure ordered execution of Read's and Write's
Reads and Writes (no ')

http://codereview.appspot.com/4082044/diff/2001/src/pkg/http/persist.go#newco...
src/pkg/http/persist.go:161: defer func() {
defer sc.pipe.EndResponse(id)

http://codereview.appspot.com/4082044/diff/2001/src/pkg/http/persist.go#newco...
src/pkg/http/persist.go:241: // Ensure ordered execution of Write's
s/'//

http://codereview.appspot.com/4082044/diff/2001/src/pkg/http/persist.go#newco...
src/pkg/http/persist.go:314: defer func() {
defer cc.pipe.EndResponse(id)

http://codereview.appspot.com/4082044/diff/2001/src/pkg/http/persist.go#newco...
src/pkg/http/persist.go:360: // Fetch is convenience method that writes a
request and reads a response.
Fetch doesn't convey the right meaning to me.
It sounds like you're fetching the request.

Do?
Transact?

http://codereview.appspot.com/4082044/diff/2001/src/pkg/http/persist.go#newco...
src/pkg/http/persist.go:366: resp, err = cc.Read(req)
return cc.Read(req) works too

[bradfitzgoog, 2011-01-22 08:24:15]

Pipelining should only be done on GET and HEAD requests, and even then big
parts of the HTTP world are broken with them.  (notably proxies, especially
the over-so-helpful "transparent" variety)  Most browsers have chosen to
disable them by default, sadly.  If Go enabled them by default I think we'd
surely hit brokenness.  We should probably also go opt-in (on the client
side).  On the server side we should support them by default correctly.

On Fri, Jan 21, 2011 at 12:05 PM, <rsc@golang.org> wrote:

> It seems like the ClientConn should be able to be told
> not to pipeline a request.  Wikipedia claims that
> non-idempotent requests like POST should not be pipelined,
> for example, but in general I think the calling code would
> know best.  Don't need to do anything in this CL but maybe
> something to think about.  I'm not sure what the API would
> look like to enable that control.
>
>
>
> http://codereview.appspot.com/4082044/diff/2001/src/pkg/http/persist.go
> File src/pkg/http/persist.go (right):
>
>
>
http://codereview.appspot.com/4082044/diff/2001/src/pkg/http/persist.go#newco...
> src/pkg/http/persist.go:68: // Ensure ordered execution of Read's and
> Write's
> Reads and Writes (no ')
>
>
>
http://codereview.appspot.com/4082044/diff/2001/src/pkg/http/persist.go#newco...
> src/pkg/http/persist.go:161: defer func() {
> defer sc.pipe.EndResponse(id)
>
>
>
http://codereview.appspot.com/4082044/diff/2001/src/pkg/http/persist.go#newco...
> src/pkg/http/persist.go:241: // Ensure ordered execution of Write's
> s/'//
>
>
>
http://codereview.appspot.com/4082044/diff/2001/src/pkg/http/persist.go#newco...
> src/pkg/http/persist.go:314: defer func() {
> defer cc.pipe.EndResponse(id)
>
>
>
http://codereview.appspot.com/4082044/diff/2001/src/pkg/http/persist.go#newco...
> src/pkg/http/persist.go:360: // Fetch is convenience method that writes
> a request and reads a response.
> Fetch doesn't convey the right meaning to me.
> It sounds like you're fetching the request.
>
> Do?
> Transact?
>
>
>
http://codereview.appspot.com/4082044/diff/2001/src/pkg/http/persist.go#newco...
> src/pkg/http/persist.go:366: resp, err = cc.Read(req)
> return cc.Read(req) works too
>
>
> http://codereview.appspot.com/4082044/
>

[petar-m, 2011-01-22 15:44:42]

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

Please take another look.

[petar-m, 2011-01-22 15:47:48]

About the pipelining concerns: Since Server/ClientConn are low-level and not
currently used by the user-facing functions of the http package, (I think) it
should be fine to have them allow pipelining for non-GET requests.

http://codereview.appspot.com/4082044/diff/2001/src/pkg/http/persist.go
File src/pkg/http/persist.go (right):

http://codereview.appspot.com/4082044/diff/2001/src/pkg/http/persist.go#newco...
src/pkg/http/persist.go:68: // Ensure ordered execution of Read's and Write's
On 2011/01/21 20:05:40, rsc wrote:
> Reads and Writes (no ')

Done.

http://codereview.appspot.com/4082044/diff/2001/src/pkg/http/persist.go#newco...
src/pkg/http/persist.go:161: defer func() {
On 2011/01/21 20:05:40, rsc wrote:
> defer sc.pipe.EndResponse(id)

Done.

http://codereview.appspot.com/4082044/diff/2001/src/pkg/http/persist.go#newco...
src/pkg/http/persist.go:241: // Ensure ordered execution of Write's
On 2011/01/21 20:05:40, rsc wrote:
> s/'//

Done.

http://codereview.appspot.com/4082044/diff/2001/src/pkg/http/persist.go#newco...
src/pkg/http/persist.go:314: defer func() {
On 2011/01/21 20:05:40, rsc wrote:
> defer cc.pipe.EndResponse(id)

Done.

http://codereview.appspot.com/4082044/diff/2001/src/pkg/http/persist.go#newco...
src/pkg/http/persist.go:360: // Fetch is convenience method that writes a
request and reads a response.
I called it Do. Another option would be WriteRead().

On 2011/01/21 20:05:40, rsc wrote:
> Fetch doesn't convey the right meaning to me.
> It sounds like you're fetching the request.
> 
> Do?
> Transact?

http://codereview.appspot.com/4082044/diff/2001/src/pkg/http/persist.go#newco...
src/pkg/http/persist.go:366: resp, err = cc.Read(req)
On 2011/01/21 20:05:40, rsc wrote:
> return cc.Read(req) works too

Done.

[rsc, 2011-02-11 20:03:32]

LGTM

On Sat, Jan 22, 2011 at 10:47,  <petarm@gmail.com> wrote:
> About the pipelining concerns: Since Server/ClientConn are low-level and
> not currently used by the user-facing functions of the http package, (I
> think) it should be fine to have them allow pipelining for non-GET
> requests.

I agree.  What's missing from this CL is a way for a
user of ClientConn to say "run this next request as
the only thing in the pipeline", so that users who do
want to de-pipeline POSTs can.  I don't know what that
interface would look like, though.

Russ

[rsc, 2011-02-11 20:05:51]

*** Submitted as f52b035a8e1e ***

http: add pipelining to ClientConn, ServerConn

R=rsc, bradfitzwork
CC=golang-dev
http://codereview.appspot.com/4082044

Committer: Russ Cox <rsc@golang.org>