code review 13329043: os/exec: return idempotent Closer from StdinPipe

os/exec: return idempotent Closer from StdinPipe

Before this fix, it was always an error to use the Close method on the
io.WriteCloser obtained from Cmd.StdinPipe, as it would race with the
Close performed by Cmd.Wait.

Fixes issue 6270.

[adg, 2013-08-28 05:40:43]

Hello golang-dev@googlegroups.com,

I'd like you to review this change to
https://code.google.com/p/go

[adg, 2013-08-28 05:43:02]

Confirmed that the new test triggers a race condition with "go test -race"
before the fix, and that the race is gone after.

[r, 2013-08-28 05:58:21]

LGTM but wait for bradfitz.

https://codereview.appspot.com/13329043/diff/6001/src/pkg/os/exec/exec.go
File src/pkg/os/exec/exec.go (right):

https://codereview.appspot.com/13329043/diff/6001/src/pkg/os/exec/exec.go#new...
src/pkg/os/exec/exec.go:16: "sync"
this is a new fundamental dependency. is it ok? (i think it is)

confirmed: yes it is. sync is in L0, exec is not leveled.

https://codereview.appspot.com/13329043/diff/6001/src/pkg/os/exec/exec.go#new...
src/pkg/os/exec/exec.go:360: // standard input when the command starts.
// If the returned WriteCloser is not closed before Wait is called, Wait will
close it.

[r, 2013-08-28 05:58:53]

Fixes issue 6270.

[remyoudompheng, 2013-08-28 06:07:41]

Le 28 août 2013 07:40, <adg@golang.org> a écrit :
>
> Reviewers: golang-dev1,
>
> Message:
> Hello golang-dev@googlegroups.com,
>
> I'd like you to review this change to
> https://code.google.com/p/go
>
>
> Description:
> os/exec: return idempotent Closer from StdinPipe
>
> Before this fix, it was always an error to use the Close method on the
> io.WriteCloser obtained from Cmd.StdinPipe, as it would race with the
> Close performed by Cmd.Wait.
>
> Please review this at https://codereview.appspot.com/13329043/
>
> Affected files:
>   M src/pkg/os/exec/exec.go
>   M src/pkg/os/exec/exec_test.go
>
>
> Index: src/pkg/os/exec/exec.go
> ===================================================================
> --- a/src/pkg/os/exec/exec.go
> +++ b/src/pkg/os/exec/exec.go
> @@ -13,6 +13,7 @@
>         "io"
>         "os"
>         "strconv"
> +       "sync"
>         "syscall"
>  )
>
> @@ -370,8 +371,21 @@
>         }
>         c.Stdin = pr
>         c.closeAfterStart = append(c.closeAfterStart, pr)
> -       c.closeAfterWait = append(c.closeAfterWait, pw)
> -       return pw, nil
> +       wc := &closeOnce{WriteCloser: pw}
> +       c.closeAfterWait = append(c.closeAfterWait, wc)
> +       return wc, nil
> +}
> +
> +type closeOnce struct {
> +       io.WriteCloser
> +       close sync.Once
> +}
> +
> +func (c *closeOnce) Close() (err error) {
> +       c.close.Do(func() {
> +               err = c.WriteCloser.Close()
> +       })
> +       return
>  }
>

Only one caller can get the error if you do that. Is it the intended
behaviour?

Rémy.

[r, 2013-08-28 06:13:05]

https://codereview.appspot.com/13329043/diff/6001/src/pkg/os/exec/exec.go
File src/pkg/os/exec/exec.go (right):

https://codereview.appspot.com/13329043/diff/6001/src/pkg/os/exec/exec.go#new...
src/pkg/os/exec/exec.go:387: })
rémy points out this means only the first caller gets the error.

errors on close are totally stupid of course (what am i supposed to do? close it
again?) but it's easy to get it right, so get it right.

[adg, 2013-08-28 06:59:07]

PTAL

https://codereview.appspot.com/13329043/diff/6001/src/pkg/os/exec/exec.go
File src/pkg/os/exec/exec.go (right):

https://codereview.appspot.com/13329043/diff/6001/src/pkg/os/exec/exec.go#new...
src/pkg/os/exec/exec.go:16: "sync"
Ack.

https://codereview.appspot.com/13329043/diff/6001/src/pkg/os/exec/exec.go#new...
src/pkg/os/exec/exec.go:387: })
On 2013/08/28 06:13:06, r wrote:
> rémy points out this means only the first caller gets the error.
> 
> errors on close are totally stupid of course (what am i supposed to do? close
it
> again?) but it's easy to get it right, so get it right.
> 

Done.

[r, 2013-08-28 07:12:30]

