code review 69970052: net/http/cgi: kill child CGI process on copy error

net/http/cgi: kill child CGI process on copy error

Fixes Issue 7196

[bradfitz, 2014-03-06 18:44:13]

Hello golang-codereviews@googlegroups.com (cc: iant@golang.org, rsc@golang.org),

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

[rsc, 2014-03-06 19:22:32]

LGTM

It's harmless as long as the pid doesn't get reused, and the pid shouldn't be
reused until cmd.Wait, which is in the defer, so it should be really harmless.

[bradfitz, 2014-03-06 19:24:33]

*** Submitted as https://code.google.com/p/go/source/detail?r=340da08f5f54 ***

net/http/cgi: kill child CGI process on copy error

Fixes Issue 7196

LGTM=rsc
R=golang-codereviews, rsc
CC=golang-codereviews, iant
https://codereview.appspot.com/69970052

[gobot, 2014-03-06 19:46:14]

This CL appears to have broken the darwin-386-cheney builder.

[0intro, 2014-03-06 21:31:11]

This change made the net/http/cgi test to hang on the Plan 9 builder. It doesn't
happen when running the test individually. I'm investigating.

[rsc, 2014-03-06 22:09:04]

does plan 9 implement os.(*Process).Kill?

[0intro, 2014-03-06 22:23:05]

> does plan 9 implement os.(*Process).Kill?

Yes, it's implemented in pkg/os/exec_plan9.go.

Disabling the call to p.Kill leads to a proper error like:

2014/03/06 22:19:22 cgi: copy error: past write limit
--- FAIL: TestKillChildAfterCopyError (5.00 seconds)
	matryoshka_test.go:119: timeout. ServeHTTP hung and didn't kill
the child process? matryoshka_test.go:123: killed process
FAIL
exit status: 'cgi.test 436170: 1'

The problem here is we still have two cgi.test processes
running after the test, which makes all.rc to hang.

-- 
David du Colombier

[0intro, 2014-03-06 23:05:45]

Could we disable this test on Plan 9 until we figure
out this problem? I can open an issue.

-- 
David du Colombier

[bradfitz, 2014-03-06 23:07:05]

I guess, but that's a pretty lame way to get an "ok" on the dashboard.  How
hard can this be to figure out?

I'd rather fix it quickly than risk forgetting about it (even with a bug
filed)

But either way.



On Thu, Mar 6, 2014 at 3:05 PM, David du Colombier <0intro@gmail.com> wrote:

> Could we disable this test on Plan 9 until we figure
> out this problem? I can open an issue.
>
> --
> David du Colombier
>

[0intro, 2014-03-06 23:18:44]

> I guess, but that's a pretty lame way to get an "ok" on the
> dashboard.  How hard can this be to figure out?

The problem I see here is that this test hangs the builder
and I will not have time to look at it until tomorrow night
at least, so we could miss other issues.

-- 
David du Colombier

[rsc, 2014-03-06 23:29:51]

can you run ps before and after the kill? my guess is that the kill is only
being delivered to one of the threads, it does not rebroadcast, and the
others continue to run.

russ

[0intro, 2014-03-06 23:40:25]

> can you run ps before and after the kill? my guess is that the kill
> is only being delivered to one of the threads, it does not
> rebroadcast, and the others continue to run.

Yes, I have the same feeling.

cpu% ps -a | grep cgi.test
djc          438880    0:00   0:00   793656K Tsemacqu
/tmp/go-build970100551/net/http/cgi/_test/cgi.test
djc          438881    0:00   0:00   793656K Semacqui cgi.test
djc          438882    0:00   0:00   793656K Tsemacqu cgi.test
djc          438883    0:00   0:00   793656K Await    cgi.test
djc          438889    0:00   0:00   793656K Tsemacqu
/tmp/go-build970100551/net/http/cgi/_test/cgi.test
-test.run=TestBeChildCGIProcess
djc          438890    0:00   0:00   793656K Semacqui cgi.test
djc          438891    0:00   0:00   793656K Pwrite   cgi.test
djc          438892    0:00   0:00   793656K Semacqui cgi.test
djc          438893    0:00   0:00   793656K Semacqui cgi.test

cpu% go test
2014/03/06 23:38:56 cgi: copy error: past write limit
pid 438889
PASS
ok  	net/http/cgi	5.243s

cpu% ps -a | grep cgi.test
djc          438890    0:00   0:00   793656K Semacqui cgi.test
djc          438892    0:00   0:00   793656K Semacqui cgi.test

-- 
David du Colombier

[ality, 2014-03-07 00:54:56]

Russ Cox <rsc@golang.org> once said:
> can you run ps before and after the kill? my guess is that the kill is only
> being delivered to one of the threads, it does not rebroadcast, and the
> others continue to run.

This is exactly right. The fix is to make os.(*Process).Kill
send a note to the process instead of writing "kill" to the
ctl file. The latter doesn't allow us to clean any procs
started by the runtime.

The downside to this is that if the child process is not a
Go program and is ignoring notes, then we won't be able to
kill it.

  Anthony

[0intro, 2014-03-07 15:43:44]

Sending a kill note to the process seems to lead to the same behavior
as writing to the ctl file. Any idea how to fix this issue?

Thanks.

-- 
David du Colombier

[ality, 2014-03-07 15:57:28]

David du Colombier <0intro@gmail.com> once said:
> Sending a kill note to the process seems to lead to the same behavior
> as writing to the ctl file. Any idea how to fix this issue?

We have to handle the kill note specially.

I'm on it. I've cleaned up the signal handling
and fixed this at the same time. I'm just waiting
on the CL for the "power of two" stack bug.

Once that lands, I'll test a bit more and update
my CL.

Cheers,
  Anthony

[0intro, 2014-03-07 16:17:42]

Thanks. Don't hesitate to share your CL when it will be ready, so I
could take a look and try on my side.

-- 
David du Colombier