os: RemoveAll does not ignore EINTR

[nigeltao]

The os.Remove function ignores EINTR when calling syscall.Unlink, as of cl/249178 (August 2020).

The os.RemoveAll function does not when calling unix.Unlinkat but should, to be consistent.

---

As the cl/249178 commit message says:

When using a FUSE file system, any system call that touches the file system can return EINTR.

and we are indeed observing os.RemoveAll returning EINTR on a FUSE file system.

[gopherbot]

Change https://go.dev/cl/463076 mentions this issue: os: have RemoveAll loop on EINTR

[robpike]

Why should it? What if I hit ^C to stop it removing everything?

What a mess.

[nigeltao]

It is a mess, but os.Remove and os.RemoveAll should be consistent, whether it's they both retry on EINTR or they both give up (and it's up to the caller to decide). The status quo today is confusing in that some but not all os.Foo calls need explicit EINTR handling.

If you want to argue for rolling back https://go-review.googlesource.com/c/go/+/249178 then I think that's a separate, bigger discussion.

[ianlancetaylor]

It is a mess but hitting ^C will still stop the program.

The semantics of ignoring EINTR are consistent with the fact that we install all of our signal handlers with SA_RESTART. It just turns out that SA_RESTART is insufficient in practice. Quoting the docs for the internal ignoringEINTR function:

// ignoringEINTR makes a function call and repeats it if it returns an
// EINTR error. This appears to be required even though we install all
// signal handlers with SA_RESTART: see #22838, #38033, #38836, #40846.
// Also #20400 and #36644 are issues in which a signal handler is
// installed without setting SA_RESTART. None of these are the common case,
// but there are enough of them that it seems that we can't avoid
// an EINTR loop.

Ignoring EINTR and retrying the system call is simply the right thing to do in a multi-threaded program. Otherwise, for example, profiling, which depends on the SIGPROF signal, makes your program unreliable. Similarly for preempting long-running threads, as the scheduler does today.

[nigeltao]

@bcmills https://go.dev/cl/463076 has your CR+1 and is Ready to Submit but you've also added the NeedsInvestigation label to this issue.

Should I go ahead and submit cl/463076 or are we waiting for further discussion?

[bcmills]

As far as I'm concerned it's fine to go ahead and fix. I just wasn't sure if the discussion on this issue was concluded.