weaselab/weaseldb
2
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Fix EINTR handling for close

Browse Source
This commit is contained in:
Andrew Noyes
2025-08-23 20:14:24 -04:00
parent 7db0e331e4
commit 18a1b30d9f
6 changed files with 86 additions and 78 deletions
Download Patch File Download Diff File Expand all files Collapse all files

8
src/connection.cpp
View File

@@ -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) {

43
src/main.cpp
View File

@@ -48,10 +48,6 @@ std::vector<int> 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<int> 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<int> 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<int> 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<int> 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<int> 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();
}

60
src/server.cpp
View File

@@ -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 {
int e = close(server_fd);
if (e == -1 && errno != EINTR) {
perror("close server_fd");
std::abort();
}
e = close(client_fd);
} while (e == -1 && errno == EINTR);
if (e == -1 && errno != EINTR) {
perror("close client_fd");
std::abort();
}
return -1;
}
@@ -384,10 +391,11 @@ void Server::start_io_threads(std::vector<std::thread> &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;
}

17
style.md
View File

@@ -331,7 +331,7 @@ static_assert(std::is_trivially_destructible_v<T>, "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`.

9
tests/test_server_connection_return.cpp
View File

@@ -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();
{

25
tools/load_tester.cpp
View File

@@ -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();
}
}
}