Add RetryOnEINTR() helpers and handle more cases that can return EINTR #4680
Conversation
libcontainer/console_linux.go
Outdated
| fd, err := utils.RetryOnEINTR2(func() (int, error) { | ||
| return unix.Open(slavePath, unix.O_RDWR, 0) | ||
| }) | ||
| if err != nil { | ||
| return &os.PathError{ | ||
| Op: "open", |
There was a problem hiding this comment.
So, I would go one step further and create libcontainer/internal/unix (or list internal/unix) and implement open like this:
func OpenAt(dirfd int, path string, flags int, mode uint32) (fd int, err error) {
fd, err := retryOnEINTR2(unix.Openat(dirfd, path, flags, mode))
if err != nil {
op := "open"
if dirfd != unix.AT_FDCWD {
op = "openat"
path = filepath.Join("/proc/self/fd/", strconv.Itoa(fd), path)
}
return -1, &os.PathError{Op: op, Path: path, Err: err}
}
return fd, nil
}
func Open(path string, flags int, mode uint32) (fd int, err error) {
return OpenAt(unix.AT_FDCWD, path, flags, mode)
}and then use these everywhere.
Or, just switch the code that uses unix.Open to os.Open and f.Fd() (and only add/use OpenAt).
There was a problem hiding this comment.
And for openat, we can actually explore using os.Root (https://pkg.go.dev/os@master#Root) and its Open (which properly uses openat(2) under the hood) -- but this needs to wait as it is only there since Go 1.24.
I mean, except for cases when we have openat2 with fancy flags.
There was a problem hiding this comment.
I tried to do this. But we already have an openAt implementation in utils:
runc/libcontainer/utils/utils_unix.go
Line 359 in 480e7a7
And it's different, see that line that adds the close on exec, for example. It also returns an os.File instead of an fd, as the function you put here does.
IMHO it seems better in this PR to keep it about handling EINTR properly and we can do another PR later that tries to use os.Open or even unify to use an internal implementation of Open/OpenAt that handles retries, maybe adds close on exec, etc.
What do you think?
kolyshkin
left a comment
kolyshkin
left a comment
There was a problem hiding this comment.
So, in general, I think it would be better to create our own wrappers for functions that we think should handle retry on EINTR, to keep the retry internal (and the code easier).
kolyshkin
left a comment
kolyshkin
left a comment
There was a problem hiding this comment.
Also, it's beneficial to move some stuff that you touch from libcontainer/system and libcontainer/utils to libcontainer/internal/whatever (and keep a backward-compatible alias/wrapper marked as deprecated for some time).
kolyshkin
left a comment
kolyshkin
left a comment
There was a problem hiding this comment.
Meant to "request changes" but I guess my hand slipped :)
|
Thanks! I fixed some issues, will ping for more reviews when I fix them all |
Adding them to internal to not expose new functions in libct. We can tackle moving more code to internal in other PRs, where we discuss exactly for which functions we want to provide deprecated wrappers and for which ones we don't. Signed-off-by: Rodrigo Campos <rodrigoca@microsoft.com>
unix.Open() can be interrupted, so let's retry on EINTR. In future PRs I'd explore using os.OpenFile and friends, but let's add a retry for now because: * os.OpenFile() means the GC can close the fd and that can cause a regression * We don't have many tests for some of these calls * If we cause a regression and we have to revert, we should revert to an option that retries on EINTR. For those reasons, let's just add a retry on EINTR for now. Signed-off-by: Rodrigo Campos <rodrigoca@microsoft.com>
|
Converted to draft; @rata please change it back to non-draft once it's ready for review. |
Signed-off-by: Rodrigo Campos <rodrigoca@microsoft.com>
Let's move the existing code to the new helper. There are two places that are not moved, because they currently use an unbounded for loop for other reasons and the code seems cleaner keeping it as it is. Signed-off-by: Rodrigo Campos <rodrigoca@microsoft.com>
|
@kolyshkin PTAL |
| // RetryOnEINTR takes a function that returns an error and calls it until it the error returned is | ||
| // not EINTR. | ||
| func RetryOnEINTR(fn func() error) error { | ||
| var err error |
There was a problem hiding this comment.
nit: no need to instantiate this, you can use err := inside the loop, or have a named return variable.
|
Closing in favor of #4697 |
3 participants
This PR adds two helpers:
RetryOnEINTR(): This helper takes a function that returns an error and retries until that error is not EINTRRetryOnEINTR2(): This helper returns an int and an error. This is useful for functions like unix.Open(). While we could handle them with the previous helper using go closures, when coding that it doesn't look nice: we need to declar the vars before and it gets kind of long. With this helper, we can just use the synta:fd, err := utils.RetryOnEINTR2()...and declare the vars on assignment, which is much nicer.Then, the rest of the commits use them for unix.Open and unix.Dup*(), that were not properly handled and switches the existing code with open-coded versions to use the helpers.
Please note the comment on the commit about unix.Open on why I didn't try to switch some cases to os.OpenFile(). I'd like to tackle that later, on another PR.