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

Issue 6821099: code review 6821099: os/exec: add test that fails under race detector. (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
11 years, 4 months ago by dsymonds
Modified:
11 years, 4 months ago
Reviewers:
rog, dvyukov, bradfitz
CC:
golang-dev
Visibility:
Public.

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.

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 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+38 lines, -0 lines) Patch
M src/pkg/os/exec/exec_test.go View 1 1 chunk +38 lines, -0 lines 0 comments Download

Messages

Total messages: 12
dsymonds
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
11 years, 4 months ago (2012-11-09 10:17:58 UTC) #1
dvyukov
Perhaps you need to wait for the output reading goroutine to finish first, and only ...
11 years, 4 months ago (2012-11-09 10:22:08 UTC) #2
dvyukov
On 2012/11/09 10:22:08, dvyukov wrote: > Perhaps you need to wait for the output reading ...
11 years, 4 months ago (2012-11-09 10:24:07 UTC) #3
dsymonds
On Fri, Nov 9, 2012 at 9:24 PM, <dvyukov@google.com> wrote: > On 2012/11/09 10:22:08, dvyukov ...
11 years, 4 months ago (2012-11-09 10:48:21 UTC) #4
dvyukov
On Fri, Nov 9, 2012 at 2:48 PM, David Symonds <dsymonds@golang.org> wrote: > On Fri, ...
11 years, 4 months ago (2012-11-09 10:57:53 UTC) #5
dsymonds
On Fri, Nov 9, 2012 at 9:57 PM, Dmitry Vyukov <dvyukov@google.com> wrote: > Do you ...
11 years, 4 months ago (2012-11-09 11:02:48 UTC) #6
dvyukov
On Fri, Nov 9, 2012 at 3:02 PM, David Symonds <dsymonds@golang.org> wrote: > On Fri, ...
11 years, 4 months ago (2012-11-09 11:11:02 UTC) #7
rog
I think this is issue 4290. I have a CL that fixes it: https://codereview.appspot.com/6789043/ (I ...
11 years, 4 months ago (2012-11-12 09:02:05 UTC) #8
dvyukov
On Mon, Nov 12, 2012 at 1:02 PM, roger peppe <rogpeppe@gmail.com> wrote: > I think ...
11 years, 4 months ago (2012-11-12 09:07:40 UTC) #9
dsymonds
On Mon, Nov 12, 2012 at 8:02 PM, roger peppe <rogpeppe@gmail.com> wrote: > I think ...
11 years, 4 months ago (2012-11-12 09:27:33 UTC) #10
dsymonds
*** Abandoned ***
11 years, 4 months ago (2012-11-12 09:28:20 UTC) #11
rog
11 years, 4 months ago (2012-11-12 10:58:28 UTC) #12
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.

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