From 18a1b30d9fca261912261414c5777ad1f71ec9cc Mon Sep 17 00:00:00 2001 From: Andrew Noyes Date: Sat, 23 Aug 2025 20:14:24 -0400 Subject: [PATCH] Fix EINTR handling for close --- src/connection.cpp | 8 ++-- src/main.cpp | 43 ++++++----------- src/server.cpp | 62 ++++++++++++++----------- style.md | 17 ++++++- tests/test_server_connection_return.cpp | 9 ++-- tools/load_tester.cpp | 25 +++++----- 6 files changed, 86 insertions(+), 78 deletions(-) diff --git a/src/connection.cpp b/src/connection.cpp index f2186f2..c4e4be0 100644 --- a/src/connection.cpp +++ b/src/connection.cpp @@ -28,14 +28,12 @@ Connection::~Connection() { if (auto server_ptr = server_.lock()) { server_ptr->active_connections_.fetch_sub(1, std::memory_order_relaxed); } - int e; - do { - e = close(fd_); - } while (e == -1 && errno == EINTR); - if (e == -1) { + int e = close(fd_); + if (e == -1 && errno != EINTR) { perror("close"); std::abort(); } + // EINTR ignored - fd is guaranteed closed on Linux } void Connection::appendMessage(std::string_view s, bool copy_to_arena) { diff --git a/src/main.cpp b/src/main.cpp index 73819a5..c6357dc 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -48,10 +48,6 @@ std::vector create_listen_sockets(const weaseldb::Config &config) { addr.sun_family = AF_UNIX; if (config.server.unix_socket_path.length() >= sizeof(addr.sun_path)) { - int e; - do { - e = close(sfd); - } while (e == -1 && errno == EINTR); std::fprintf(stderr, "Unix socket path too long\n"); std::abort(); } @@ -61,19 +57,11 @@ std::vector create_listen_sockets(const weaseldb::Config &config) { if (bind(sfd, (struct sockaddr *)&addr, sizeof(addr)) == -1) { perror("bind"); - int e; - do { - e = close(sfd); - } while (e == -1 && errno == EINTR); std::abort(); } if (listen(sfd, SOMAXCONN) == -1) { perror("listen"); - int e; - do { - e = close(sfd); - } while (e == -1 && errno == EINTR); std::abort(); } @@ -112,10 +100,11 @@ std::vector create_listen_sockets(const weaseldb::Config &config) { int val = 1; if (setsockopt(sfd, SOL_SOCKET, SO_REUSEADDR, &val, sizeof(val)) == -1) { perror("setsockopt SO_REUSEADDR"); - int e; - do { - e = close(sfd); - } while (e == -1 && errno == EINTR); + int e = close(sfd); + if (e == -1 && errno != EINTR) { + perror("close sfd (SO_REUSEADDR failed)"); + std::abort(); + } continue; } @@ -123,10 +112,11 @@ std::vector create_listen_sockets(const weaseldb::Config &config) { if (rp->ai_family == AF_INET || rp->ai_family == AF_INET6) { if (setsockopt(sfd, IPPROTO_TCP, TCP_NODELAY, &val, sizeof(val)) == -1) { perror("setsockopt TCP_NODELAY"); - int e; - do { - e = close(sfd); - } while (e == -1 && errno == EINTR); + int e = close(sfd); + if (e == -1 && errno != EINTR) { + perror("close sfd (TCP_NODELAY failed)"); + std::abort(); + } continue; } } @@ -135,10 +125,11 @@ std::vector create_listen_sockets(const weaseldb::Config &config) { break; /* Success */ } - int e; - do { - e = close(sfd); - } while (e == -1 && errno == EINTR); + int e = close(sfd); + if (e == -1 && errno != EINTR) { + perror("close sfd (bind failed)"); + std::abort(); + } sfd = -1; } @@ -151,10 +142,6 @@ std::vector create_listen_sockets(const weaseldb::Config &config) { if (listen(sfd, SOMAXCONN) == -1) { perror("listen"); - int e; - do { - e = close(sfd); - } while (e == -1 && errno == EINTR); std::abort(); } diff --git a/src/server.cpp b/src/server.cpp index 7e80a09..e3c3ea7 100644 --- a/src/server.cpp +++ b/src/server.cpp @@ -59,27 +59,30 @@ Server::Server(const weaseldb::Config &config, ConnectionHandler &handler, Server::~Server() { if (shutdown_pipe_[0] != -1) { - int e; - do { - e = close(shutdown_pipe_[0]); - } while (e == -1 && errno == EINTR); + int e = close(shutdown_pipe_[0]); + if (e == -1 && errno != EINTR) { + perror("close shutdown_pipe_[0]"); + std::abort(); + } shutdown_pipe_[0] = -1; } if (shutdown_pipe_[1] != -1) { - int e; - do { - e = close(shutdown_pipe_[1]); - } while (e == -1 && errno == EINTR); + int e = close(shutdown_pipe_[1]); + if (e == -1 && errno != EINTR) { + perror("close shutdown_pipe_[1]"); + std::abort(); + } shutdown_pipe_[1] = -1; } // Close all epoll instances for (int epollfd : epoll_fds_) { if (epollfd != -1) { - int e; - do { - e = close(epollfd); - } while (e == -1 && errno == EINTR); + int e = close(epollfd); + if (e == -1 && errno != EINTR) { + perror("close epollfd"); + std::abort(); + } } } epoll_fds_.clear(); @@ -87,10 +90,11 @@ Server::~Server() { // Close all listen sockets (Server always owns them) for (int fd : listen_fds_) { if (fd != -1) { - int e; - do { - e = close(fd); - } while (e == -1 && errno == EINTR); + int e = close(fd); + if (e == -1 && errno != EINTR) { + perror("close listen_fd"); + std::abort(); + } } } @@ -226,13 +230,16 @@ int Server::createLocalConnection() { if (epoll_ctl(epollfd, EPOLL_CTL_ADD, server_fd, &event) == -1) { perror("epoll_ctl ADD local connection"); connection_registry_.remove(server_fd); - int e; - do { - e = close(server_fd); - } while (e == -1 && errno == EINTR); - do { - e = close(client_fd); - } while (e == -1 && errno == EINTR); + int e = close(server_fd); + if (e == -1 && errno != EINTR) { + perror("close server_fd"); + std::abort(); + } + e = close(client_fd); + if (e == -1 && errno != EINTR) { + perror("close client_fd"); + std::abort(); + } return -1; } @@ -384,10 +391,11 @@ void Server::start_io_threads(std::vector &threads) { if (config_.server.max_connections > 0 && active_connections_.load(std::memory_order_relaxed) >= config_.server.max_connections) { - int e; - do { - e = close(fd); - } while (e == -1 && errno == EINTR); + int e = close(fd); + if (e == -1 && errno != EINTR) { + perror("close fd (max connections)"); + std::abort(); + } continue; } diff --git a/style.md b/style.md index ce1b5d3..700583f 100644 --- a/style.md +++ b/style.md @@ -331,7 +331,7 @@ static_assert(std::is_trivially_destructible_v, "Arena requires trivially des ### System Call Error Handling -When a system call is interrupted by a signal (`EINTR`), it is usually necessary to retry the call. This is especially true for "slow" system calls that can block for a long time, such as `read`, `write`, `accept`, `connect`, `close`, `sem_wait`, and `epoll_wait`. +When a system call is interrupted by a signal (`EINTR`), it is usually necessary to retry the call. This is especially true for "slow" system calls that can block for a long time, such as `read`, `write`, `accept`, `connect`, `sem_wait`, and `epoll_wait`. **Rule:** Always wrap potentially interruptible system calls in a `do-while` loop that checks for `EINTR`. @@ -350,6 +350,20 @@ if (fd == -1) { } ``` +**Special case - close():** + +The `close()` system call is a special case on Linux. According to `man 2 close`, when `close()` returns `EINTR` on Linux, the file descriptor is still guaranteed to be closed. Therefore, `close()` should **never** be retried. + +```cpp +// Correct: Do not retry close() on EINTR +int e = close(fd); +if (e == -1 && errno != EINTR) { + // Handle non-EINTR errors only + perror("close"); + std::abort(); +} +// Note: fd is guaranteed closed even on EINTR +``` **Non-interruptible calls:** @@ -361,6 +375,7 @@ Most system calls are not interruptible in practice. For these, it is not necess * `pipe` * `setsockopt` * `epoll_create1` +* `close` (special case: guaranteed closed even on EINTR on Linux) When in doubt, consult the `man` page for the specific system call to see if it can return `EINTR`. diff --git a/tests/test_server_connection_return.cpp b/tests/test_server_connection_return.cpp index decbc9c..415333f 100644 --- a/tests/test_server_connection_return.cpp +++ b/tests/test_server_connection_return.cpp @@ -90,10 +90,11 @@ TEST_CASE( CHECK(std::string(buffer, bytes_read) == std::string(test_message)); // Cleanup - int e; - do { - e = close(client_fd); - } while (e == -1 && errno == EINTR); + int e = close(client_fd); + if (e == -1 && errno != EINTR) { + perror("close client_fd"); + abort(); + } server->shutdown(); server_thread.join(); { diff --git a/tools/load_tester.cpp b/tools/load_tester.cpp index c60887e..27063f9 100644 --- a/tools/load_tester.cpp +++ b/tools/load_tester.cpp @@ -94,10 +94,11 @@ int getConnectFd(const char *node, const char *service) { break; /* Success */ } - int e; - do { - e = close(sfd); - } while (e == -1 && errno == EINTR); + int e = close(sfd); + if (e == -1 && errno != EINTR) { + perror("close sfd (load_tester)"); + abort(); + } } freeaddrinfo(result); /* No longer needed */ @@ -252,11 +253,8 @@ struct Connection { bool error = false; ~Connection() { - int e; - do { - e = close(fd); - } while (e == -1 && errno == EINTR); - if (e == -1) { + int e = close(fd); + if (e == -1 && errno != EINTR) { perror("close"); abort(); } @@ -830,9 +828,10 @@ int main(int argc, char *argv[]) { // Clean up epoll file descriptors for (int epollfd : g_epoll_fds) { - int e; - do { - e = close(epollfd); - } while (e == -1 && errno == EINTR); + int e = close(epollfd); + if (e == -1 && errno != EINTR) { + perror("close epollfd"); + abort(); + } } }