Issue 56060045: code review 56060045: net/http/cgi: Fix leaking CGI processes
|
Created:
11 years, 11 months ago by smh Modified:
11 years, 10 months ago CC:
golang-codereviews Visibility:
Public. |
Descriptionnet/http/cgi: Fix leaking CGI processes
Addresses leaking CGI processes and stalling a handler forever when
an output error occurs, such as a client disconnecting while a
response is still in progress.
Fixes issue 7196.
Patch Set 1 #Patch Set 2 : diff -r 0ba5ecac8038 https://code.google.com/p/go #Patch Set 3 : diff -r 0ba5ecac8038 https://code.google.com/p/go #
Total comments: 2
MessagesTotal messages: 15
Hello golang-dev@googlegroups.com (cc: golang-codereviews@googlegroups.com), I'd like you to review this change to https://code.google.com/p/go
Sign in to reply to this message.
Replacing golang-dev with golang-codereviews. To the author of this CL: If you are using 'hg mail -r golang-dev' to mail the CL, use simply 'hg mail' instead. If you did not name golang-dev explicitly and it was still added to the CL, it means your working copy of the repo has a stale codereview.cfg (or lib/codereview/codereview.cfg). Please run 'hg sync' to update your client to the most recent codereview.cfg. If the most recent codereview.cfg has defaultcc set to golang-dev instead of golang-codereviews, please send a CL correcting it. Thanks very much.
Sign in to reply to this message.
https://codereview.appspot.com/56060045/diff/40001/src/pkg/net/http/cgi/host.go File src/pkg/net/http/cgi/host.go (right): https://codereview.appspot.com/56060045/diff/40001/src/pkg/net/http/cgi/host.... src/pkg/net/http/cgi/host.go:298: // To ensure we don't stall forever in this case we force It might be more friendly to close our reading side of the pipe immediately to hopefully cause the writer side to fail and exit gracefully on its own, and only kill it in a second if it doesn't go away (relatively gracefully) on its own. This one second wait+kill could be in its own goroutine, so the HTTP handler finishes quickly.
Sign in to reply to this message.
Also, can you include a test? On Thu, Jan 23, 2014 at 3:28 PM, <steven.hartland@multiplay.co.uk> wrote: > Reviewers: golang-dev, > > Message: > Hello golang-dev@googlegroups.com (cc: > golang-codereviews@googlegroups.com), > > I'd like you to review this change to > https://code.google.com/p/go > > > Description: > net/http/cgi: Fix leaking CGI processes > > Addresses leaking CGI processes and stalling a handler forever when > an output error occurs, such as a client disconnecting while a > response is still in progress. > > Fixes issue 7196. > > Please review this at https://codereview.appspot.com/56060045/ > > Affected files (+8, -1 lines): > M src/pkg/net/http/cgi/host.go > > > Index: src/pkg/net/http/cgi/host.go > =================================================================== > --- a/src/pkg/net/http/cgi/host.go > +++ b/src/pkg/net/http/cgi/host.go > @@ -291,7 +291,14 @@ > > _, err = io.Copy(rw, linebody) > if err != nil { > - h.printf("cgi: copy error: %v", err) > + // We've recieved an error copying data from the process > + // to our output but this may be due to an output error not > + // an input error. > + // > + // To ensure we don't stall forever in this case we force > + // kill the process. > + cmd.Process.Kill() > + h.printf("cgi: copy error: %v (killed pid: %v)", err, > cmd.Process.Pid) > } > } > > > > -- > You received this message because you are subscribed to the Google Groups > "golang-codereviews" group. > To unsubscribe from this group and stop receiving emails from it, send an > email to golang-codereviews+unsubscribe@googlegroups.com. > For more options, visit https://groups.google.com/groups/opt_out. >
Sign in to reply to this message.
First time using codereview and didn't realise it didn't publish comments when you saved them, so sorry for the slow reply its been sitting there for days ;-) https://codereview.appspot.com/56060045/diff/40001/src/pkg/net/http/cgi/host.go File src/pkg/net/http/cgi/host.go (right): https://codereview.appspot.com/56060045/diff/40001/src/pkg/net/http/cgi/host.... src/pkg/net/http/cgi/host.go:298: // To ensure we don't stall forever in this case we force It's something that I considered but has some drawbacks mainly not all cgi processes are going to be friendly in this manor so I could imagine the situation where this could be used as a dos vector on the server.
Sign in to reply to this message.
Still no test, though?
Sign in to reply to this message.
ping? if this is going to go in, we need this review to wrap up in the next few days. with a test.
Sign in to reply to this message.
Steven, you also haven't submitted a CLA: See http://golang.org/doc/contribute.html#copyright On Wed, Mar 5, 2014 at 12:02 PM, <rsc@golang.org> wrote: > ping? > > if this is going to go in, we need this review to wrap up in the next > few days. with a test. > > > https://codereview.appspot.com/56060045/ >
Sign in to reply to this message.
Sorry guys been off ill for a while, signed this morning for you.
Regards
Steve
----- Original Message -----
From: "Brad Fitzpatrick" <bradfitz@golang.org>
To: <steven.hartland@multiplay.co.uk>; <golang-codereviews@googlegroups.com>;
"Brad Fitzpatrick" <bradfitz@golang.org>; "Russ Cox"
<rsc@golang.org>; <reply@codereview-hr.appspotmail.com>
Sent: Wednesday, March 05, 2014 9:56 PM
Subject: Re: code review 56060045: net/http/cgi: Fix leaking CGI processes
(issue 56060045)
> Steven, you also haven't submitted a CLA:
>
> See http://golang.org/doc/contribute.html#copyright
>
>
> On Wed, Mar 5, 2014 at 12:02 PM, <rsc@golang.org> wrote:
>
>> ping?
>>
>> if this is going to go in, we need this review to wrap up in the next
>> few days. with a test.
>>
>>
>> https://codereview.appspot.com/56060045/
>>
>
================================================
This e.mail is private and confidential between Multiplay (UK) Ltd. and the
person or entity to whom it is addressed. In the event of misdirection, the
recipient is prohibited from using, copying, printing or otherwise disseminating
it or any information contained in it.
In the event of misdirection, illegible or incomplete transmission please
telephone +44 845 868 1337
or return the E.mail to postmaster@multiplay.co.uk.
Sign in to reply to this message.
I see no record of you submitting a CLA in the past few days or hours. If you postal mailed or faxed in a corporate CLA, it might take a few days for a human to process it. On Mon, Mar 10, 2014 at 3:13 AM, Steven Hartland < steven.hartland@multiplay.co.uk> wrote: > Sorry guys been off ill for a while, signed this morning for you. > > Regards > Steve > ----- Original Message ----- From: "Brad Fitzpatrick" <bradfitz@golang.org > > > To: <steven.hartland@multiplay.co.uk>; <golang-codereviews@ > googlegroups.com>; "Brad Fitzpatrick" <bradfitz@golang.org>; "Russ Cox" < > rsc@golang.org>; <reply@codereview-hr.appspotmail.com> > Sent: Wednesday, March 05, 2014 9:56 PM > Subject: Re: code review 56060045: net/http/cgi: Fix leaking CGI processes > (issue 56060045) > > > > Steven, you also haven't submitted a CLA: >> >> See http://golang.org/doc/contribute.html#copyright >> >> >> On Wed, Mar 5, 2014 at 12:02 PM, <rsc@golang.org> wrote: >> >> ping? >>> >>> if this is going to go in, we need this review to wrap up in the next >>> few days. with a test. >>> >>> >>> https://codereview.appspot.com/56060045/ >>> >>> >> > > ================================================ > This e.mail is private and confidential between Multiplay (UK) Ltd. and > the person or entity to whom it is addressed. In the event of misdirection, > the recipient is prohibited from using, copying, printing or otherwise > disseminating it or any information contained in it. > In the event of misdirection, illegible or incomplete transmission please > telephone +44 845 868 1337 > or return the E.mail to postmaster@multiplay.co.uk. > >
Sign in to reply to this message.
Hi Brad, I used the electronic form with the email steven.hartland@multiplay.co.uk If it got confused with my various google accounts you may find it under steven@multiplay.co.uk Regards Steve ----- Original Message ----- From: "Brad Fitzpatrick" <bradfitz@golang.org> To: "Steven Hartland" <steven.hartland@multiplay.co.uk> Cc: <golang-codereviews@googlegroups.com>; "Russ Cox" <rsc@golang.org>; <reply@codereview-hr.appspotmail.com> Sent: Monday, March 10, 2014 2:55 PM Subject: Re: code review 56060045: net/http/cgi: Fix leaking CGI processes (issue 56060045) >I see no record of you submitting a CLA in the past few days or hours. > > If you postal mailed or faxed in a corporate CLA, it might take a few days > for a human to process it. > > > > On Mon, Mar 10, 2014 at 3:13 AM, Steven Hartland < > steven.hartland@multiplay.co.uk> wrote: > >> Sorry guys been off ill for a while, signed this morning for you. >> >> Regards >> Steve >> ----- Original Message ----- From: "Brad Fitzpatrick" <bradfitz@golang.org >> > >> To: <steven.hartland@multiplay.co.uk>; <golang-codereviews@ >> googlegroups.com>; "Brad Fitzpatrick" <bradfitz@golang.org>; "Russ Cox" < >> rsc@golang.org>; <reply@codereview-hr.appspotmail.com> >> Sent: Wednesday, March 05, 2014 9:56 PM >> Subject: Re: code review 56060045: net/http/cgi: Fix leaking CGI processes >> (issue 56060045) >> >> >> >> Steven, you also haven't submitted a CLA: >>> >>> See http://golang.org/doc/contribute.html#copyright >>> >>> >>> On Wed, Mar 5, 2014 at 12:02 PM, <rsc@golang.org> wrote: >>> >>> ping? >>>> >>>> if this is going to go in, we need this review to wrap up in the next >>>> few days. with a test. >>>> >>>> >>>> https://codereview.appspot.com/56060045/ >>>> >>>> >>> >> >> ================================================ >> This e.mail is private and confidential between Multiplay (UK) Ltd. and >> the person or entity to whom it is addressed. In the event of misdirection, >> the recipient is prohibited from using, copying, printing or otherwise >> disseminating it or any information contained in it. >> In the event of misdirection, illegible or incomplete transmission please >> telephone +44 845 868 1337 >> or return the E.mail to postmaster@multiplay.co.uk. >> >> > ================================================ This e.mail is private and confidential between Multiplay (UK) Ltd. and the person or entity to whom it is addressed. In the event of misdirection, the recipient is prohibited from using, copying, printing or otherwise disseminating it or any information contained in it. In the event of misdirection, illegible or incomplete transmission please telephone +44 845 868 1337 or return the E.mail to postmaster@multiplay.co.uk.
Sign in to reply to this message.
Still nothing. I think you're confused and thinks it infers your email address from your Google account. It does not. It's a form and you have to type your name, number, address, and the words I AGREE in all caps, then hit submit. I see one blank record submission 4 hours ago. Perhaps that was you. On Mon, Mar 10, 2014 at 7:59 AM, Steven Hartland < steven.hartland@multiplay.co.uk> wrote: > Hi Brad, I used the electronic form with the email > steven.hartland@multiplay.co.uk > > If it got confused with my various google accounts you may find it under > steven@multiplay.co.uk > > > Regards > Steve > ----- Original Message ----- From: "Brad Fitzpatrick" < > bradfitz@golang.org> > To: "Steven Hartland" <steven.hartland@multiplay.co.uk> > Cc: <golang-codereviews@googlegroups.com>; "Russ Cox" <rsc@golang.org>; < > reply@codereview-hr.appspotmail.com> > Sent: Monday, March 10, 2014 2:55 PM > > Subject: Re: code review 56060045: net/http/cgi: Fix leaking CGI processes > (issue 56060045) > > > I see no record of you submitting a CLA in the past few days or hours. >> >> If you postal mailed or faxed in a corporate CLA, it might take a few days >> for a human to process it. >> >> >> >> On Mon, Mar 10, 2014 at 3:13 AM, Steven Hartland < >> steven.hartland@multiplay.co.uk> wrote: >> >> Sorry guys been off ill for a while, signed this morning for you. >>> >>> Regards >>> Steve >>> ----- Original Message ----- From: "Brad Fitzpatrick" < >>> bradfitz@golang.org >>> > >>> To: <steven.hartland@multiplay.co.uk>; <golang-codereviews@ >>> googlegroups.com>; "Brad Fitzpatrick" <bradfitz@golang.org>; "Russ Cox" >>> < >>> rsc@golang.org>; <reply@codereview-hr.appspotmail.com> >>> Sent: Wednesday, March 05, 2014 9:56 PM >>> Subject: Re: code review 56060045: net/http/cgi: Fix leaking CGI >>> processes >>> (issue 56060045) >>> >>> >>> >>> Steven, you also haven't submitted a CLA: >>> >>>> >>>> See http://golang.org/doc/contribute.html#copyright >>>> >>>> >>>> On Wed, Mar 5, 2014 at 12:02 PM, <rsc@golang.org> wrote: >>>> >>>> ping? >>>> >>>>> >>>>> if this is going to go in, we need this review to wrap up in the next >>>>> few days. with a test. >>>>> >>>>> >>>>> https://codereview.appspot.com/56060045/ >>>>> >>>>> >>>>> >>>> >>> ================================================ >>> This e.mail is private and confidential between Multiplay (UK) Ltd. and >>> the person or entity to whom it is addressed. In the event of >>> misdirection, >>> the recipient is prohibited from using, copying, printing or otherwise >>> disseminating it or any information contained in it. >>> In the event of misdirection, illegible or incomplete transmission please >>> telephone +44 845 868 1337 >>> or return the E.mail to postmaster@multiplay.co.uk. >>> >>> >>> >> > ================================================ > This e.mail is private and confidential between Multiplay (UK) Ltd. and > the person or entity to whom it is addressed. In the event of misdirection, > the recipient is prohibited from using, copying, printing or otherwise > disseminating it or any information contained in it. > In the event of misdirection, illegible or incomplete transmission please > telephone +44 845 868 1337 > or return the E.mail to postmaster@multiplay.co.uk. > >
Sign in to reply to this message.
I'm guessing the account requirement is triggered as its a google doc.
I've filled it out again and got the same response as last time but without
the prompt to login to google docs, as I was already logged in this time
"code.google.com signatories
Thank you. Your CLA submission will be processed shortly."
See if you get that this time?
Regards
Steve
----- Original Message -----
From: "Brad Fitzpatrick" <bradfitz@golang.org>
To: "Steven Hartland" <steven.hartland@multiplay.co.uk>
Cc: <golang-codereviews@googlegroups.com>; "Russ Cox" <rsc@golang.org>;
<reply@codereview-hr.appspotmail.com>
Sent: Monday, March 10, 2014 3:04 PM
Subject: Re: code review 56060045: net/http/cgi: Fix leaking CGI processes
(issue 56060045)
> Still nothing.
>
> I think you're confused and thinks it infers your email address from your
> Google account. It does not.
>
> It's a form and you have to type your name, number, address, and the words
> I AGREE in all caps, then hit submit.
>
> I see one blank record submission 4 hours ago. Perhaps that was you.
>
>
> On Mon, Mar 10, 2014 at 7:59 AM, Steven Hartland <
> steven.hartland@multiplay.co.uk> wrote:
>
>> Hi Brad, I used the electronic form with the email
>> steven.hartland@multiplay.co.uk
>>
>> If it got confused with my various google accounts you may find it under
>> steven@multiplay.co.uk
>>
>>
>> Regards
>> Steve
>> ----- Original Message ----- From: "Brad Fitzpatrick" <
>> bradfitz@golang.org>
>> To: "Steven Hartland" <steven.hartland@multiplay.co.uk>
>> Cc: <golang-codereviews@googlegroups.com>; "Russ Cox" <rsc@golang.org>; <
>> reply@codereview-hr.appspotmail.com>
>> Sent: Monday, March 10, 2014 2:55 PM
>>
>> Subject: Re: code review 56060045: net/http/cgi: Fix leaking CGI processes
>> (issue 56060045)
>>
>>
>> I see no record of you submitting a CLA in the past few days or hours.
>>>
>>> If you postal mailed or faxed in a corporate CLA, it might take a few days
>>> for a human to process it.
>>>
>>>
>>>
>>> On Mon, Mar 10, 2014 at 3:13 AM, Steven Hartland <
>>> steven.hartland@multiplay.co.uk> wrote:
>>>
>>> Sorry guys been off ill for a while, signed this morning for you.
>>>>
>>>> Regards
>>>> Steve
>>>> ----- Original Message ----- From: "Brad Fitzpatrick" <
>>>> bradfitz@golang.org
>>>> >
>>>> To: <steven.hartland@multiplay.co.uk>; <golang-codereviews@
>>>> googlegroups.com>; "Brad Fitzpatrick" <bradfitz@golang.org>; "Russ Cox"
>>>> <
>>>> rsc@golang.org>; <reply@codereview-hr.appspotmail.com>
>>>> Sent: Wednesday, March 05, 2014 9:56 PM
>>>> Subject: Re: code review 56060045: net/http/cgi: Fix leaking CGI
>>>> processes
>>>> (issue 56060045)
>>>>
>>>>
>>>>
>>>> Steven, you also haven't submitted a CLA:
>>>>
>>>>>
>>>>> See http://golang.org/doc/contribute.html#copyright
>>>>>
>>>>>
>>>>> On Wed, Mar 5, 2014 at 12:02 PM, <rsc@golang.org> wrote:
>>>>>
>>>>> ping?
>>>>>
>>>>>>
>>>>>> if this is going to go in, we need this review to wrap up in the next
>>>>>> few days. with a test.
>>>>>>
>>>>>>
>>>>>> https://codereview.appspot.com/56060045/
>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>> ================================================
>>>> This e.mail is private and confidential between Multiplay (UK) Ltd. and
>>>> the person or entity to whom it is addressed. In the event of
>>>> misdirection,
>>>> the recipient is prohibited from using, copying, printing or otherwise
>>>> disseminating it or any information contained in it.
>>>> In the event of misdirection, illegible or incomplete transmission please
>>>> telephone +44 845 868 1337
>>>> or return the E.mail to postmaster@multiplay.co.uk.
>>>>
>>>>
>>>>
>>>
>> ================================================
>> This e.mail is private and confidential between Multiplay (UK) Ltd. and
>> the person or entity to whom it is addressed. In the event of misdirection,
>> the recipient is prohibited from using, copying, printing or otherwise
>> disseminating it or any information contained in it.
>> In the event of misdirection, illegible or incomplete transmission please
>> telephone +44 845 868 1337
>> or return the E.mail to postmaster@multiplay.co.uk.
>>
>>
>
================================================
This e.mail is private and confidential between Multiplay (UK) Ltd. and the
person or entity to whom it is addressed. In the event of misdirection, the
recipient is prohibited from using, copying, printing or otherwise disseminating
it or any information contained in it.
In the event of misdirection, illegible or incomplete transmission please
telephone +44 845 868 1337
or return the E.mail to postmaster@multiplay.co.uk.
Sign in to reply to this message.
Got it, thanks. On Mon, Mar 10, 2014 at 8:17 AM, Steven Hartland < steven.hartland@multiplay.co.uk> wrote: > I'm guessing the account requirement is triggered as its a google doc. > > I've filled it out again and got the same response as last time but without > the prompt to login to google docs, as I was already logged in this time > > "code.google.com signatories > Thank you. Your CLA submission will be processed shortly." > > See if you get that this time? > > > Regards > Steve > > ----- Original Message ----- From: "Brad Fitzpatrick" <bradfitz@golang.org > > > To: "Steven Hartland" <steven.hartland@multiplay.co.uk> > Cc: <golang-codereviews@googlegroups.com>; "Russ Cox" <rsc@golang.org>; < > reply@codereview-hr.appspotmail.com> > Sent: Monday, March 10, 2014 3:04 PM > > Subject: Re: code review 56060045: net/http/cgi: Fix leaking CGI processes > (issue 56060045) > > > Still nothing. >> >> I think you're confused and thinks it infers your email address from your >> Google account. It does not. >> >> It's a form and you have to type your name, number, address, and the words >> I AGREE in all caps, then hit submit. >> >> I see one blank record submission 4 hours ago. Perhaps that was you. >> >> >> On Mon, Mar 10, 2014 at 7:59 AM, Steven Hartland < >> steven.hartland@multiplay.co.uk> wrote: >> >> Hi Brad, I used the electronic form with the email >>> steven.hartland@multiplay.co.uk >>> >>> If it got confused with my various google accounts you may find it under >>> steven@multiplay.co.uk >>> >>> >>> Regards >>> Steve >>> ----- Original Message ----- From: "Brad Fitzpatrick" < >>> bradfitz@golang.org> >>> To: "Steven Hartland" <steven.hartland@multiplay.co.uk> >>> Cc: <golang-codereviews@googlegroups.com>; "Russ Cox" <rsc@golang.org>; >>> < >>> reply@codereview-hr.appspotmail.com> >>> Sent: Monday, March 10, 2014 2:55 PM >>> >>> Subject: Re: code review 56060045: net/http/cgi: Fix leaking CGI >>> processes >>> (issue 56060045) >>> >>> >>> I see no record of you submitting a CLA in the past few days or hours. >>> >>>> >>>> If you postal mailed or faxed in a corporate CLA, it might take a few >>>> days >>>> for a human to process it. >>>> >>>> >>>> >>>> On Mon, Mar 10, 2014 at 3:13 AM, Steven Hartland < >>>> steven.hartland@multiplay.co.uk> wrote: >>>> >>>> Sorry guys been off ill for a while, signed this morning for you. >>>> >>>>> >>>>> Regards >>>>> Steve >>>>> ----- Original Message ----- From: "Brad Fitzpatrick" < >>>>> bradfitz@golang.org >>>>> > >>>>> To: <steven.hartland@multiplay.co.uk>; <golang-codereviews@ >>>>> googlegroups.com>; "Brad Fitzpatrick" <bradfitz@golang.org>; "Russ >>>>> Cox" >>>>> < >>>>> rsc@golang.org>; <reply@codereview-hr.appspotmail.com> >>>>> Sent: Wednesday, March 05, 2014 9:56 PM >>>>> Subject: Re: code review 56060045: net/http/cgi: Fix leaking CGI >>>>> processes >>>>> (issue 56060045) >>>>> >>>>> >>>>> >>>>> Steven, you also haven't submitted a CLA: >>>>> >>>>> >>>>>> See http://golang.org/doc/contribute.html#copyright >>>>>> >>>>>> >>>>>> On Wed, Mar 5, 2014 at 12:02 PM, <rsc@golang.org> wrote: >>>>>> >>>>>> ping? >>>>>> >>>>>> >>>>>>> if this is going to go in, we need this review to wrap up in the next >>>>>>> few days. with a test. >>>>>>> >>>>>>> >>>>>>> https://codereview.appspot.com/56060045/ >>>>>>> >>>>>>> >>>>>>> >>>>>>> >>>>>> ================================================ >>>>> This e.mail is private and confidential between Multiplay (UK) Ltd. and >>>>> the person or entity to whom it is addressed. In the event of >>>>> misdirection, >>>>> the recipient is prohibited from using, copying, printing or otherwise >>>>> disseminating it or any information contained in it. >>>>> In the event of misdirection, illegible or incomplete transmission >>>>> please >>>>> telephone +44 845 868 1337 >>>>> or return the E.mail to postmaster@multiplay.co.uk. >>>>> >>>>> >>>>> >>>>> >>>> ================================================ >>> This e.mail is private and confidential between Multiplay (UK) Ltd. and >>> the person or entity to whom it is addressed. In the event of >>> misdirection, >>> the recipient is prohibited from using, copying, printing or otherwise >>> disseminating it or any information contained in it. >>> In the event of misdirection, illegible or incomplete transmission please >>> telephone +44 845 868 1337 >>> or return the E.mail to postmaster@multiplay.co.uk. >>> >>> >>> >> > ================================================ > This e.mail is private and confidential between Multiplay (UK) Ltd. and > the person or entity to whom it is addressed. In the event of misdirection, > the recipient is prohibited from using, copying, printing or otherwise > disseminating it or any information contained in it. > In the event of misdirection, illegible or incomplete transmission please > telephone +44 845 868 1337 > or return the E.mail to postmaster@multiplay.co.uk. > >
Sign in to reply to this message.
|
