This is an automated email from the ASF dual-hosted git repository.
alexey pushed a commit to branch branch-1.18.x
in repository https://gitbox.apache.org/repos/asf/kudu.git
The following commit(s) were added to refs/heads/branch-1.18.x by this push:
new b2dc17907 KUDU-3645 do not retry close() syscall on EINTR
b2dc17907 is described below
commit b2dc179072aa50f3f27ea35e6cc123e2f6a03501
Author: Alexey Serbin <[email protected]>
AuthorDate: Fri Feb 21 12:20:41 2025 -0800
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]>
(cherry picked from commit b6e0da9b20e4b96b2be1d34342dd9f7c595d8686)
Reviewed-on: http://gerrit.cloudera.org:8080/22545
---
src/kudu/gutil/sysinfo.cc | 17 +++----
src/kudu/subprocess/subprocess_protocol.cc | 12 +++--
src/kudu/util/env-test.cc | 7 ++-
src/kudu/util/env_posix.cc | 68 +++++++++++++------------
src/kudu/util/net/diagnostic_socket.cc | 17 ++++---
src/kudu/util/net/socket.cc | 18 +++----
src/kudu/util/pstack_watcher-test.cc | 5 +-
src/kudu/util/pstack_watcher.cc | 12 +++--
src/kudu/util/subprocess-test.cc | 7 +--
src/kudu/util/subprocess.cc | 80 +++++++++++++++++++-----------
10 files changed, 139 insertions(+), 104 deletions(-)
diff --git a/src/kudu/gutil/sysinfo.cc b/src/kudu/gutil/sysinfo.cc
index 610c7db93..ce18fbf0d 100644
--- a/src/kudu/gutil/sysinfo.cc
+++ b/src/kudu/gutil/sysinfo.cc
@@ -130,7 +130,9 @@ static bool SlurpSmallTextFile(const char* file, char* buf,
int buflen) {
bool ret = false;
int fd;
RETRY_ON_EINTR(fd, open(file, O_RDONLY));
- if (fd == -1) return ret;
+ if (fd == -1) {
+ return ret;
+ }
memset(buf, '\0', buflen);
int n;
@@ -141,10 +143,8 @@ static bool SlurpSmallTextFile(const char* file, char*
buf, int buflen) {
ret = true;
}
- int close_ret;
- RETRY_ON_EINTR(close_ret, close(fd));
- if (PREDICT_FALSE(close_ret != 0)) {
- PLOG(WARNING) << "Failed to close fd " << fd;
+ if (PREDICT_FALSE(close(fd) != 0)) {
+ PLOG(WARNING) << "error closing " << file;
}
return ret;
@@ -348,10 +348,9 @@ static void InitializeSystemInfo() {
num_cpus++; // count up every time we see an "processor :" entry
}
} while (chars_read > 0);
- int ret;
- RETRY_ON_EINTR(ret, close(fd));
- if (PREDICT_FALSE(ret != 0)) {
- PLOG(WARNING) << "Failed to close fd " << fd;
+
+ if (PREDICT_FALSE(close(fd) != 0)) {
+ PLOG(WARNING) << "error closing " << pname;
}
if (!saw_mhz) {
diff --git a/src/kudu/subprocess/subprocess_protocol.cc
b/src/kudu/subprocess/subprocess_protocol.cc
index cbb863ee7..42667bb3c 100644
--- a/src/kudu/subprocess/subprocess_protocol.cc
+++ b/src/kudu/subprocess/subprocess_protocol.cc
@@ -35,6 +35,7 @@
#include "kudu/gutil/strings/substitute.h"
#include "kudu/subprocess/subprocess.pb.h" // IWYU pragma: keep
#include "kudu/tools/tool.pb.h" // IWYU pragma: keep
+#include "kudu/util/errno.h"
#include "kudu/util/faststring.h"
#include "kudu/util/pb_util.h"
#include "kudu/util/status.h"
@@ -60,9 +61,14 @@ SubprocessProtocol::SubprocessProtocol(SerializationMode
serialization_mode,
SubprocessProtocol::~SubprocessProtocol() {
if (close_mode_ == CloseMode::CLOSE_ON_DESTROY) {
- int ret;
- RETRY_ON_EINTR(ret, close(read_fd_));
- RETRY_ON_EINTR(ret, close(write_fd_));
+ if (PREDICT_FALSE(close(read_fd_) != 0)) {
+ const int err = errno;
+ LOG(WARNING) << Substitute("error closing read fd: $0",
ErrnoToString(err));
+ }
+ if (PREDICT_FALSE(close(write_fd_) != 0)) {
+ const int err = errno;
+ LOG(WARNING) << Substitute("error closing write fd: $0",
ErrnoToString(err));
+ }
}
}
diff --git a/src/kudu/util/env-test.cc b/src/kudu/util/env-test.cc
index f4d6ed96f..c758a277f 100644
--- a/src/kudu/util/env-test.cc
+++ b/src/kudu/util/env-test.cc
@@ -56,11 +56,13 @@
#include "kudu/gutil/macros.h"
#include "kudu/gutil/map-util.h"
+#include "kudu/gutil/port.h"
#include "kudu/gutil/strings/human_readable.h"
#include "kudu/gutil/strings/substitute.h"
#include "kudu/gutil/strings/util.h"
#include "kudu/util/array_view.h" // IWYU pragma: keep
#include "kudu/util/env_util.h"
+#include "kudu/util/errno.h"
#include "kudu/util/faststring.h"
#include "kudu/util/monotime.h"
#include "kudu/util/path_util.h"
@@ -132,7 +134,10 @@ class TestEnv : public KuduTest {
}
}
- RETRY_ON_EINTR(err, close(fd));
+ if (PREDICT_FALSE(close(fd) != 0)) {
+ const int err = errno;
+ LOG(WARNING) << Substitute("error closing fd $0: $1", fd,
ErrnoToString(err));
+ }
#endif
checked = true;
diff --git a/src/kudu/util/env_posix.cc b/src/kudu/util/env_posix.cc
index 853bdd8ac..cb17c50ee 100644
--- a/src/kudu/util/env_posix.cc
+++ b/src/kudu/util/env_posix.cc
@@ -220,7 +220,7 @@ TAG_FLAG(enable_multi_tenancy, experimental);
bool ValidateMultiTenancySettings() {
if (FLAGS_enable_multi_tenancy && !FLAGS_encrypt_data_at_rest) {
- LOG(ERROR) << strings::Substitute(
+ LOG(ERROR) << Substitute(
"The --enable_multi_tenancy can be set 'true' only when
--encrypt_data_at_rest "
"is set 'true'. Current settings are $0 and $1 correspondingly",
FLAGS_enable_multi_tenancy, FLAGS_encrypt_data_at_rest);
@@ -392,10 +392,9 @@ ssize_t pwritevsim(int fd, const struct iovec* iovec, int
count, off_t offset) {
#endif
void DoClose(int fd) {
- int err;
- RETRY_ON_EINTR(err, close(fd));
- if (PREDICT_FALSE(err != 0)) {
- PLOG(WARNING) << "Failed to close fd " << fd;
+ if (PREDICT_FALSE(close(fd) != 0)) {
+ const int err = errno;
+ LOG(WARNING) << Substitute("error closing fd $0: $1", fd,
ErrnoToString(err));
}
}
@@ -988,10 +987,10 @@ class PosixSequentialFile: public SequentialFile {
encryption_header_(eh) {}
~PosixSequentialFile() {
- int err;
- RETRY_ON_EINTR(err, fclose(file_));
- if (PREDICT_FALSE(err != 0)) {
- PLOG(WARNING) << "Failed to close " << filename_;
+ if (PREDICT_FALSE(fclose(file_) != 0)) {
+ const int err = errno;
+ LOG(WARNING) << Substitute("error closing '$0': $1",
+ filename_, ErrnoToString(err));
}
}
@@ -1180,11 +1179,12 @@ class PosixWritableFile : public WritableFile {
}
}
- int ret;
- RETRY_ON_EINTR(ret, close(fd_));
- if (ret < 0) {
+ if (PREDICT_FALSE(close(fd_) != 0)) {
+ const int err = errno;
+ auto err_status = IOError(filename_, err);
+ LOG(WARNING) << Substitute("error closing file: $0",
err_status.ToString());
if (s.ok()) {
- s = IOError(filename_, errno);
+ s = std::move(err_status);
}
}
@@ -1410,11 +1410,12 @@ class PosixRWFile : public RWFile {
}
}
- int ret;
- RETRY_ON_EINTR(ret, close(fd_));
- if (ret < 0) {
+ if (PREDICT_FALSE(close(fd_) != 0)) {
+ const int err = errno;
+ auto err_status = IOError(filename_, err);
+ LOG(WARNING) << Substitute("error closing file: $0",
err_status.ToString());
if (s.ok()) {
- s = IOError(filename_, errno);
+ s = std::move(err_status);
}
}
@@ -2370,10 +2371,9 @@ class PosixEnv : public Env {
struct FtsCloser {
void operator()(FTS *fts) const {
if (fts) {
- int err;
- RETRY_ON_EINTR(err, fts_close(fts));
- if (PREDICT_FALSE(err != 0)) {
- PLOG(WARNING) << "Failed to close fts";
+ if (PREDICT_FALSE(fts_close(fts) != 0)) {
+ const int err = errno;
+ LOG(WARNING) << Substitute("failed to close FTS handle: $0",
ErrnoToString(err));
}
}
}
@@ -2472,23 +2472,29 @@ class PosixEnv : public Env {
}
Status EchoToFile(const char* file_path, const char* data_ptr, int
data_size) override {
+ constexpr const char* const kErrFmt = "error closing file '$0': $1";
int f;
RETRY_ON_EINTR(f, open(file_path, O_WRONLY));
- if (f == -1)
- return IOError(file_path, errno);
+ if (f == -1) {
+ const int err = errno;
+ return IOError(file_path, err);
+ }
ssize_t write_ret;
RETRY_ON_EINTR(write_ret, write(f, data_ptr, data_size));
if (write_ret == -1) {
- // Try to close it anyway, but return the error during write().
- int saved_errno = errno;
- int dont_care;
- RETRY_ON_EINTR(dont_care, close(f));
+ // Try to close it anyway, but return the error happened during write().
+ const int saved_errno = errno;
+ if (PREDICT_FALSE(close(f) != 0)) {
+ int err = errno;
+ LOG(WARNING) << Substitute(kErrFmt, file_path, ErrnoToString(err));
+ }
return IOError(file_path, saved_errno);
}
- int close_ret;
- RETRY_ON_EINTR(close_ret, close(f));
- if (close_ret == -1)
- return IOError(file_path, errno);
+ if (PREDICT_FALSE(close(f) != 0)) {
+ int err = errno;
+ LOG(WARNING) << Substitute(kErrFmt, file_path, ErrnoToString(err));
+ return IOError(file_path, err);
+ }
return Status::OK();
}
diff --git a/src/kudu/util/net/diagnostic_socket.cc
b/src/kudu/util/net/diagnostic_socket.cc
index da75ce062..22d461725 100644
--- a/src/kudu/util/net/diagnostic_socket.cc
+++ b/src/kudu/util/net/diagnostic_socket.cc
@@ -84,17 +84,18 @@ Status DiagnosticSocket::Init() {
}
Status DiagnosticSocket::Close() {
- if (fd_ < 0) {
+ if (PREDICT_FALSE(fd_ < 0)) {
return Status::OK();
}
- int ret;
- RETRY_ON_EINTR(ret, ::close(fd_));
- if (ret < 0) {
- int err = errno;
- return Status::IOError("close error", ErrnoToString(err), err);
- }
+ const int fd = fd_;
fd_ = -1;
- return Status::OK();
+
+ Status s;
+ if (PREDICT_FALSE(::close(fd) != 0)) {
+ const int err = errno;
+ s = Status::IOError("error closing diagnostic socket", ErrnoToString(err),
err);
+ }
+ return s;
}
Status DiagnosticSocket::Query(const Sockaddr& socket_src_addr,
diff --git a/src/kudu/util/net/socket.cc b/src/kudu/util/net/socket.cc
index 7245f9b2a..e3cc8ca9e 100644
--- a/src/kudu/util/net/socket.cc
+++ b/src/kudu/util/net/socket.cc
@@ -264,18 +264,18 @@ Socket::~Socket() {
}
Status Socket::Close() {
- if (fd_ < 0) {
+ if (PREDICT_FALSE(fd_ < 0)) {
return Status::OK();
}
- int fd = fd_;
- int ret;
- RETRY_ON_EINTR(ret, ::close(fd));
- if (ret < 0) {
- int err = errno;
- return Status::NetworkError("close error", ErrnoToString(err), err);
- }
+ const int fd = fd_;
fd_ = -1;
- return Status::OK();
+
+ Status s;
+ if (PREDICT_FALSE(::close(fd) != 0)) {
+ const int err = errno;
+ s = Status::NetworkError("close error", ErrnoToString(err), err);
+ }
+ return s;
}
Status Socket::Shutdown(bool shut_read, bool shut_write) {
diff --git a/src/kudu/util/pstack_watcher-test.cc
b/src/kudu/util/pstack_watcher-test.cc
index a993d66cd..8e0bc423e 100644
--- a/src/kudu/util/pstack_watcher-test.cc
+++ b/src/kudu/util/pstack_watcher-test.cc
@@ -76,9 +76,8 @@ TEST(TestPstackWatcher, TestPstackWatcherRunning) {
FILE* out_fp = RedirectStdout(&stdout_file);
PCHECK(out_fp != nullptr);
SCOPED_CLEANUP({
- int err;
- RETRY_ON_EINTR(err, fclose(out_fp));
- });
+ fclose(out_fp);
+ });
PstackWatcher watcher(MonoDelta::FromMilliseconds(500));
while (watcher.IsRunning()) {
SleepFor(MonoDelta::FromMilliseconds(1));
diff --git a/src/kudu/util/pstack_watcher.cc b/src/kudu/util/pstack_watcher.cc
index c727ca49a..395a29eda 100644
--- a/src/kudu/util/pstack_watcher.cc
+++ b/src/kudu/util/pstack_watcher.cc
@@ -28,7 +28,7 @@
#include <glog/logging.h>
-#include "kudu/gutil/macros.h"
+#include "kudu/gutil/port.h"
#include "kudu/gutil/strings/numbers.h"
#include "kudu/gutil/strings/split.h"
#include "kudu/gutil/strings/strip.h"
@@ -250,11 +250,13 @@ Status PstackWatcher::RunStackDump(const vector<string>&
argv) {
}
Subprocess pstack_proc(argv);
RETURN_NOT_OK_PREPEND(pstack_proc.Start(), "RunStackDump proc.Start()
failed");
- int ret;
- RETRY_ON_EINTR(ret, ::close(pstack_proc.ReleaseChildStdinFd()));
- if (ret == -1) {
- return Status::IOError("Unable to close child stdin",
ErrnoToString(errno), errno);
+
+ const int fd = pstack_proc.ReleaseChildStdinFd();
+ if (PREDICT_FALSE(::close(fd) != 0)) {
+ const int err = errno;
+ return Status::IOError("unable to close child stdin", ErrnoToString(err),
err);
}
+
RETURN_NOT_OK_PREPEND(pstack_proc.Wait(), "RunStackDump proc.Wait() failed");
int exit_code;
string exit_info;
diff --git a/src/kudu/util/subprocess-test.cc b/src/kudu/util/subprocess-test.cc
index 9a586582e..fc9cef0ab 100644
--- a/src/kudu/util/subprocess-test.cc
+++ b/src/kudu/util/subprocess-test.cc
@@ -36,7 +36,6 @@
#include <glog/logging.h>
#include <gtest/gtest.h>
-#include "kudu/gutil/macros.h"
#include "kudu/gutil/strings/substitute.h"
#include "kudu/util/barrier.h"
#include "kudu/util/env.h"
@@ -70,8 +69,7 @@ TEST_F(SubprocessTest, TestSimplePipe) {
fprintf(out, "hello world\n");
// We have to close 'out' or else tr won't write any output, since
// it enters a buffered mode if it detects that its input is a FIFO.
- int err;
- RETRY_ON_EINTR(err, fclose(out));
+ ASSERT_EQ(0, fclose(out));
char buf[1024];
ASSERT_EQ(buf, fgets(buf, sizeof(buf), in));
@@ -94,8 +92,7 @@ TEST_F(SubprocessTest, TestErrPipe) {
fprintf(out, "Hello, World\n");
// Same reasoning as above, flush to prevent tee buffering.
- int err;
- RETRY_ON_EINTR(err, fclose(out));
+ ASSERT_EQ(0, fclose(out));
FILE* in = fdopen(p.from_child_stderr_fd(), "r");
PCHECK(in);
diff --git a/src/kudu/util/subprocess.cc b/src/kudu/util/subprocess.cc
index ff631dee0..4e84e9438 100644
--- a/src/kudu/util/subprocess.cc
+++ b/src/kudu/util/subprocess.cc
@@ -138,13 +138,14 @@ void CloseNonStandardFDs(DIR* fd_dir) {
// dir->lock, so seems not worth the added complexity in lifecycle &
plumbing.
while ((ent = READDIR(fd_dir)) != nullptr) {
uint32_t fd;
- if (!safe_strtou32(ent->d_name, &fd)) continue;
+ if (!safe_strtou32(ent->d_name, &fd)) {
+ continue;
+ }
if (!(fd == STDIN_FILENO ||
fd == STDOUT_FILENO ||
fd == STDERR_FILENO ||
fd == dir_fd)) {
- int ret;
- RETRY_ON_EINTR(ret, close(fd));
+ close(fd);
}
}
}
@@ -284,8 +285,11 @@ Subprocess::~Subprocess() {
for (int i = 0; i < 3; ++i) {
if (fd_state_[i] == PIPED && child_fds_[i] >= 0) {
- int ret;
- RETRY_ON_EINTR(ret, close(child_fds_[i]));
+ if (PREDICT_FALSE(close(child_fds_[i]) != 0)) {
+ const int err = errno;
+ LOG(WARNING) << Substitute(
+ "error closing fd $0: $1", child_fds_[i], ErrnoToString(err));
+ }
}
}
}
@@ -299,15 +303,13 @@ static int pipe2(int pipefd[2], int flags) {
return -1;
}
if (fcntl(new_fds[0], F_SETFD, O_CLOEXEC) == -1) {
- int ret;
- RETRY_ON_EINTR(ret, close(new_fds[0]));
- RETRY_ON_EINTR(ret, close(new_fds[1]));
+ close(new_fds[0]);
+ close(new_fds[1]);
return -1;
}
if (fcntl(new_fds[1], F_SETFD, O_CLOEXEC) == -1) {
- int ret;
- RETRY_ON_EINTR(ret, close(new_fds[0]));
- RETRY_ON_EINTR(ret, close(new_fds[1]));
+ close(new_fds[0]);
+ close(new_fds[1]);
return -1;
}
pipefd[0] = new_fds[0];
@@ -452,9 +454,7 @@ Status Subprocess::Start() {
// Close the read side of the sync pipe;
// the write side should be closed upon execvp().
- int close_ret;
- RETRY_ON_EINTR(close_ret, close(sync_pipe[0]));
- if (close_ret == -1) {
+ if (PREDICT_FALSE(close(sync_pipe[0]) != 0)) {
int err = errno;
RAW_LOG(FATAL, "close() on the read side of sync pipe failed: [%d]",
err);
}
@@ -491,10 +491,24 @@ Status Subprocess::Start() {
// We are the parent
child_pid_ = ret;
// Close child's side of the pipes
- int close_ret;
- if (fd_state_[STDIN_FILENO] == PIPED) RETRY_ON_EINTR(close_ret,
close(child_stdin[0]));
- if (fd_state_[STDOUT_FILENO] == PIPED) RETRY_ON_EINTR(close_ret,
close(child_stdout[1]));
- if (fd_state_[STDERR_FILENO] == PIPED) RETRY_ON_EINTR(close_ret,
close(child_stderr[1]));
+ if (fd_state_[STDIN_FILENO] == PIPED) {
+ if (PREDICT_FALSE(close(child_stdin[0]) != 0)) {
+ const int err = errno;
+ RAW_LOG(WARNING, "could not close child's STDIN: [%d]", err);
+ }
+ }
+ if (fd_state_[STDOUT_FILENO] == PIPED) {
+ if (PREDICT_FALSE(close(child_stdout[1]) != 0)) {
+ const int err = errno;
+ RAW_LOG(WARNING, "could not close child's STDOUT: [%d]", err);
+ }
+ }
+ if (fd_state_[STDERR_FILENO] == PIPED) {
+ if (PREDICT_FALSE(close(child_stderr[1]) != 0)) {
+ const int err = errno;
+ RAW_LOG(WARNING, "could not close child's STDERR: [%d]", err);
+ }
+ }
// Keep parent's side of the pipes
child_fds_[STDIN_FILENO] = child_stdin[1];
child_fds_[STDOUT_FILENO] = child_stdout[0];
@@ -510,28 +524,31 @@ Status Subprocess::Start() {
// Close the write side of the sync pipe. It's crucial to make sure
// it succeeds otherwise the blocking read() below might wait forever
// even if the child process has closed the pipe.
- RETRY_ON_EINTR(close_ret, close(sync_pipe[1]));
+ const int close_ret = close(sync_pipe[1]);
PCHECK(close_ret == 0);
while (true) {
uint8_t buf;
- int err = 0;
+ int read_errno = 0;
int rc;
RETRY_ON_EINTR(rc, read(sync_pipe[0], &buf, 1));
if (rc == -1) {
- err = errno;
+ read_errno = errno;
+ }
+ if (PREDICT_FALSE(close(sync_pipe[0]) != 0)) {
+ const int err = errno;
+ RAW_LOG(FATAL, "could not close the read side of the sync pipe:
[%d]", err);
}
- RETRY_ON_EINTR(close_ret, close(sync_pipe[0]));
- PCHECK(close_ret == 0);
if (rc == 0) {
// That's OK -- expecting EOF from the other side of the pipe.
break;
- } else if (rc == -1) {
+ }
+ if (rc == -1) {
// Other errors besides EINTR are not expected.
return Status::RuntimeError("Unexpected error from the sync pipe",
- ErrnoToString(err), err);
+ ErrnoToString(read_errno), read_errno);
}
// No data is expected from the sync pipe.
- LOG(FATAL) << Substitute("$0: unexpected data from the sync pipe", rc);
+ RAW_LOG(FATAL, "%d: unexpected data from the sync pipe", rc);
}
}
}
@@ -756,10 +773,13 @@ Status Subprocess::Call(const vector<string>& argv,
}
}
- int err;
- RETRY_ON_EINTR(err, close(p.ReleaseChildStdinFd()));
- if (PREDICT_FALSE(err != 0)) {
- return Status::IOError("Unable to close child process stdin",
ErrnoToString(errno), errno);
+ {
+ const int fd = p.ReleaseChildStdinFd();
+ if (PREDICT_FALSE(close(fd) != 0)) {
+ const int err = errno;
+ return Status::IOError(
+ "Unable to close child process stdin", ErrnoToString(err), err);
+ }
}
vector<int> fds;
