Retry direct unix package calls if observing EINTR #4637
Conversation
evanphx
force-pushed
the
evanphx/b-eintr
branch
3 times, most recently
from
5cce86e to
ce293b9
Compare
|
@evanphx thanks for the PR to fix this! So, you saw this with runc 1.2, right? I guess we will need a backport also, in that case. |
There was a problem hiding this comment.
LGTM, thanks!
@evanphx can you also open another PR (or add it here, as you prefer) changing other occurences?
In utils/cmsg.go I see unhandled cases for unix.Recvmsg() and unix.Sendmsg(). The manpage says they can return EINTR, and as we are using those through the unix package (the same applies to the syscall package), we need to handle that.
rata
added
the
backport/1.2-todo
rata
dismissed
their stale review
I'll review again when the code is slightly simpler
|
@rata I can go through and add the other checks as well, no trouble. |
evanphx
force-pushed
the
evanphx/b-eintr
branch
2 times, most recently
from
adeef38 to
69f5417
Compare
|
@rata There ya go, let me know how this looks! I added the guard to a call to unix.Read as the man page also says it returns EINTR. |
There was a problem hiding this comment.
Also,
-
You can compare err directly here (as it's done in other places) but will need to add
//nolint:errorlint // unix errors are bareannotation). I ever so slightly prefer a direct comparison as it should be faster, but I'm fine either way. -
While at it, we can take a look at proper error wrapping, too. It seems that in a few cases we may benefit in wrapping an error in an
os.SyscallErr(either directly or usingos.NewSyscallError).
|
@kolyshkin I'll have a look at those! |
Retry Recvfrom, Sendmsg, Readmsg, and Read as they can return EINTR. Signed-off-by: Evan Phoenix <[email protected]>
evanphx
force-pushed
the
evanphx/b-eintr
branch
from
69f5417 to
28475f1
Compare
|
@rata Forgot to mention this earlier, yes, I'm seeing it with 1.2 so we need to backport. |
| @@ -40,7 +41,11 @@ func registerMemoryEventV2(cgDir, evName, cgEvName string) (<-chan struct{}, err | |||
|
|
|||
| for { | |||
| n, err := unix.Read(fd, buffer[:]) | |||
There was a problem hiding this comment.
Overall LGTM, a small suggestion:
Maybe we can use os.File to read data from this fd.
There was a problem hiding this comment.
Oh, nice idea, but fd comes from:
fd, err := unix.InotifyInit()
...
evFd, err := unix.InotifyAddWatch(fd, filepath.Join(cgDir, evName), unix.IN_MODIFY)
....
And os.File closes the underlying fd when go detects is not used anymore. It might cause issues here, in the inotify stuff. So I'd not do it as part of this PR, we can explore in another one if this works or not IMHO.
| @@ -40,7 +41,11 @@ func registerMemoryEventV2(cgDir, evName, cgEvName string) (<-chan struct{}, err | |||
|
|
|||
| for { | |||
| n, err := unix.Read(fd, buffer[:]) | |||
There was a problem hiding this comment.
Oh, nice idea, but fd comes from:
fd, err := unix.InotifyInit()
...
evFd, err := unix.InotifyAddWatch(fd, filepath.Join(cgDir, evName), unix.IN_MODIFY)
....
And os.File closes the underlying fd when go detects is not used anymore. It might cause issues here, in the inotify stuff. So I'd not do it as part of this PR, we can explore in another one if this works or not IMHO.
lifubang
changed the title
It appears this similar pattern is done elsewhere in libcontainer. Unsure if it needs to be do everywhere unix. is used directly or not.
This was observed during a buildkit run, with this output: