Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

os: RemoveAll does not ignore EINTR #57966

Closed
nigeltao opened this issue Jan 23, 2023 · 6 comments
Closed

os: RemoveAll does not ignore EINTR #57966

nigeltao opened this issue Jan 23, 2023 · 6 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@nigeltao
Copy link
Contributor

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
Copy link

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

@robpike
Copy link
Contributor

robpike commented Jan 23, 2023

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

What a mess.

@nigeltao
Copy link
Contributor Author

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
Copy link
Contributor

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.

@bcmills bcmills added this to the Go1.21 milestone Jan 24, 2023
@bcmills bcmills added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jan 24, 2023
@nigeltao
Copy link
Contributor Author

@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
Copy link
Contributor

bcmills commented Jan 25, 2023

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.

@golang golang locked and limited conversation to collaborators Jan 25, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

5 participants