code review 5503063: exec: disable the ExtraFiles test on darwin

exec: disable the ExtraFiles test on darwin

Still a mystery. New issue 2603 filed.

[bradfitz, 2011-12-22 00:42:29]

Hello golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com),

I'd like you to review this change to
https://go.googlecode.com/hg/

[dsymonds, 2011-12-22 00:50:17]

LGTM

[iant2, 2011-12-22 00:57:43]

LGTM

bradfitz@golang.org writes:

> Reviewers: golang-dev_googlegroups.com,
>
> Message:
> Hello golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com),
>
> I'd like you to review this change to
> https://go.googlecode.com/hg/
>
>
> Description:
> exec: disable the ExtraFiles test on darwin
>
> Still a mystery. New issue 2603 filed.
>
> Please review this at http://codereview.appspot.com/5503063/
>
> 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
> @@ -67,6 +67,9 @@
>  	// ExtraFiles specifies additional open files to be inherited by the
>  	// new process. It does not include standard input, standard output, or
>  	// standard error. If non-nil, entry i becomes file descriptor 3+i.
> +	//
> +	// BUG: on OS X 10.6, child processes may sometimes inherit extra fds.
> +	// http://golang.org/issue/2603
>  	ExtraFiles []*os.File
>
>  	// SysProcAttr holds optional, operating system-specific attributes.
> Index: src/pkg/os/exec/exec_test.go
> ===================================================================
> --- a/src/pkg/os/exec/exec_test.go
> +++ b/src/pkg/os/exec/exec_test.go
> @@ -11,6 +11,8 @@
>  	"io"
>  	"io/ioutil"
>  	"net"
> +	"net/http"
> +	"net/http/httptest"
>  	"os"
>  	"runtime"
>  	"strconv"
> @@ -156,6 +158,14 @@
>  	}
>  	defer ln.Close()
>
> +	// Force TLS root certs to be loaded (which might involve
> +	// cgo), to make sure none of that potential C code leaks fds.
> +	ts := httptest.NewTLSServer(http.HandlerFunc(func(w
> http.ResponseWriter, r *http.Request) {
> +		w.Write([]byte("Hello"))
> +	}))
> +	defer ts.Close()
> +	http.Get(ts.URL) // ignore result; just calling to force root cert loading
> +
>  	tf, err := ioutil.TempFile("", "")
>  	if err != nil {
>  		t.Fatalf("TempFile: %v", err)
> @@ -256,23 +266,31 @@
>  			fmt.Printf("ReadAll from fd 3: %v", err)
>  			os.Exit(1)
>  		}
> -		// Now verify that there are no other open fds.
> -		var files []*os.File
> -		for wantfd := os.Stderr.Fd() + 2; wantfd <= 100; wantfd++ {
> -			f, err := os.Open(os.Args[0])
> -			if err != nil {
> -				fmt.Printf("error opening file with expected fd %d: %v", wantfd, err)
> -				os.Exit(1)
> +		switch runtime.GOOS {
> +		case "darwin":
> +			// TODO(bradfitz): broken? Sometimes.
> +			// http://golang.org/issue/2603
> +			// Skip this additional part of the test for now.
> +		default:
> +			// Now verify that there are no other open fds.
> +			var files []*os.File
> +			for wantfd := os.Stderr.Fd() + 2; wantfd <= 100; wantfd++ {
> +				f, err := os.Open(os.Args[0])
> +				if err != nil {
> +					fmt.Printf("error opening file with expected fd %d: %v", wantfd, err)
> +					os.Exit(1)
> +				}
> +				if got := f.Fd(); got != wantfd {
> +					fmt.Printf("leaked parent file. fd = %d; want %d\n", got, wantfd)
> +					out, _ := Command("lsof",
> "-p", fmt.Sprint(os.Getpid())).CombinedOutput()
> +					fmt.Print(string(out))
> +					os.Exit(1)
> +				}
> +				files = append(files, f)
>  			}
> -			if got := f.Fd(); got != wantfd {
> -				fmt.Printf("leaked parent file. fd = %d; want %d", got, wantfd)
> -				fmt.Println(Command("lsof", "-p",
> fmt.Sprint(os.Getpid())).CombinedOutput())
> -				os.Exit(1)
> +			for _, f := range files {
> +				f.Close()
>  			}
> -			files = append(files, f)
> -		}
> -		for _, f := range files {
> -			f.Close()
>  		}
>  		os.Stderr.Write(bs)
>  	case "exit":

[bradfitz, 2011-12-22 01:00:52]

I'll submit this, but the caveat documented in Issue 2603 is that this
additional TLS check I added (while a valid fear, and useful to keep in the
long run), doesn't actually trigger a failure on OS X 10.6 or 10.7.

So something's still mysterious.


On Wed, Dec 21, 2011 at 4:57 PM, Ian Lance Taylor <iant@google.com> wrote:

> LGTM
>
> bradfitz@golang.org writes:
>
> > Reviewers: golang-dev_googlegroups.com,
> >
> > Message:
> > Hello golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com),
> >
> > I'd like you to review this change to
> > https://go.googlecode.com/hg/
> >
> >
> > Description:
> > exec: disable the ExtraFiles test on darwin
> >
> > Still a mystery. New issue 2603 filed.
> >
> > Please review this at http://codereview.appspot.com/5503063/
> >
> > 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
> > @@ -67,6 +67,9 @@
> >       // ExtraFiles specifies additional open files to be inherited by
> the
> >       // new process. It does not include standard input, standard
> output, or
> >       // standard error. If non-nil, entry i becomes file descriptor 3+i.
> > +     //
> > +     // BUG: on OS X 10.6, child processes may sometimes inherit extra
> fds.
> > +     // http://golang.org/issue/2603
> >       ExtraFiles []*os.File
> >
> >       // SysProcAttr holds optional, operating system-specific
> attributes.
> > Index: src/pkg/os/exec/exec_test.go
> > ===================================================================
> > --- a/src/pkg/os/exec/exec_test.go
> > +++ b/src/pkg/os/exec/exec_test.go
> > @@ -11,6 +11,8 @@
> >       "io"
> >       "io/ioutil"
> >       "net"
> > +     "net/http"
> > +     "net/http/httptest"
> >       "os"
> >       "runtime"
> >       "strconv"
> > @@ -156,6 +158,14 @@
> >       }
> >       defer ln.Close()
> >
> > +     // Force TLS root certs to be loaded (which might involve
> > +     // cgo), to make sure none of that potential C code leaks fds.
> > +     ts := httptest.NewTLSServer(http.HandlerFunc(func(w
> > http.ResponseWriter, r *http.Request) {
> > +             w.Write([]byte("Hello"))
> > +     }))
> > +     defer ts.Close()
> > +     http.Get(ts.URL) // ignore result; just calling to force root cert
> loading
> > +
> >       tf, err := ioutil.TempFile("", "")
> >       if err != nil {
> >               t.Fatalf("TempFile: %v", err)
> > @@ -256,23 +266,31 @@
> >                       fmt.Printf("ReadAll from fd 3: %v", err)
> >                       os.Exit(1)
> >               }
> > -             // Now verify that there are no other open fds.
> > -             var files []*os.File
> > -             for wantfd := os.Stderr.Fd() + 2; wantfd <= 100; wantfd++ {
> > -                     f, err := os.Open(os.Args[0])
> > -                     if err != nil {
> > -                             fmt.Printf("error opening file with
> expected fd %d: %v", wantfd, err)
> > -                             os.Exit(1)
> > +             switch runtime.GOOS {
> > +             case "darwin":
> > +                     // TODO(bradfitz): broken? Sometimes.
> > +                     // http://golang.org/issue/2603
> > +                     // Skip this additional part of the test for now.
> > +             default:
> > +                     // Now verify that there are no other open fds.
> > +                     var files []*os.File
> > +                     for wantfd := os.Stderr.Fd() + 2; wantfd <= 100;
> wantfd++ {
> > +                             f, err := os.Open(os.Args[0])
> > +                             if err != nil {
> > +                                     fmt.Printf("error opening file
> with expected fd %d: %v", wantfd, err)
> > +                                     os.Exit(1)
> > +                             }
> > +                             if got := f.Fd(); got != wantfd {
> > +                                     fmt.Printf("leaked parent file. fd
> = %d; want %d\n", got, wantfd)
> > +                                     out, _ := Command("lsof",
> > "-p", fmt.Sprint(os.Getpid())).CombinedOutput()
> > +                                     fmt.Print(string(out))
> > +                                     os.Exit(1)
> > +                             }
> > +                             files = append(files, f)
> >                       }
> > -                     if got := f.Fd(); got != wantfd {
> > -                             fmt.Printf("leaked parent file. fd = %d;
> want %d", got, wantfd)
> > -                             fmt.Println(Command("lsof", "-p",
> > fmt.Sprint(os.Getpid())).CombinedOutput())
> > -                             os.Exit(1)
> > +                     for _, f := range files {
> > +                             f.Close()
> >                       }
> > -                     files = append(files, f)
> > -             }
> > -             for _, f := range files {
> > -                     f.Close()
> >               }
> >               os.Stderr.Write(bs)
> >       case "exit":
>

[bradfitz, 2011-12-22 01:08:21]

*** Submitted as http://code.google.com/p/go/source/detail?r=0e4a52de4c96 ***

exec: disable the ExtraFiles test on darwin

Still a mystery. New issue 2603 filed.

R=golang-dev, dsymonds, iant
CC=golang-dev
http://codereview.appspot.com/5503063