Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(59)

Issue 6903061: code review 6903061: cmd/go: handle os signals (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
9 years, 6 months ago by brainman
Modified:
9 years, 6 months ago
Reviewers:
CC:
golang-dev, dfc, rsc
Visibility:
Public.

Description

cmd/go: handle os signals Ignore signals during "go run" and wait for running child process to exit. Stop executing further tests during "go test", wait for running tests to exit and report error exit code. Original CL 6351053 by dfc. Fixes issue 3572. Fixes issue 3581.

Patch Set 1 #

Patch Set 2 : diff -r 3c932286e5f5 https://go.googlecode.com/hg/ #

Patch Set 3 : diff -r 3c932286e5f5 https://go.googlecode.com/hg/ #

Total comments: 3

Patch Set 4 : diff -r 3c932286e5f5 https://go.googlecode.com/hg/ #

Total comments: 2

Patch Set 5 : diff -r 7b9e9fc59eb5 https://go.googlecode.com/hg/ #

Total comments: 2

Patch Set 6 : diff -r 8b89b6326704 https://go.googlecode.com/hg/ #

Patch Set 7 : diff -r b1822aac85b7 https://go.googlecode.com/hg/ #

Total comments: 2

Patch Set 8 : diff -r b1822aac85b7 https://go.googlecode.com/hg/ #

Total comments: 1

Patch Set 9 : diff -r b1822aac85b7 https://go.googlecode.com/hg/ #

Patch Set 10 : diff -r b1822aac85b7 https://go.googlecode.com/hg/ #

Unified diffs Side-by-side diffs Delta from patch set Stats (+82 lines, -10 lines) Patch
M src/cmd/dist/build.c View 1 1 chunk +1 line, -0 lines 0 comments Download
M src/cmd/go/build.go View 1 2 3 4 5 6 3 chunks +21 lines, -10 lines 0 comments Download
M src/cmd/go/run.go View 1 2 3 1 chunk +1 line, -0 lines 0 comments Download
A src/cmd/go/signal.go View 1 2 3 4 5 6 7 8 1 chunk +31 lines, -0 lines 0 comments Download
A src/cmd/go/signal_notunix.go View 1 1 chunk +13 lines, -0 lines 0 comments Download
A src/cmd/go/signal_unix.go View 1 1 chunk +14 lines, -0 lines 0 comments Download
M src/cmd/go/test.go View 1 2 3 1 chunk +1 line, -0 lines 0 comments Download

Messages

Total messages: 33
brainman
Hello golang-dev@googlegroups.com (cc: dave@cheney.net), I'd like you to review this change to https://go.googlecode.com/hg/
9 years, 6 months ago (2012-12-10 02:00:19 UTC) #1
dfc
Thank you for tackling this. In 6351053 rsc ask that signals be ignored for go ...
9 years, 6 months ago (2012-12-10 02:34:53 UTC) #2
brainman
Hello golang-dev@googlegroups.com, dave@cheney.net (cc: golang-dev@googlegroups.com), Please take another look.
9 years, 6 months ago (2012-12-10 03:34:55 UTC) #3
brainman
On 2012/12/10 02:34:53, dfc wrote: > ... In 6351053 rsc ask that signals be ignored ...
9 years, 6 months ago (2012-12-10 03:35:39 UTC) #4
dfc
LGTM. This works as expected on linux. Interesting sidebar, if you do go test ./... ...
9 years, 6 months ago (2012-12-10 03:52:52 UTC) #5
dfc
Actually, one more thing ^c go test now does not stop the tests, it should ...
9 years, 6 months ago (2012-12-10 05:07:49 UTC) #6
rsc
Thanks very much for working on this. I agree with Dave: the loop should set ...
9 years, 6 months ago (2012-12-10 06:41:16 UTC) #7
brainman
Hello golang-dev@googlegroups.com, dave@cheney.net, rsc@golang.org (cc: golang-dev@googlegroups.com), Please take another look.
9 years, 6 months ago (2012-12-11 06:25:36 UTC) #8
brainman
https://codereview.appspot.com/6903061/diff/3/src/cmd/go/signal.go File src/cmd/go/signal.go (right): https://codereview.appspot.com/6903061/diff/3/src/cmd/go/signal.go#newcode20 src/cmd/go/signal.go:20: fmt.Fprintf(os.Stderr, "\n%s: %s received, ignoring\n", os.Args[0], s) On 2012/12/10 ...
9 years, 6 months ago (2012-12-11 06:25:42 UTC) #9
rsc
LGTM https://codereview.appspot.com/6903061/diff/13001/src/cmd/go/signal.go File src/cmd/go/signal.go (right): https://codereview.appspot.com/6903061/diff/13001/src/cmd/go/signal.go#newcode16 src/cmd/go/signal.go:16: // interrupted returns true, if go process received ...
9 years, 6 months ago (2012-12-11 16:34:58 UTC) #10
dfc
NOT LGTM try go test net/http send SIGQUIT to the test process via ^\ go ...
9 years, 6 months ago (2012-12-13 02:10:03 UTC) #11
rsc
Shows how much I know about signals I guess. Any ideas why, Dave?
9 years, 6 months ago (2012-12-13 02:48:55 UTC) #12
brainman
On 2012/12/13 02:10:03, dfc wrote: > ... > go test itself will hang, ^C or ...
9 years, 6 months ago (2012-12-13 02:49:03 UTC) #13
dfc
At a guess the test process is exiting, but the go tool has lost a ...
9 years, 6 months ago (2012-12-13 02:54:16 UTC) #14
brainman
https://codereview.appspot.com/6903061/diff/13001/src/cmd/go/signal.go File src/cmd/go/signal.go (right): https://codereview.appspot.com/6903061/diff/13001/src/cmd/go/signal.go#newcode16 src/cmd/go/signal.go:16: // interrupted returns true, if go process received signal. ...
9 years, 6 months ago (2012-12-13 03:18:51 UTC) #15
brainman
On 2012/12/13 02:54:16, dfc wrote: > At a guess the test process is exiting, but ...
9 years, 6 months ago (2012-12-13 03:21:19 UTC) #16
brainman
I have stared plenty at my change. I cannot see any problems. Dave, I will ...
9 years, 6 months ago (2012-12-13 06:11:27 UTC) #17
dfc
Thank Alex, I'm having a look now. What I see is also a lot of ...
9 years, 6 months ago (2012-12-13 08:45:24 UTC) #18
dfc
ok, what I see in the simple case (go test go/build) is the go process ...
9 years, 6 months ago (2012-12-13 08:50:57 UTC) #19
dfc
Hi Alex, please consider this alternative. https://codereview.appspot.com/6943043 I believe the problem is because my host ...
9 years, 6 months ago (2012-12-13 10:57:30 UTC) #20
brainman
On 2012/12/13 10:57:30, dfc wrote: > Hi Alex, please consider this alternative. > https://codereview.appspot.com/6943043 > ...
9 years, 6 months ago (2012-12-13 23:13:51 UTC) #21
dfc
You've done the hard work on this CL, you should take the credit. I think ...
9 years, 6 months ago (2012-12-13 23:15:53 UTC) #22
brainman
Hello golang-dev@googlegroups.com, dave@cheney.net, rsc@golang.org (cc: golang-dev@googlegroups.com), Please take another look.
9 years, 6 months ago (2012-12-13 23:40:26 UTC) #23
dfc
https://codereview.appspot.com/6903061/diff/22002/src/cmd/go/signal.go File src/cmd/go/signal.go (right): https://codereview.appspot.com/6903061/diff/22002/src/cmd/go/signal.go#newcode28 src/cmd/go/signal.go:28: onceCloseInterrupted.Do(closeInterrupted) Why do we need to keep this func ...
9 years, 6 months ago (2012-12-13 23:45:54 UTC) #24
brainman
https://codereview.appspot.com/6903061/diff/22002/src/cmd/go/signal.go File src/cmd/go/signal.go (right): https://codereview.appspot.com/6903061/diff/22002/src/cmd/go/signal.go#newcode28 src/cmd/go/signal.go:28: onceCloseInterrupted.Do(closeInterrupted) On 2012/12/13 23:45:54, dfc wrote: > Why do ...
9 years, 6 months ago (2012-12-13 23:53:19 UTC) #25
dfc
> go process is ignoring these signals, so, I think, we can get second signal ...
9 years, 6 months ago (2012-12-13 23:56:20 UTC) #26
brainman
Hello golang-dev@googlegroups.com, dave@cheney.net, rsc@golang.org (cc: golang-dev@googlegroups.com), Please take another look.
9 years, 6 months ago (2012-12-14 00:06:01 UTC) #27
dfc
https://codereview.appspot.com/6903061/diff/29009/src/cmd/go/signal.go File src/cmd/go/signal.go (right): https://codereview.appspot.com/6903061/diff/29009/src/cmd/go/signal.go#newcode28 src/cmd/go/signal.go:28: onceCloseInterrupted.Do(closeInterrupted) I'm sorry to harp on this. I don't ...
9 years, 6 months ago (2012-12-14 00:16:44 UTC) #28
dfc
Sorry, I meant to write go func() { <- sig close(interrupted) }()
9 years, 6 months ago (2012-12-14 00:17:30 UTC) #29
brainman
Hello golang-dev@googlegroups.com, dave@cheney.net, rsc@golang.org (cc: golang-dev@googlegroups.com), Please take another look.
9 years, 6 months ago (2012-12-14 00:26:14 UTC) #30
dfc
LGTM. All my manual tests pass. Thank you for your persistence, this is going to ...
9 years, 6 months ago (2012-12-14 00:39:16 UTC) #31
brainman
On 2012/12/14 00:39:16, dfc wrote: > LGTM. All my manual tests pass. > Thank you ...
9 years, 6 months ago (2012-12-14 00:41:58 UTC) #32
brainman
9 years, 6 months ago (2012-12-14 06:34:10 UTC) #33
*** Submitted as https://code.google.com/p/go/source/detail?r=ee5afd4b14b7 ***

cmd/go: handle os signals

Ignore signals during "go run" and wait for running child
process to exit. Stop executing further tests during "go test",
wait for running tests to exit and report error exit code.

Original CL 6351053 by dfc.

Fixes issue 3572.
Fixes issue 3581.

R=golang-dev, dave, rsc
CC=golang-dev
https://codereview.appspot.com/6903061
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b