Code review - Issue 6821099: code review 6821099: os/exec: add test that fails under race detector.https://codereview.appspot.com/2012-11-12T10:58:28+00:00rietveld
Message from unknown
2012-11-09T10:17:04+00:00dsymondsurn:md5:bec19426b7a7f7dc1a2ed88a5b4cf709
Message from unknown
2012-11-09T10:17:08+00:00dsymondsurn:md5:d7c799c9424cf544cb9752805047c1d1
Message from unknown
2012-11-09T10:17:53+00:00dsymondsurn:md5:86f04e899cbaecf899360fadda13a60c
Message from dsymonds@golang.org
2012-11-09T10:17:58+00:00dsymondsurn:md5:4e518cb5e44e5ea18d0fac2528ffef1c
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
Message from dvyukov@google.com
2012-11-09T10:22:08+00:00dvyukovurn:md5:ce2acd4f3bbc2e55411503c1565de308
Perhaps you need to wait for the output reading goroutine to finish first, and only then call cmd.Wait() (merely to free resources).
Message from dvyukov@google.com
2012-11-09T10:24:07+00:00dvyukovurn:md5:8936174db0271f4884fd0db2a74a58b6
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.
Message from dsymonds@golang.org
2012-11-09T10:48:21+00:00dsymondsurn:md5:cb8efb4b6c60e882dabfcba7176aa144
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.
Message from dvyukov@google.com
2012-11-09T10:57:53+00:00dvyukovurn:md5:5eae7d068cd9b23858e2990ff8920088
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.
>
Message from dsymonds@golang.org
2012-11-09T11:02:48+00:00dsymondsurn:md5:0f3a84861f53bd6345ad7e141f209e8a
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.
Message from dvyukov@google.com
2012-11-09T11:11:02+00:00dvyukovurn:md5:9e4a5e2486fd6767947b2bdc29703a37
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.
Message from rogpeppe@gmail.com
2012-11-12T09:02:05+00:00rogurn:md5:0efa6e651c93aab49f3b8459925bcb94
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) {
>
>
Message from dvyukov@google.com
2012-11-12T09:07:40+00:00dvyukovurn:md5:89d641afbc2ad12f954de8f2dbdc448e
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) {
> >
> >
>
Message from dsymonds@golang.org
2012-11-12T09:27:33+00:00dsymondsurn:md5:e7e07476efbc7ab16ee01f61f9b6e9ad
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.
Message from dsymonds@golang.org
2012-11-12T09:28:20+00:00dsymondsurn:md5:ca42f5e51e044dd19c66492483474492
*** Abandoned ***
Message from rogpeppe@gmail.com
2012-11-12T10:58:28+00:00rogurn:md5:44f6865d15596427ef6434bdefc5bbfb
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) {
>> >
>> >
>
>