From a820efa2e69656feb465270f7cfcfe47abfb1e47 Mon Sep 17 00:00:00 2001 From: Andrew Noyes Date: Sat, 23 Aug 2025 17:28:34 -0400 Subject: [PATCH] Fix EINTR handling --- src/connection.cpp | 5 +++- src/main.cpp | 35 ++++++++++++++++++++----- src/server.cpp | 34 +++++++++++++++++++----- style.md | 35 +++++++++++++++++++++++++ tests/test_server_connection_return.cpp | 15 ++++++++--- tools/load_tester.cpp | 31 +++++++++++++++++----- 6 files changed, 131 insertions(+), 24 deletions(-) diff --git a/src/connection.cpp b/src/connection.cpp index d86ac38..f2186f2 100644 --- a/src/connection.cpp +++ b/src/connection.cpp @@ -28,7 +28,10 @@ Connection::~Connection() { if (auto server_ptr = server_.lock()) { server_ptr->active_connections_.fetch_sub(1, std::memory_order_relaxed); } - int e = close(fd_); + int e; + do { + e = close(fd_); + } while (e == -1 && errno == EINTR); if (e == -1) { perror("close"); std::abort(); diff --git a/src/main.cpp b/src/main.cpp index 336642f..73819a5 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -48,7 +48,10 @@ 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)) { - close(sfd); + int e; + do { + e = close(sfd); + } while (e == -1 && errno == EINTR); std::fprintf(stderr, "Unix socket path too long\n"); std::abort(); } @@ -58,13 +61,19 @@ std::vector create_listen_sockets(const weaseldb::Config &config) { if (bind(sfd, (struct sockaddr *)&addr, sizeof(addr)) == -1) { perror("bind"); - close(sfd); + int e; + do { + e = close(sfd); + } while (e == -1 && errno == EINTR); std::abort(); } if (listen(sfd, SOMAXCONN) == -1) { perror("listen"); - close(sfd); + int e; + do { + e = close(sfd); + } while (e == -1 && errno == EINTR); std::abort(); } @@ -103,7 +112,10 @@ 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"); - close(sfd); + int e; + do { + e = close(sfd); + } while (e == -1 && errno == EINTR); continue; } @@ -111,7 +123,10 @@ 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"); - close(sfd); + int e; + do { + e = close(sfd); + } while (e == -1 && errno == EINTR); continue; } } @@ -120,7 +135,10 @@ std::vector create_listen_sockets(const weaseldb::Config &config) { break; /* Success */ } - close(sfd); + int e; + do { + e = close(sfd); + } while (e == -1 && errno == EINTR); sfd = -1; } @@ -133,7 +151,10 @@ std::vector create_listen_sockets(const weaseldb::Config &config) { if (listen(sfd, SOMAXCONN) == -1) { perror("listen"); - close(sfd); + int e; + do { + e = close(sfd); + } while (e == -1 && errno == EINTR); std::abort(); } diff --git a/src/server.cpp b/src/server.cpp index f38c13d..7e80a09 100644 --- a/src/server.cpp +++ b/src/server.cpp @@ -59,18 +59,27 @@ Server::Server(const weaseldb::Config &config, ConnectionHandler &handler, Server::~Server() { if (shutdown_pipe_[0] != -1) { - close(shutdown_pipe_[0]); + int e; + do { + e = close(shutdown_pipe_[0]); + } while (e == -1 && errno == EINTR); shutdown_pipe_[0] = -1; } if (shutdown_pipe_[1] != -1) { - close(shutdown_pipe_[1]); + int e; + do { + e = close(shutdown_pipe_[1]); + } while (e == -1 && errno == EINTR); shutdown_pipe_[1] = -1; } // Close all epoll instances for (int epollfd : epoll_fds_) { if (epollfd != -1) { - close(epollfd); + int e; + do { + e = close(epollfd); + } while (e == -1 && errno == EINTR); } } epoll_fds_.clear(); @@ -78,7 +87,10 @@ Server::~Server() { // Close all listen sockets (Server always owns them) for (int fd : listen_fds_) { if (fd != -1) { - close(fd); + int e; + do { + e = close(fd); + } while (e == -1 && errno == EINTR); } } @@ -214,8 +226,13 @@ 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); - close(server_fd); - close(client_fd); + int e; + do { + e = close(server_fd); + } while (e == -1 && errno == EINTR); + do { + e = close(client_fd); + } while (e == -1 && errno == EINTR); return -1; } @@ -367,7 +384,10 @@ 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) { - close(fd); + int e; + do { + e = close(fd); + } while (e == -1 && errno == EINTR); continue; } diff --git a/style.md b/style.md index 583d846..ce1b5d3 100644 --- a/style.md +++ b/style.md @@ -329,6 +329,41 @@ static_assert(std::is_trivially_destructible_v, "Arena requires trivially des // assert(file_exists(path)); // File might legitimately not exist - use return code instead ``` +### 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`. + +**Rule:** Always wrap potentially interruptible system calls in a `do-while` loop that checks for `EINTR`. + +**Example:** + +```cpp +int fd; +do { + fd = accept(listen_fd, nullptr, nullptr); +} while (fd == -1 && errno == EINTR); + +if (fd == -1) { + // Handle other errors + perror("accept"); + abort(); +} +``` + + +**Non-interruptible calls:** + +Most system calls are not interruptible in practice. For these, it is not necessary to add a retry loop. This includes: + +* `fcntl` (with `F_GETFL`, `F_SETFL`, `F_GETFD`, `F_SETFD` - note: `F_SETLKW` and `F_OFD_SETLKW` CAN return EINTR) +* `epoll_ctl` +* `socketpair` +* `pipe` +* `setsockopt` +* `epoll_create1` + +When in doubt, consult the `man` page for the specific system call to see if it can return `EINTR`. + --- ## Documentation diff --git a/tests/test_server_connection_return.cpp b/tests/test_server_connection_return.cpp index fb377ca..decbc9c 100644 --- a/tests/test_server_connection_return.cpp +++ b/tests/test_server_connection_return.cpp @@ -69,12 +69,18 @@ TEST_CASE( // Write some test data const char *test_message = "Hello, World!"; - ssize_t bytes_written = write(client_fd, test_message, strlen(test_message)); + ssize_t bytes_written; + do { + bytes_written = write(client_fd, test_message, strlen(test_message)); + } while (bytes_written == -1 && errno == EINTR); REQUIRE(bytes_written == strlen(test_message)); // Read the echoed response char buffer[1024] = {0}; - ssize_t bytes_read = read(client_fd, buffer, sizeof(buffer) - 1); + ssize_t bytes_read; + do { + bytes_read = read(client_fd, buffer, sizeof(buffer) - 1); + } while (bytes_read == -1 && errno == EINTR); if (bytes_read == -1) { perror("read failed"); } @@ -84,7 +90,10 @@ TEST_CASE( CHECK(std::string(buffer, bytes_read) == std::string(test_message)); // Cleanup - close(client_fd); + int e; + do { + e = close(client_fd); + } while (e == -1 && errno == EINTR); server->shutdown(); server_thread.join(); { diff --git a/tools/load_tester.cpp b/tools/load_tester.cpp index 16f2fd6..c60887e 100644 --- a/tools/load_tester.cpp +++ b/tools/load_tester.cpp @@ -85,11 +85,19 @@ int getConnectFd(const char *node, const char *service) { continue; } - if (connect(sfd, rp->ai_addr, rp->ai_addrlen) == 0) { + int conn_result; + do { + conn_result = connect(sfd, rp->ai_addr, rp->ai_addrlen); + } while (conn_result == -1 && errno == EINTR); + + if (conn_result == 0) { break; /* Success */ } - close(sfd); + int e; + do { + e = close(sfd); + } while (e == -1 && errno == EINTR); } freeaddrinfo(result); /* No longer needed */ @@ -113,7 +121,10 @@ int getConnectFdUnix(const char *socket_name) { memset(&addr, 0, sizeof(addr)); addr.sun_family = AF_UNIX; strncpy(addr.sun_path, socket_name, sizeof(addr.sun_path) - 1); - int e = connect(sfd, (struct sockaddr *)&addr, sizeof(addr)); + int e; + do { + e = connect(sfd, (struct sockaddr *)&addr, sizeof(addr)); + } while (e == -1 && errno == EINTR); if (e == -1) { perror("connect"); abort(); @@ -241,7 +252,10 @@ struct Connection { bool error = false; ~Connection() { - int e = close(fd); + int e; + do { + e = close(fd); + } while (e == -1 && errno == EINTR); if (e == -1) { perror("close"); abort(); @@ -711,7 +725,9 @@ int main(int argc, char *argv[]) { while (!g_shutdown.load(std::memory_order_relaxed)) { int e; { - e = sem_wait(&connectionLimit); + do { + e = sem_wait(&connectionLimit); + } while (e == -1 && errno == EINTR); if (e == -1) { perror("sem_wait"); abort(); @@ -814,6 +830,9 @@ int main(int argc, char *argv[]) { // Clean up epoll file descriptors for (int epollfd : g_epoll_fds) { - close(epollfd); + int e; + do { + e = close(epollfd); + } while (e == -1 && errno == EINTR); } }