Go version

last commit as of now (beaf7f3282c25)

Output of go env in your module/workspace:

n/a

What did you do?

A test case added by recently merged CL 588675 is consistently failing on FreeBSD, OpenBSD, NetBSD.

What did you see happen?

--- FAIL: TestProcessAlreadyDone (0.00s)
    exec_test.go:97: Wait() got err wait6: no child processes (ps <nil>), want ErrProcessDone
FAIL
FAIL    os  0.463s
ok      os/exec 0.282s
FAIL
go tool dist: Failed: exit status 1

What did you expect to see?

passing test

Comment From: gabyhelp

Similar Issues

  • https://github.com/golang/go/issues/44131
  • https://github.com/golang/go/issues/56215
  • https://github.com/golang/go/issues/20835
  • https://github.com/golang/go/issues/48789
  • https://github.com/golang/go/issues/43722

(Emoji vote if this was helpful or unhelpful; more detailed feedback welcome in this discussion.)

Comment From: prattmic

Oh, I missed that Wait doesn't use convertESRCH. It seems like it probably should? Though to be conservative in the freeze, just making the test accept ESRCH as well is probably best.

I think this would fail on Linux without pidfd as well. I guess we don't have any Linux builders with old enough kernels to cover non-pidfd.

Comment From: kolyshkin

@prattmic PTAL. I think that TestProcessAlreadyDone has a wrong assumption that Wait on a non-existent process should return ErrProcessDone, while in fact it does it only on Linux when pidfd is working (because statusDone is set by FindProcess).

Or, if the assumption is right and we want to tidy up the API, we should add code converting ECHILD (not ESRCH) to ErrProcessDone.

BTW there's a test very similar to TestProcessAlreadyDone but it only runs on Linux and only if pidfd is working.

Comment From: kolyshkin

I think this would fail on Linux without pidfd as well. I guess we don't have any Linux builders with old enough kernels to cover non-pidfd.

An idea, perhaps we can test non-pidfd functionality on linux, too; let me take a look.

Comment From: kolyshkin

I guess the easiest path forward is to remove the test case (as it has a wrong assumption for general case, and the specific case (when the assumption is right) is already tested by TestFindProcessViaPidfd.

Comment From: prattmic

Oops, I assumed that text was ESRCH, indeed it is ECHILD.

I think for this release we should change the statusDone case in Wait for pidfd to return ECHILD instead of ErrProcessDone. That makes the PID and pidfd cases return the same error (as-is, returning ErrProcessDone is a slight API change).

For next release, I do think we should consider changing all of the ECHILD cases to ErrProcessDone, as I agree it is cleaner.

Comment From: kolyshkin

think for this release we should change the statusDone case in Wait for pidfd to return ECHILD instead of ErrProcessDone.

...wrapped in NewSyscallError("wait").

Yes you're probably right. Meaning, TestFindProcessViaPidfd needs to be changed or removed as well.

(and I was hoping I just found a way to find out if pidfd is used)

Comment From: prattmic

...wrapped in NewSyscallError("wait").

Thanks, you saved me a round of CL review. :)

(and I was hoping I just found a way to find out if pidfd is used)

It isn't the cleanest, but given the way os is structured now, if you do the equivalent of pidfdWorks, then you know pidfd will always be used.

Comment From: gopherbot

Change https://go.dev/cl/591816 mentions this issue: os: always return syscall.ECHILD from Wait for done process