Alexey Serbin has submitted this change and it was merged. (
http://gerrit.cloudera.org:8080/22521 )
Change subject: KUDU-3645 do not retry close() syscall on EINTR
......................................................................
KUDU-3645 do not retry close() syscall on EINTR
It's not a good idea to retry calling close() on EINTR, at least
on Linux and other Unices except for HP-UX. Since Kudu isn't officially
supported on HP-UX, this patch removes the retry of close() from all the
applicable call sites where it was blanket-wrapped with the
RETRY_ON_EINTR macro.
The same issue has been addressed in the Boost library as well [1][2].
It's a well known issue and it's been discussed a long time ago [3].
Also, it's well documented in the manual page [4]:
The EINTR error is a somewhat special case. Regarding the EINTR
error, POSIX.1-2008 says:
If close() is interrupted by a signal that is to be caught,
it shall return -1 with errno set to EINTR and the state of
fildes is unspecified.
This permits the behavior that occurs on Linux and many other
implementations, where, as with other errors that may be reported
by close(), the file descriptor is guaranteed to be closed.
However, it also permits another possibility: that the
implementation returns an EINTR error and keeps the file
descriptor open. (According to its documentation, HP-UX's close()
does this.) The caller must then once more use close() to close
the file descriptor, to avoid file descriptor leaks. This
divergence in implementation behaviors provides a difficult hurdle
for portable applications, since on many implementations, close()
must not be called again after an EINTR error, and on at least
one, close() must be called again. There are plans to address
this conundrum for the next major release of the POSIX.1 standard.
[1] https://github.com/boostorg/beast/issues/1445
[2] https://github.com/boostorg/beast/commit/0ce8ebbef
[3] https://lwn.net/Articles/576478/
[4] https://man7.org/linux/man-pages/man2/close.2.html
Change-Id: I938e487bd937ef6ee8764b9aa88cb5f7a2c33158
Reviewed-on: http://gerrit.cloudera.org:8080/22521
Tested-by: Alexey Serbin <[email protected]>
Reviewed-by: Abhishek Chennaka <[email protected]>
---
M src/kudu/gutil/sysinfo.cc
M src/kudu/subprocess/subprocess_protocol.cc
M src/kudu/util/env-test.cc
M src/kudu/util/env_posix.cc
M src/kudu/util/net/diagnostic_socket.cc
M src/kudu/util/net/socket.cc
M src/kudu/util/pstack_watcher-test.cc
M src/kudu/util/pstack_watcher.cc
M src/kudu/util/subprocess-test.cc
M src/kudu/util/subprocess.cc
10 files changed, 139 insertions(+), 104 deletions(-)
Approvals:
Alexey Serbin: Verified
Abhishek Chennaka: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/22521
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I938e487bd937ef6ee8764b9aa88cb5f7a2c33158
Gerrit-Change-Number: 22521
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin <[email protected]>
Gerrit-Reviewer: Abhishek Chennaka <[email protected]>
Gerrit-Reviewer: Alexey Serbin <[email protected]>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <[email protected]>
Gerrit-Reviewer: Yifan Zhang <[email protected]>
