os/exec: occasional "leaked parent file" on illumos/amd64 #42431
Comments
|
Change https://golang.org/cl/268019 mentions this issue: |
dmitshur
added
the
NeedsFix
On illumos (and Solaris) systems, the native "pfiles" tool provides the
best information about open file descriptors for a process:
https://illumos.org/man/1/pfiles
Use that instead of "lsof" when debugging file descriptor leaks.
Updates #42431.
Change-Id: If1250c4e6c9e8adbd076495a09fb1ce63abcc68b
Reviewed-on: https://go-review.googlesource.com/c/go/+/268019
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
Trust: Dmitri Shuralyov <dmitshur@golang.org>
|
Now that we're using There's nothing currently at fd 4, but presumably there was when we tried to open the test file. This doesn't seem to happen on most test runs, which implies a race of some kind. We should look at running the test under tracing to see if there is another thread opening and closing a file descriptor early in the process lifetime. |
bcmills
added
OS-illumos
NeedsInvestigation
gopherbot
removed
the
NeedsFix
|
On GNU/Linux we had a similar problem that turned out to be because the C library would sometimes open Is it possible to write something similar to https://golang.org/cl/225799 for Illumos, to help identify what file is being opened? |
|
Because this seemed like a race, I decided to start by just investigating Once I had a successful build of the latest revision, I prebuilt the test and ran it: Next, looking at the test, it seems like the interesting failure occurs in the Then, because this script needs to grab the specific child process in order to resolve symbols, I wrote another script, I was then able to trace the test process: Note that I'm not that familiar with the internals of the Go runtime, or why it is creating a new thread here. Merely running the test (not under tracing) in a loop is enough to hit the race with relative frequency: From some limited experiments, it seems that setting |
|
Indeed, this seems to help: diff --git a/src/os/exec/exec_test.go b/src/os/exec/exec_test.go
index cd3d759ebc..2ad82033cb 100644
--- a/src/os/exec/exec_test.go
+++ b/src/os/exec/exec_test.go
@@ -684,20 +684,23 @@ func TestExtraFiles(t *testing.T) {
var cancel context.CancelFunc
ctx, cancel = context.WithDeadline(ctx, deadline)
defer cancel()
}
c = exec.CommandContext(ctx, exe)
var stdout, stderr bytes.Buffer
c.Stdout = &stdout
c.Stderr = &stderr
c.ExtraFiles = []*os.File{tf}
+ if runtime.GOOS == "illumos" {
+ c.Env = append(os.Environ(), "GOMAXPROCS=1")
+ }
err = c.Run()
if err != nil {
t.Fatalf("Run: %v\n--- stdout:\n%s--- stderr:\n%s", err, stdout.Bytes(), stderr.Bytes())
}
if stdout.String() != text {
t.Errorf("got stdout %q, stderr %q; want %q on stdout", stdout.String(), stderr.String(), text)
}
} |
|
I assume that the C library always reads from In https://golang.org/cl/228639 I split the code into a separate program, so that it could run without the C library, and avoid any cases where the C library might open a file. But, of course, on illumos we can't avoid the C library. So I think we should just skip the test entirely on illumos. |
|
Looking into this a little more, it seems like we could probably void making this particular use of proc in the OS because Go is not presently setting thread names. At the moment we seem to be setting the name (to nothing) unconditionally here. (I have filed illumos bug 13312 to cover investigating this). In the interim, though, the diff I suggested above (to use |
|
OK, sure, let's try it. Can you send a change? The code needs an appropriate comment that should mention this issue. Thanks. |
|
Change https://golang.org/cl/273966 mentions this issue: |
Occasionally we seem to see a test failure in the illumos buildlet:
The test in
os/exec/read3.gois trying to executelsofto get debugging information, but unfortunately at least in the distribution that the buildlet is running under that command is not terribly useful. The native tool for inspecting open file descriptors on illumos systems ispfiles, and I see some code in there to handle platform differences already so I will put up a CL to add our distinctiveness alongside. Hopefully it will then be easier to debug subsequent instances of this issue.The text was updated successfully, but these errors were encountered: