[go] os/exec: don't wait if grandchild holds pipe open
205 views
Skip to first unread message
Ian Lance Taylor (Gerrit)
Feb 15, 2021, 9:38:42 PM2/15/21
to goph...@pubsubhelper.golang.org, golang-co...@googlegroups.com
Ian Lance Taylor has uploaded this change for review.
os/exec: don't wait if grandchild holds pipe open
If a pipe is used to read standard output or standard error from a
child process, and the child process passes the pipe to a grandchild
process, we used to wait until the grandchild process closed the pipe
(typically this meant waiting for the grandchild process to exit).
With this change, we stop waiting once the child process exits,
even if a grandchild process is keeping the pipe open.
Fixes #23019
Change-Id: I3eb6d46e7e8e553d87e1e4357a8e5a7d9a295ea4
---
M src/os/exec/exec.go
M src/os/exec/exec_test.go
2 files changed, 107 insertions(+), 3 deletions(-)
diff --git a/src/os/exec/exec.go b/src/os/exec/exec.go
index 0c49575..b8e2525 100644
--- a/src/os/exec/exec.go
+++ b/src/os/exec/exec.go
@@ -33,6 +33,7 @@
"strings"
"sync"
"syscall"
+ "time"
)
// Error is returned by LookPath when it fails to classify a file as an
@@ -305,16 +306,75 @@
return
}
+ if c.waitDone == nil {
+ c.waitDone = make(chan struct{})
+ }
+ cr := waiterReader{pr, c.waitDone}
c.closeAfterStart = append(c.closeAfterStart, pw)
c.closeAfterWait = append(c.closeAfterWait, pr)
c.goroutine = append(c.goroutine, func() error {
- _, err := io.Copy(w, pr)
- pr.Close() // in case io.Copy stopped due to write error
+ _, err := io.Copy(w, cr)
+ cr.Close() // in case io.Copy stopped due to write error
return err
})
return pw, nil
}
+// A waiterReader reads from r until EOF or ch is closed.
+// The r field is always a pipe created by os.Pipe.
+type waiterReader struct {
+ r *os.File
+ ch chan struct{}
+}
+
+func (wr waiterReader) Read(buf []byte) (int, error) {
+ // In the normal case the pipe, wr.r, will EOF when the child exits.
+ // However, if the child starts a grandchild, and passes the
+ // descriptor down, and the child exits before the grandchild,
+ // then the grandchild will hold the pipe open. In that case
+ // we don't want to wait for the grandchild. We use a timeout
+ // on the read to avoid waiting too long.
+ // See issue #23019 for background.
+ const timeout = 100 * time.Millisecond
+
+ var (
+ nr int
+ err error
+ )
+ for {
+ // Ignore errors on SetReadDeadline. If we can't set a
+ // deadline on a pipe, we just have to wait.
+ wr.r.SetReadDeadline(time.Now().Add(timeout))
+
+ nr, err = wr.r.Read(buf)
+
+ if err != nil && errors.Is(err, os.ErrDeadlineExceeded) {
+ select {
+ case <-wr.ch:
+ // The child has exited and the read timed out.
+ // Treat this as EOF.
+ return nr, io.EOF
+ default:
+ // The child is still running.
+ if nr == 0 {
+ // Do another read.
+ continue
+ }
+ // Return the data we got, with no error.
+ err = nil
+ }
+ }
+
+ break
+ }
+
+ return nr, err
+}
+
+func (wr waiterReader) Close() error {
+ return wr.r.Close()
+}
+
func (c *Cmd) closeDescriptors(closers []io.Closer) {
for _, fd := range closers {
fd.Close()
@@ -444,7 +504,9 @@
}
if c.ctx != nil {
- c.waitDone = make(chan struct{})
+ if c.waitDone == nil {
+ c.waitDone = make(chan struct{})
+ }
go func() {
select {
case <-c.ctx.Done():
diff --git a/src/os/exec/exec_test.go b/src/os/exec/exec_test.go
index 8b0c93f..2b1227d 100644
--- a/src/os/exec/exec_test.go
+++ b/src/os/exec/exec_test.go
@@ -915,6 +915,16 @@
case "sleep":
time.Sleep(3 * time.Second)
os.Exit(0)
+ case "startgrandchild":
+ cmd := exec.Command(os.Args[0], "-test.run=TestHelperProcess", "--", "sleep")
+ cmd.Stdin = os.Stdin
+ cmd.Stdout = os.Stdout
+ cmd.Stderr = os.Stderr
+ if err := cmd.Start(); err != nil {
+ fmt.Fprintf(os.Stderr, "Start: %v\n", err)
+ os.Exit(1)
+ }
+ os.Exit(0)
default:
fmt.Fprintf(os.Stderr, "Unknown command %q\n", cmd)
os.Exit(2)
@@ -1174,3 +1184,35 @@
t.Error("no SYSTEMROOT found")
}
}
+
+// Test that if we start a child process with stdout going to
+// something that requires us to create a pipe, and that process
+// starts a grandchild process, and the child process exits, that we
+// don't wait for the grandchild process to exit.
+func TestChildExits(t *testing.T) {
+ if os.Getenv("GO_TEST_TIMEOUT_SCALE") != "" {
+ // Skip this test on slow systems.
+ t.Skipf("skipping because GO_TEST_TIMEOUT_SCALE is set")
+ }
+
+ cmd := helperCommand(t, "startgrandchild")
+ var buf bytes.Buffer
+ cmd.Stdin = os.Stdin
+ cmd.Stdout = &buf
+ cmd.Stderr = &buf
+ now := time.Now()
+ if err := cmd.Run(); err != nil {
+ t.Error(err)
+ }
+ if buf.Len() > 0 {
+ t.Errorf("%s", buf.Bytes())
+ }
+
+ // The child will exit as soon as it has started the grandchild.
+ // The grandchild will sleep for 3 seconds. Run should only
+ // take as long as it takes for the child to exit; it should
+ // not wait for the grandchild to exit. We give it 2.5 seconds.
+ if since := time.Since(now); since > 2500*time.Millisecond {
+ t.Errorf("child took too long: %v", since)
+ }
+}
To view, visit change 292130. To unsubscribe, or for help writing mail filters, visit settings.
Ian Lance Taylor (Gerrit)
Feb 15, 2021, 9:38:52 PM2/15/21
to goph...@pubsubhelper.golang.org, golang-co...@googlegroups.com
Patch set 1:Run-TryBot +1Trust +1
1 comment:
Patchset:
RELNOTE=yes
To view, visit change 292130. To unsubscribe, or for help writing mail filters, visit settings.
Ian Lance Taylor (Gerrit)
Feb 20, 2021, 2:49:44 PM2/20/21
to goph...@pubsubhelper.golang.org, golang-co...@googlegroups.com
Attention is currently required from: Brad Fitzpatrick, Ian Lance Taylor.
Ian Lance Taylor uploaded patch set #2 to this change.
os/exec: don't wait if grandchild holds pipe open
If a pipe is used to read standard output or standard error from a
child process, and the child process passes the pipe to a grandchild
process, we used to wait until the grandchild process closed the pipe
(typically this meant waiting for the grandchild process to exit).
With this change, we stop waiting once the child process exits,
even if a grandchild process is keeping the pipe open.
Fixes #23019
Change-Id: I3eb6d46e7e8e553d87e1e4357a8e5a7d9a295ea4
---
M src/os/exec/exec.go
M src/os/exec/exec_test.go
2 files changed, 105 insertions(+), 3 deletions(-)
To view, visit change 292130. To unsubscribe, or for help writing mail filters, visit settings.
Ian Lance Taylor (Gerrit)
Feb 20, 2021, 3:22:37 PM2/20/21
to goph...@pubsubhelper.golang.org, golang-co...@googlegroups.com
Attention is currently required from: Brad Fitzpatrick, Ian Lance Taylor.
Ian Lance Taylor uploaded patch set #3 to this change.
os/exec: don't wait if grandchild holds pipe open
If a pipe is used to read standard output or standard error from a
child process, and the child process passes the pipe to a grandchild
process, we used to wait until the grandchild process closed the pipe
(typically this meant waiting for the grandchild process to exit).
With this change, we stop waiting once the child process exits,
even if a grandchild process is keeping the pipe open.
Fixes #23019
Change-Id: I3eb6d46e7e8e553d87e1e4357a8e5a7d9a295ea4
---
M src/os/exec/exec.go
M src/os/exec/exec_test.go
2 files changed, 111 insertions(+), 3 deletions(-)
To view, visit change 292130. To unsubscribe, or for help writing mail filters, visit settings.
Brad Fitzpatrick (Gerrit)
Feb 22, 2021, 10:47:45 AM2/22/21
to Ian Lance Taylor, goph...@pubsubhelper.golang.org, Go Bot, Brad Fitzpatrick, golang-co...@googlegroups.com
Attention is currently required from: Ian Lance Taylor.
2 comments:
File src/os/exec/exec.go:
Patch Set #3, Line 340:
ch chan struct{}<-chan struct{}
We don't set the deadline in the past because that may cause the Read
// to fail with a deadline error before the kernel actually delivers the
// final bytes written to the pipe.
func (wr wakeReader) wake() {
// Ignore errors. If we can't set a deadline on a pipe,
// we'll just have to wait for the grandchild.
wr.r.SetReadDeadline(time.Now().Add(100 * time.Millisecond)this seems super scary/racy at first read.
sounds like we have 100ms for the kernel to do something, otherwise this is incorrect?
To view, visit change 292130. To unsubscribe, or for help writing mail filters, visit settings.
Ian Lance Taylor (Gerrit)
Feb 23, 2021, 12:24:03 AM2/23/21
to goph...@pubsubhelper.golang.org, Go Bot, Brad Fitzpatrick, golang-co...@googlegroups.com
Attention is currently required from: Brad Fitzpatrick.
1 comment:
File src/os/exec/exec.go:
We don't set the deadline in the past because that may cause the Read
// to fail with a deadline error before the kernel actually delivers the
// final bytes written to the pipe.
func (wr wakeReader) wake() {
// Ignore errors. If we can't set a deadline on a pipe,
// we'll just have to wait for the grandchild.
wr.r.SetReadDeadline(time.Now().Add(100 * time.Millisecond)
this seems super scary/racy at first read. […]
Yes, sorry, I jumped the gun on adding you as a reviewer. This CL isn't ready.
To view, visit change 292130. To unsubscribe, or for help writing mail filters, visit settings.
Ian Lance Taylor (Gerrit)
May 6, 2022, 7:10:18 PM5/6/22
to Ian Lance Taylor, goph...@pubsubhelper.golang.org, Gopher Robot, Brad Fitzpatrick, golang-co...@googlegroups.com
Ian Lance Taylor abandoned this change.
Abandoned
This CL doesn't work, and we're probably going to do issue #50436 instead.
To view, visit change 292130. To unsubscribe, or for help writing mail filters, visit settings.
Reply all
Reply to author
Forward
0 new messages