Issue 12632043: code review 12632043: net/http: fix early side effects in the ResponseWriter'... (Closed)
Descriptionnet/http: fix early side effects in the ResponseWriter's ReadFrom
The ResponseWriter's ReadFrom method was causing side effects on
the output before any data was read.
Now, bail out early and do a normal copy (which does a read
before writing) when our input and output are known to not to
be the pair of types we need for sendfile.
Fixes issue 5660
Patch Set 1 #Patch Set 2 : diff -r c71f9b21cc13 https://go.googlecode.com/hg/ #Patch Set 3 : diff -r c71f9b21cc13 https://go.googlecode.com/hg/ #Patch Set 4 : diff -r 08caa64606f9 https://go.googlecode.com/hg/ #Patch Set 5 : diff -r 08caa64606f9 https://go.googlecode.com/hg/ #
Total comments: 2
Patch Set 6 : diff -r c6b37ee14a9f https://go.googlecode.com/hg/ #MessagesTotal messages: 18
Hello golang-dev@googlegroups.com, I'd like you to review this change to https://go.googlecode.com/hg/
Sign in to reply to this message.
Hmm. To work around a bad interaction between io.Copy and an http.ResponseWriter, we're making a change in os/exec? That doesn't seem right. Russ
Sign in to reply to this message.
Suggestions welcome. On Wed, Aug 7, 2013 at 4:25 PM, Russ Cox <rsc@golang.org> wrote: > Hmm. > > To work around a bad interaction between io.Copy and an > http.ResponseWriter, we're making a change in os/exec? That doesn't seem > right. > > Russ > >
Sign in to reply to this message.
It is possible that, now that we understand what is going on, issue 5660 is working as intended. Passing the ResponseWriter to os/exec counts as a Write, and you can't do that before you consume the body. It is also possible that ResponseWriter's ReadFrom should copy the first block itself, without sendfile; basically, delete the w.wroteHeader check and change if w.needsSniff to if true. That will delay the first write - and therefore the header flush etc - until there is data available. Russ
Sign in to reply to this message.
On Wed, Aug 7, 2013 at 4:28 PM, Russ Cox <rsc@golang.org> wrote: > It is possible that, now that we understand what is going on, issue 5660 > is working as intended. Passing the ResponseWriter to os/exec counts as a > Write, and you can't do that before you consume the body. > > It is also possible that ResponseWriter's ReadFrom should copy the first > block itself, without sendfile; > I considered that but wasn't sure that was worth the price. In both the sendfile case and the os/exec case, the src io.Reader is of type *os.File. So I can't do it conditional on the type. What I *could* do is make it conditional on (*os.File).Name(). Which for pipes is "|0" and "|1". How gross is that? Can I depend on Name starting with "|" to go into the slow path? We couldn't really change that safely at this point now anyway, right? > basically, delete the w.wroteHeader check and change if w.needsSniff to if > true. That will delay the first write - and therefore the header flush etc > - until there is data available. > > Russ >
Sign in to reply to this message.
On Wed, Aug 7, 2013 at 7:32 PM, Brad Fitzpatrick <bradfitz@golang.org>wrote: > >> It is also possible that ResponseWriter's ReadFrom should copy the first >> block itself, without sendfile; >> > > I considered that but wasn't sure that was worth the price. > The more I think about this the more I think it's the correct fix, regardless of price. ReadFrom should not be causing the effects of a Write until there is something to write. The implementation here is buggy. Russ
Sign in to reply to this message.
On Wed, Aug 7, 2013 at 4:56 PM, Russ Cox <rsc@golang.org> wrote: > On Wed, Aug 7, 2013 at 7:32 PM, Brad Fitzpatrick <bradfitz@golang.org>wrote: > >> >>> It is also possible that ResponseWriter's ReadFrom should copy the first >>> block itself, without sendfile; >>> >> >> I considered that but wasn't sure that was worth the price. >> > > The more I think about this the more I think it's the correct fix, > regardless of price. ReadFrom should not be causing the effects of a Write > until there is something to write. The implementation here is buggy. > Sure, will do. I have a nice test for this written from earlier today. Can I keep a fast path avoiding that Read if the src is an *os.File and its Name doesn't begin with a Pipe? That is the common case (http.FileServer) for ReadFrom.
Sign in to reply to this message.
On Wed, Aug 7, 2013 at 8:01 PM, Brad Fitzpatrick <bradfitz@golang.org>wrote: > Can I keep a fast path avoiding that Read if the src is an *os.File and > its Name doesn't begin with a Pipe? That is the common case > (http.FileServer) for ReadFrom. > How about calling f.Stat and checking f.Mode().IsRegular()?
Sign in to reply to this message.
On Wed, Aug 7, 2013 at 5:02 PM, Russ Cox <rsc@golang.org> wrote: > On Wed, Aug 7, 2013 at 8:01 PM, Brad Fitzpatrick <bradfitz@golang.org>wrote: > >> Can I keep a fast path avoiding that Read if the src is an *os.File and >> its Name doesn't begin with a Pipe? That is the common case >> (http.FileServer) for ReadFrom. >> > > How about calling f.Stat and checking f.Mode().IsRegular()? > Sure, that works. It's unfortunate because it means we go from 2 stats on the same file to 3 stats on the same file. (Peter Buhr was noticing this on his ktrace analysis of the file server recently) Oh well, the kernel will have it cached.
Sign in to reply to this message.
Hello golang-dev@googlegroups.com, rsc@golang.org (cc: golang-dev@googlegroups.com), Please take another look.
Sign in to reply to this message.
PTAL Now without any os/exec changes. On Wed, Aug 7, 2013 at 7:45 PM, <bradfitz@golang.org> wrote: > Hello golang-dev@googlegroups.com, rsc@golang.org (cc: > golang-dev@googlegroups.com), > > Please take another look. > > > https://codereview.appspot.**com/12632043/<https://codereview.appspot.com/126... >
Sign in to reply to this message.
On Thursday, August 8, 2013 2:05:49 AM UTC+2, Brad Fitzpatrick wrote: > On Wed, Aug 7, 2013 at 5:02 PM, Russ Cox <r...@golang.org <javascript:>>wrote: > >> On Wed, Aug 7, 2013 at 8:01 PM, Brad Fitzpatrick <brad...@golang.org<javascript:> >> > wrote: >> >>> Can I keep a fast path avoiding that Read if the src is an *os.File and >>> its Name doesn't begin with a Pipe? That is the common case >>> (http.FileServer) for ReadFrom. >>> >> >> How about calling f.Stat and checking f.Mode().IsRegular()? >> > > Sure, that works. It's unfortunate because it means we go from 2 stats on > the same file to 3 stats on the same file. (Peter Buhr was noticing this > on his ktrace analysis of the file server recently) > > Oh well, the kernel will have it cached. > Can't we cache at least the file type of opened files? I guess they won't change after open, as the driver is then usually bound to the handle in all the operating systems I am aware of.
Sign in to reply to this message.
On Wed, Aug 7, 2013 at 8:05 PM, Brad Fitzpatrick <bradfitz@golang.org>wrote: > Sure, that works. It's unfortunate because it means we go from 2 stats on > the same file to 3 stats on the same file. (Peter Buhr was noticing this > on his ktrace analysis of the file server recently) > Would you rather not use sendfile at all? Surely the benefit of using sendfile more than pays for the cost of three stat calls. Russ
Sign in to reply to this message.
I'm happy statting. On Aug 8, 2013 9:08 AM, "Russ Cox" <rsc@golang.org> wrote: > On Wed, Aug 7, 2013 at 8:05 PM, Brad Fitzpatrick <bradfitz@golang.org>wrote: > >> Sure, that works. It's unfortunate because it means we go from 2 stats >> on the same file to 3 stats on the same file. (Peter Buhr was noticing >> this on his ktrace analysis of the file server recently) >> > > Would you rather not use sendfile at all? > > Surely the benefit of using sendfile more than pays for the cost of three > stat calls. > > Russ >
Sign in to reply to this message.
Russ, This cool? On Wed, Aug 7, 2013 at 7:46 PM, Brad Fitzpatrick <bradfitz@golang.org>wrote: > PTAL > > Now without any os/exec changes. > > > > On Wed, Aug 7, 2013 at 7:45 PM, <bradfitz@golang.org> wrote: > >> Hello golang-dev@googlegroups.com, rsc@golang.org (cc: >> golang-dev@googlegroups.com), >> >> Please take another look. >> >> >> https://codereview.appspot.**com/12632043/<https://codereview.appspot.com/126... >> > >
Sign in to reply to this message.
LGTM https://codereview.appspot.com/12632043/diff/19001/src/pkg/net/http/server.go File src/pkg/net/http/server.go (right): https://codereview.appspot.com/12632043/diff/19001/src/pkg/net/http/server.go... src/pkg/net/http/server.go:381: if !ok || !regFile { you could possibly do || !w.bodyAllowed() here too, instead of at the bottom, if bodyAllowed is not something that is going to change before you get there. either way.
Sign in to reply to this message.
https://codereview.appspot.com/12632043/diff/19001/src/pkg/net/http/server.go File src/pkg/net/http/server.go (right): https://codereview.appspot.com/12632043/diff/19001/src/pkg/net/http/server.go... src/pkg/net/http/server.go:381: if !ok || !regFile { On 2013/08/08 20:54:40, rsc wrote: > you could possibly do || !w.bodyAllowed() here too, instead of at the bottom, > if bodyAllowed is not something that is going to change before you get there. > either way. That's illegal to call before the headers have been flushed.
Sign in to reply to this message.
*** Submitted as https://code.google.com/p/go/source/detail?r=f53cee5f15de *** net/http: fix early side effects in the ResponseWriter's ReadFrom The ResponseWriter's ReadFrom method was causing side effects on the output before any data was read. Now, bail out early and do a normal copy (which does a read before writing) when our input and output are known to not to be the pair of types we need for sendfile. Fixes issue 5660 R=golang-dev, rsc, nightlyone CC=golang-dev https://codereview.appspot.com/12632043
Sign in to reply to this message.
|