LGTM but wait for badboyfitz

[bradfitz, 2013-08-28 16:23:31]

https://codereview.appspot.com/13329043/diff/16001/src/pkg/os/exec/exec.go
File src/pkg/os/exec/exec.go (right):

https://codereview.appspot.com/13329043/diff/16001/src/pkg/os/exec/exec.go#ne...
src/pkg/os/exec/exec.go:376: wc := &closeOnce{WriteCloser: pw}
This was proposed and rejected previously because it changes the underlying type
of the io.WriteCloser from an *os.File (from which you can get its Fd).

Are we okay with that change?  It might break programs.  That was the concern
before.

https://codereview.appspot.com/13329043/diff/16001/src/pkg/os/exec/exec.go#ne...
src/pkg/os/exec/exec.go:387: func (c *closeOnce) Close() (err error) {
not sure why this is named result

https://codereview.appspot.com/13329043/diff/16001/src/pkg/os/exec/exec.go#ne...
src/pkg/os/exec/exec.go:388: c.close.Do(func() {
if you use a method value here, I believe you save an allocation.

func (c *closeOnce) Close() error {
  c.close.Do(c.close)
  return c.closeErr
}

func (c *closeOnce) close() {
 c.closeErr = c.WriteCloser.Close()
}

[r, 2013-08-28 22:21:53]

Would it mitigate enough if we embed the *os.File?

-rob

[adg, 2013-08-28 22:50:44]

If *os.File were an interface, yes. Unfortunately it is not and that
thwarts this approach I believe. Perhaps it needs to be a new method.
On 29 Aug 2013 08:21, "Rob Pike" <r@golang.org> wrote:

> Would it mitigate enough if we embed the *os.File?
>
> -rob
>

[bradfitz, 2013-08-28 22:51:24]

On Wed, Aug 28, 2013 at 3:21 PM, Rob Pike <r@golang.org> wrote:

> Would it mitigate enough if we embed the *os.File?
>

Not my call.  It would permit access to the Fd() uintptr and all the other
methods, though.

But I imagine most code does:

foo.(*os.File).Fd()

or

anyway, and not gracefully testing for a specific method.

[r, 2013-08-28 22:58:12]

Perhaps we go back to plan B then, and introduce new methods that work
and discourage the old.

-rob

[dsymonds, 2013-08-29 02:39:21]

The whole point of StdinPipe returning interfaces is that it *doesn't*
promise that it's an *os.File. It doesn't document that it's an
*os.File. If people are making such assumptions then I don't feel bad
if their code breaks. It'll at least break in a very obvious way.

I'm pretty sure we've made other similar changes elsewhere in the
standard library since Go 1.0.

[r, 2013-08-29 03:13:40]

All right, you've convinced me. It's an io.WriteCloser. If someone
asserts that to something else, they're trusting promises that aren't
made.

Let's go with what's here.

-rob

[r, 2013-08-29 03:15:17]

https://codereview.appspot.com/13329043/diff/16001/src/pkg/os/exec/exec.go
File src/pkg/os/exec/exec.go (right):

https://codereview.appspot.com/13329043/diff/16001/src/pkg/os/exec/exec.go#ne...
src/pkg/os/exec/exec.go:388: c.close.Do(func() {
a method value is an allocation too.

i don't care about one allocation like this, and i think it's clearer as
written, but if you want it as a method value that's fine with me.

[bradfitz, 2013-08-29 03:24:00]

I'm fine changing it too.

But I still think it'd be prudent to embed the *os.File so we have an
answer to breakage questions that works with Go 1, 1.1, or 1.2.


On Wed, Aug 28, 2013 at 8:13 PM, Rob Pike <r@golang.org> wrote:

> All right, you've convinced me. It's an io.WriteCloser. If someone
> asserts that to something else, they're trusting promises that aren't
> made.
>
> Let's go with what's here.
>
> -rob
>

[r, 2013-08-29 03:45:45]

SGTM

[adg, 2013-08-29 04:26:47]

PTAL

Now with embedded *os.File.

https://codereview.appspot.com/13329043/diff/16001/src/pkg/os/exec/exec.go
File src/pkg/os/exec/exec.go (right):

https://codereview.appspot.com/13329043/diff/16001/src/pkg/os/exec/exec.go#ne...
src/pkg/os/exec/exec.go:387: func (c *closeOnce) Close() (err error) {
On 2013/08/28 16:23:31, bradfitz wrote:
> not sure why this is named result

It needn't be; a holdover from a previous revision

https://codereview.appspot.com/13329043/diff/16001/src/pkg/os/exec/exec.go#ne...
src/pkg/os/exec/exec.go:388: c.close.Do(func() {
On 2013/08/29 03:15:18, r wrote:
> a method value is an allocation too.
> 
> i don't care about one allocation like this, and i think it's clearer as
> written, but if you want it as a method value that's fine with me.

I would prefer to leave it as written.

[r, 2013-08-29 04:31:11]

LGTM but again wait for the fitzoid

[bradfitz, 2013-08-29 04:31:39]

Document (with issue link) and test the embedding?
 On Aug 28, 2013 9:26 PM, <adg@golang.org> wrote:

> PTAL
>
> Now with embedded *os.File.
>
>
> https://codereview.appspot.**com/13329043/diff/16001/src/**
>
pkg/os/exec/exec.go<https://codereview.appspot.com/13329043/diff/16001/src/pkg/os/exec/exec.go>
> File src/pkg/os/exec/exec.go (right):
>
> https://codereview.appspot.**com/13329043/diff/16001/src/**
>
pkg/os/exec/exec.go#newcode387<https://codereview.appspot.com/13329043/diff/16001/src/pkg/os/exec/exec.go#newcode387>
> src/pkg/os/exec/exec.go:387: func (c *closeOnce) Close() (err error) {
> On 2013/08/28 16:23:31, bradfitz wrote:
>
>> not sure why this is named result
>>
>
> It needn't be; a holdover from a previous revision
>
> https://codereview.appspot.**com/13329043/diff/16001/src/**
>
pkg/os/exec/exec.go#newcode388<https://codereview.appspot.com/13329043/diff/16001/src/pkg/os/exec/exec.go#newcode388>
> src/pkg/os/exec/exec.go:388: c.close.Do(func() {
> On 2013/08/29 03:15:18, r wrote:
>
>> a method value is an allocation too.
>>
>
>  i don't care about one allocation like this, and i think it's clearer
>>
> as
>
>> written, but if you want it as a method value that's fine with me.
>>
>
> I would prefer to leave it as written.
>
>
https://codereview.appspot.**com/13329043/<https://codereview.appspot.com/133...
>

[dsymonds, 2013-08-29 04:34:14]

LGTM modulo the things raised by the fitzoid.

[bradfitz, 2013-08-29 04:37:12]

LGTM after test/comment addition.

I'm off now. Don't wait for me to submit.
 On Aug 28, 2013 9:31 PM, "Brad Fitzpatrick" <bradfitz@golang.org> wrote:

> Document (with issue link) and test the embedding?
>  On Aug 28, 2013 9:26 PM, <adg@golang.org> wrote:
>
>> PTAL
>>
>> Now with embedded *os.File.
>>
>>
>> https://codereview.appspot.**com/13329043/diff/16001/src/**
>>
pkg/os/exec/exec.go<https://codereview.appspot.com/13329043/diff/16001/src/pkg/os/exec/exec.go>
>> File src/pkg/os/exec/exec.go (right):
>>
>> https://codereview.appspot.**com/13329043/diff/16001/src/**
>>
pkg/os/exec/exec.go#newcode387<https://codereview.appspot.com/13329043/diff/16001/src/pkg/os/exec/exec.go#newcode387>
>> src/pkg/os/exec/exec.go:387: func (c *closeOnce) Close() (err error) {
>> On 2013/08/28 16:23:31, bradfitz wrote:
>>
>>> not sure why this is named result
>>>
>>
>> It needn't be; a holdover from a previous revision
>>
>> https://codereview.appspot.**com/13329043/diff/16001/src/**
>>
pkg/os/exec/exec.go#newcode388<https://codereview.appspot.com/13329043/diff/16001/src/pkg/os/exec/exec.go#newcode388>
>> src/pkg/os/exec/exec.go:388: c.close.Do(func() {
>> On 2013/08/29 03:15:18, r wrote:
>>
>>> a method value is an allocation too.
>>>
>>
>>  i don't care about one allocation like this, and i think it's clearer
>>>
>> as
>>
>>> written, but if you want it as a method value that's fine with me.
>>>
>>
>> I would prefer to leave it as written.
>>
>>
https://codereview.appspot.**com/13329043/<https://codereview.appspot.com/133...
>>
>

[adg, 2013-08-29 04:41:51]

*** Submitted as https://code.google.com/p/go/source/detail?r=ef345053cf89 ***

os/exec: return idempotent Closer from StdinPipe

Before this fix, it was always an error to use the Close method on the
io.WriteCloser obtained from Cmd.StdinPipe, as it would race with the
Close performed by Cmd.Wait.

Fixes issue 6270.

R=golang-dev, r, remyoudompheng, bradfitz, dsymonds
CC=golang-dev
https://codereview.appspot.com/13329043