FAQ
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) {

Search Discussions

  • Dvyukov at Nov 9, 2012 at 10:22 am
    Perhaps you need to wait for the output reading goroutine to finish
    first, and only then call cmd.Wait() (merely to free resources).

    https://codereview.appspot.com/6821099/
  • Dvyukov at Nov 9, 2012 at 10:24 am

    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.


    https://codereview.appspot.com/6821099/
  • David Symonds at Nov 9, 2012 at 10:48 am

    On Fri, Nov 9, 2012 at 9:24 PM, 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.
  • Dmitry Vyukov at Nov 9, 2012 at 10:57 am

    On Fri, Nov 9, 2012 at 2:48 PM, David Symonds wrote:
    On Fri, Nov 9, 2012 at 9:24 PM, 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.
  • David Symonds at Nov 9, 2012 at 11:02 am

    On Fri, Nov 9, 2012 at 9:57 PM, Dmitry Vyukov 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.
  • Dmitry Vyukov at Nov 9, 2012 at 11:11 am

    On Fri, Nov 9, 2012 at 3:02 PM, David Symonds wrote:
    On Fri, Nov 9, 2012 at 9:57 PM, Dmitry Vyukov 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.
  • Roger peppe at Nov 12, 2012 at 9:02 am
    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, 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) {
  • Dmitry Vyukov at Nov 12, 2012 at 9:07 am

    On Mon, Nov 12, 2012 at 1:02 PM, roger peppe 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, 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) {
  • Roger peppe at Nov 12, 2012 at 10:58 am
    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 wrote:
    On Mon, Nov 12, 2012 at 1:02 PM, roger peppe 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, 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) {
  • David Symonds at Nov 12, 2012 at 9:27 am

    On Mon, Nov 12, 2012 at 8:02 PM, roger peppe 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.
  • Dsymonds at Nov 12, 2012 at 9:28 am

Related Discussions

Discussion Navigation
viewthread | post
Discussion Overview
groupgolang-dev @
categoriesgo
postedNov 9, '12 at 10:18a
activeNov 12, '12 at 10:58a
posts12
users3
websitegolang.org

People

Translate

site design / logo © 2022 Grokbase