|
|
Descriptionos/exec: add test that fails under race detector.
This seems like legitimate code to me, and naturally occurs
if you wrap an *exec.Cmd in something that watches the output.
I don't know the correct way to solve this race, if it is indeed valid.
Patch Set 1 #Patch Set 2 : diff -r 7d57660045db https://code.google.com/p/go #Patch Set 3 : diff -r 7d57660045db https://code.google.com/p/go #MessagesTotal messages: 12
Hello bradfitz, dvyukov@google.com (cc: golang-dev@googlegroups.com), I'd like you to review this change to https://code.google.com/p/go
Sign in to reply to this message.
Perhaps you need to wait for the output reading goroutine to finish first, and only then call cmd.Wait() (merely to free resources).
Sign in to reply to this message.
On 2012/11/09 10:22:08, dvyukov wrote: > Perhaps you need to wait for the output reading goroutine to finish first, and > only then call cmd.Wait() (merely to free resources). In this case you do not need the goroutine at all -- cmd.Start, read until EOF, cmd.Wait.
Sign in to reply to this message.
On Fri, Nov 9, 2012 at 9:24 PM, <dvyukov@google.com> wrote: > On 2012/11/09 10:22:08, dvyukov wrote: >> >> Perhaps you need to wait for the output reading goroutine to finish > > first, and >> >> only then call cmd.Wait() (merely to free resources). > > > In this case you do not need the goroutine at all -- cmd.Start, read > until EOF, cmd.Wait. The test is a little bit artificial in that regard, but imagine passing the pipe off to something else as a plain io.Reader, and then independently waiting for the command to finish. There doesn't feel like those should have to be coordinated, which is why I think perhaps os/exec needs some locking.
Sign in to reply to this message.
On Fri, Nov 9, 2012 at 2:48 PM, David Symonds <dsymonds@golang.org> wrote: > On Fri, Nov 9, 2012 at 9:24 PM, <dvyukov@google.com> wrote: > > > On 2012/11/09 10:22:08, dvyukov wrote: > >> > >> Perhaps you need to wait for the output reading goroutine to finish > > > > first, and > >> > >> only then call cmd.Wait() (merely to free resources). > > > > > > In this case you do not need the goroutine at all -- cmd.Start, read > > until EOF, cmd.Wait. > > The test is a little bit artificial in that regard, but imagine > passing the pipe off to something else as a plain io.Reader, and then > independently waiting for the command to finish. Do you mean that plain io.Writer can block and we do not want to wait for it? Because otherwise it seems fine w/o the goroutine. > There doesn't feel > like those should have to be coordinated, which is why I think perhaps > os/exec needs some locking. >
Sign in to reply to this message.
On Fri, Nov 9, 2012 at 9:57 PM, Dmitry Vyukov <dvyukov@google.com> wrote: > Do you mean that plain io.Writer can block and we do not want to wait for > it? Because otherwise it seems fine w/o the goroutine. No, I mean the operation using the io.Writer (from StdoutPipe) might be completely unrelated to the main flow. Imagine, say, some sort of logger that's reading from it (until io.EOF), maybe grepping for something, and is running in its own goroutine because it's logically separable from the main part of our code, which is only interested in waiting for the command to complete. The race can be avoided by the logger having some signal to indicate it has reached io.EOF before the main part calls cmd.Wait, but I'm suggesting that that should not be necessary.
Sign in to reply to this message.
On Fri, Nov 9, 2012 at 3:02 PM, David Symonds <dsymonds@golang.org> wrote: > On Fri, Nov 9, 2012 at 9:57 PM, Dmitry Vyukov <dvyukov@google.com> wrote: > > > Do you mean that plain io.Writer can block and we do not want to wait for > > it? Because otherwise it seems fine w/o the goroutine. > > No, I mean the operation using the io.Writer (from StdoutPipe) might > be completely unrelated to the main flow. Imagine, say, some sort of > logger that's reading from it (until io.EOF), maybe grepping for > something, and is running in its own goroutine because it's logically > separable from the main part of our code, which is only interested in > waiting for the command to complete. The race can be avoided by the > logger having some signal to indicate it has reached io.EOF before the > main part calls cmd.Wait, but I'm suggesting that that should not be > necessary. > You can wrap the pipe into Writer that will automatically signal waiting goroutine. Then you can pass it to the logger. It can be done in os.exec as well. It may have some problems if the user does not read from the pipe till EOF, though.
Sign in to reply to this message.
I think this is issue 4290. I have a CL that fixes it: https://codereview.appspot.com/6789043/ (I have been waiting for a reply for a while now, as I'm not entirely sure about one aspect). Dmitry: waiting for the output reading goroutine to finish first is not always sufficient - when and whether you will get EOF on the output is actually independent of when Wait terminates (the process you've started may close its stdout early; or it may fork another process that keeps the pipe open while the original exits). I believe it's just wrong that Wait closes the client side of the pipe, as it means it is almost never correct to call Wait while reading from the output pipe. I think this will be the source of bugs in quite a few programs, but I am willing to be persuaded otherwise. On 9 November 2012 10:17, <dsymonds@golang.org> wrote: > Reviewers: bradfitz, dvyukov, > > Message: > Hello bradfitz, dvyukov@google.com (cc: golang-dev@googlegroups.com), > > I'd like you to review this change to > https://code.google.com/p/go > > > Description: > os/exec: add test that fails under race detector. > > This seems like legitimate code to me, and naturally occurs > if you wrap an *exec.Cmd in something that watches the output. > I don't know the correct way to solve this race, if it is indeed valid. > > Please review this at http://codereview.appspot.com/6821099/ > > Affected files: > M src/pkg/os/exec/exec_test.go > > > Index: src/pkg/os/exec/exec_test.go > =================================================================== > --- a/src/pkg/os/exec/exec_test.go > +++ b/src/pkg/os/exec/exec_test.go > @@ -265,6 +265,44 @@ > } > } > > +func TestConcurrentReadWait(t *testing.T) { > + // Test that it is safe to concurrently read stdout and wait for a > process to finish. > + cmd := helperCommand("echo", "foo") > + out, err := cmd.StdoutPipe() > + if err != nil { > + t.Fatalf("cmd.StdoutPipe: %v", err) > + } > + > + rd := make(chan string) > + go func() { > + r := bufio.NewReader(out) > + var s string > + for { > + line, _, err := r.ReadLine() > + if err != nil { > + if err != io.EOF { > + t.Errorf("ReadLine: %v", err) > + } > + break > + } > + s += string(line) > + } > + rd <- s > + }() > + > + if err := cmd.Start(); err != nil { > + t.Fatalf("cmd.Start: %v", err) > + } > + if err := cmd.Wait(); err != nil { > + t.Fatalf("cmd.Wait: %v", err) > + } > + s := <-rd > + > + if s != "foo" { > + t.Errorf(`Command output was %q, want "foo"`, s) > + } > +} > + > // TestHelperProcess isn't a real test. It's used as a helper process > // for TestParameterRun. > func TestHelperProcess(*testing.T) { > >
Sign in to reply to this message.
On Mon, Nov 12, 2012 at 1:02 PM, roger peppe <rogpeppe@gmail.com> wrote: > I think this is issue 4290. > > I have a CL that fixes it: https://codereview.appspot.com/6789043/ > (I have been waiting for a reply for a while now, as I'm not entirely > sure about one aspect). > > Dmitry: waiting for the output reading goroutine to finish first is not > always sufficient - when and whether you will get EOF on the output is > actually independent of when Wait terminates (the process you've started > may close its stdout early; or it may fork another process that keeps > the pipe open while the original exits). > > I believe it's just wrong that Wait closes the client side of the pipe, > as it means it is almost never correct to call Wait while > reading from the output pipe. I think this will be the source of bugs in > quite a few programs, but I am willing to be persuaded otherwise. > > Doesn't your patch change public API and break existing programs? I mean it may lead to leaked decriptors. > > On 9 November 2012 10:17, <dsymonds@golang.org> wrote: > > Reviewers: bradfitz, dvyukov, > > > > Message: > > Hello bradfitz, dvyukov@google.com (cc: golang-dev@googlegroups.com), > > > > I'd like you to review this change to > > https://code.google.com/p/go > > > > > > Description: > > os/exec: add test that fails under race detector. > > > > This seems like legitimate code to me, and naturally occurs > > if you wrap an *exec.Cmd in something that watches the output. > > I don't know the correct way to solve this race, if it is indeed valid. > > > > Please review this at http://codereview.appspot.com/6821099/ > > > > Affected files: > > M src/pkg/os/exec/exec_test.go > > > > > > Index: src/pkg/os/exec/exec_test.go > > =================================================================== > > --- a/src/pkg/os/exec/exec_test.go > > +++ b/src/pkg/os/exec/exec_test.go > > @@ -265,6 +265,44 @@ > > } > > } > > > > +func TestConcurrentReadWait(t *testing.T) { > > + // Test that it is safe to concurrently read stdout and wait for > a > > process to finish. > > + cmd := helperCommand("echo", "foo") > > + out, err := cmd.StdoutPipe() > > + if err != nil { > > + t.Fatalf("cmd.StdoutPipe: %v", err) > > + } > > + > > + rd := make(chan string) > > + go func() { > > + r := bufio.NewReader(out) > > + var s string > > + for { > > + line, _, err := r.ReadLine() > > + if err != nil { > > + if err != io.EOF { > > + t.Errorf("ReadLine: %v", err) > > + } > > + break > > + } > > + s += string(line) > > + } > > + rd <- s > > + }() > > + > > + if err := cmd.Start(); err != nil { > > + t.Fatalf("cmd.Start: %v", err) > > + } > > + if err := cmd.Wait(); err != nil { > > + t.Fatalf("cmd.Wait: %v", err) > > + } > > + s := <-rd > > + > > + if s != "foo" { > > + t.Errorf(`Command output was %q, want "foo"`, s) > > + } > > +} > > + > > // TestHelperProcess isn't a real test. It's used as a helper process > > // for TestParameterRun. > > func TestHelperProcess(*testing.T) { > > > > >
Sign in to reply to this message.
On Mon, Nov 12, 2012 at 8:02 PM, roger peppe <rogpeppe@gmail.com> wrote: > I think this is issue 4290. > > I have a CL that fixes it: https://codereview.appspot.com/6789043/ > (I have been waiting for a reply for a while now, as I'm not entirely > sure about one aspect). Yes, your CL subsumes this one. It's got essentially the same test, so I'll abandon this CL.
Sign in to reply to this message.
*** Abandoned ***
Sign in to reply to this message.
Yes, if there is code out there that is relying on Wait to close the pipe, then it will leak. Three points though: - We're fixing a bug (IMHO), and that's allowed. - Most reasonable code is going to see that StdoutPipe is returning an io.ReadCloser, and close it anyway. - It's not a permanent leak - the finalizer will get it eventually. On 12 November 2012 09:07, Dmitry Vyukov <dvyukov@google.com> wrote: > On Mon, Nov 12, 2012 at 1:02 PM, roger peppe <rogpeppe@gmail.com> wrote: >> >> I think this is issue 4290. >> >> I have a CL that fixes it: https://codereview.appspot.com/6789043/ >> (I have been waiting for a reply for a while now, as I'm not entirely >> sure about one aspect). >> >> Dmitry: waiting for the output reading goroutine to finish first is not >> always sufficient - when and whether you will get EOF on the output is >> actually independent of when Wait terminates (the process you've started >> may close its stdout early; or it may fork another process that keeps >> the pipe open while the original exits). >> >> I believe it's just wrong that Wait closes the client side of the pipe, >> as it means it is almost never correct to call Wait while >> reading from the output pipe. I think this will be the source of bugs in >> quite a few programs, but I am willing to be persuaded otherwise. >> > > > Doesn't your patch change public API and break existing programs? I mean it > may lead to leaked decriptors. > > >> >> >> On 9 November 2012 10:17, <dsymonds@golang.org> wrote: >> > Reviewers: bradfitz, dvyukov, >> > >> > Message: >> > Hello bradfitz, dvyukov@google.com (cc: golang-dev@googlegroups.com), >> > >> > I'd like you to review this change to >> > https://code.google.com/p/go >> > >> > >> > Description: >> > os/exec: add test that fails under race detector. >> > >> > This seems like legitimate code to me, and naturally occurs >> > if you wrap an *exec.Cmd in something that watches the output. >> > I don't know the correct way to solve this race, if it is indeed valid. >> > >> > Please review this at http://codereview.appspot.com/6821099/ >> > >> > Affected files: >> > M src/pkg/os/exec/exec_test.go >> > >> > >> > Index: src/pkg/os/exec/exec_test.go >> > =================================================================== >> > --- a/src/pkg/os/exec/exec_test.go >> > +++ b/src/pkg/os/exec/exec_test.go >> > @@ -265,6 +265,44 @@ >> > } >> > } >> > >> > +func TestConcurrentReadWait(t *testing.T) { >> > + // Test that it is safe to concurrently read stdout and wait for >> > a >> > process to finish. >> > + cmd := helperCommand("echo", "foo") >> > + out, err := cmd.StdoutPipe() >> > + if err != nil { >> > + t.Fatalf("cmd.StdoutPipe: %v", err) >> > + } >> > + >> > + rd := make(chan string) >> > + go func() { >> > + r := bufio.NewReader(out) >> > + var s string >> > + for { >> > + line, _, err := r.ReadLine() >> > + if err != nil { >> > + if err != io.EOF { >> > + t.Errorf("ReadLine: %v", err) >> > + } >> > + break >> > + } >> > + s += string(line) >> > + } >> > + rd <- s >> > + }() >> > + >> > + if err := cmd.Start(); err != nil { >> > + t.Fatalf("cmd.Start: %v", err) >> > + } >> > + if err := cmd.Wait(); err != nil { >> > + t.Fatalf("cmd.Wait: %v", err) >> > + } >> > + s := <-rd >> > + >> > + if s != "foo" { >> > + t.Errorf(`Command output was %q, want "foo"`, s) >> > + } >> > +} >> > + >> > // TestHelperProcess isn't a real test. It's used as a helper process >> > // for TestParameterRun. >> > func TestHelperProcess(*testing.T) { >> > >> > > >
Sign in to reply to this message.
|