diff --git a/CHANGELOG.md b/CHANGELOG.md index dd732457..8051c280 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -430,6 +430,18 @@ API surface). ### Fixed +- **`net::SocketServer` goes on accepting after the process runs out of file + descriptors.** Any `accept(2)` failure but `EINTR` and `EAGAIN` ended the + accept thread, so a single `EMFILE` stopped the server accepting for good, + while the port stayed bound and clients hung in their Upgrade read. The loop + now backs off on exhaustion (`EMFILE`, `ENFILE`, `ENOBUFS`, `ENOMEM`), from + 10 ms to at most 1 s, and serves again once descriptors are free; `close()` + still ends it at once. A connection that failed while pending — the network + errors Linux `accept(2)` reports for it, and a firewall's `EPERM` — is + skipped like `EAGAIN`. The loop ends only for a listener that cannot accept, + and now logs why. `TcpSocket::tryAccept()` throws `std::system_error` + carrying the `errno`, a `std::runtime_error` as before. + - **An installed `qt_forms` component compiles.** `forms_controller_core.hpp` includes `morph/qt/qt_executor.hpp`, which was installed only by the `qt` component (`MORPH_BUILD_QT`, which needs Qt WebSockets), so an install built diff --git a/docs/spec/core/backend.md b/docs/spec/core/backend.md index 126c1d36..2f179342 100644 --- a/docs/spec/core/backend.md +++ b/docs/spec/core/backend.md @@ -2012,6 +2012,37 @@ ready connection with `TcpSocket::tryAccept()`, which answers `std::nullopt` rather than parking when a readiness report has gone stale. `close()` signals the wakeup before `join()`, which is what ends the loop. +**A failed accept ends the loop only when the listener cannot accept.** +`accept(2)` answers for three different things, and `kAcceptErrors` in +`detail/tcp_socket.hpp` sorts each `errno` into one of them: + +- **The pending connection failed** (`AcceptFailure::NoConnection`). Linux + `accept(2)` hands back a network error already pending on the new connection + — `ENETDOWN`, `EPROTO`, `ENOPROTOOPT`, `EHOSTDOWN`, `ENONET`, `EHOSTUNREACH`, + `EOPNOTSUPP`, `ENETUNREACH` — and says to treat them like `EAGAIN`; `EPERM` is + a firewall rule refusing that one connection. `tryAccept()` answers these + `std::nullopt`, exactly as for a stale readiness report, so the loop waits + for the next connection through the branch it already takes for `EAGAIN`. + `ECONNABORTED` is not among them: a connection reset while pending is still + accepted on Linux, and its reset reaches the first `recvSome()` as an + orderly close. +- **The process or the system ran out of something** (`Exhausted`: `EMFILE`, + `ENFILE`, `ENOBUFS`, `ENOMEM`). The same `accept` succeeds once it is + released, so the loop waits and tries again: 10 ms after the first failure, + doubling to 1 s, and back to none once an accept stops failing. The wait is a + `poll()` on the wakeup alone — the listener stays readable while the + connection that failed is queued, so polling it too would spin — which is + also why `close()` still ends a loop that is waiting. The first failure of a + run is logged at warn. +- **The listener cannot accept** (`ListenerUnusable`: `EBADF`, `ENOTSOCK`, + `EINVAL`, `EFAULT`, and any `errno` no row names). The loop ends and logs + why at warn, since the port stays bound until `close()` and a client would + otherwise only hang in its Upgrade read. + +`tests/net/test_socket_server.cpp` exhausts `RLIMIT_NOFILE` under a running +server, restores it, and requires a connection that queued in between to +complete its handshake. + **The listener's non-blocking mode stops at the listener.** `TcpSocket`'s fd-adopting constructor clears `O_NONBLOCK` on every descriptor it takes ownership of, so a connection from `accept()` or `tryAccept()` is always diff --git a/include/morph/net/detail/tcp_socket.hpp b/include/morph/net/detail/tcp_socket.hpp index d9365d9c..1806135a 100644 --- a/include/morph/net/detail/tcp_socket.hpp +++ b/include/morph/net/detail/tcp_socket.hpp @@ -9,6 +9,7 @@ #include #include +#include #include #include #include @@ -31,6 +32,76 @@ namespace morph::net::detail { +/// @brief What a failed `accept(2)` says, for a loop that keeps accepting. +enum class AcceptFailure : std::uint8_t { + /// The connection that was pending failed before it was taken. Nothing is + /// wrong with the listener: wait for the next one, as after `EAGAIN`. + NoConnection, + /// The process or the system is out of something `accept(2)` needs. The + /// same call succeeds once it is released, so the loop waits and retries. + Exhausted, + /// The listener itself cannot accept, now or later. + ListenerUnusable, +}; + +/// @brief One `errno` that `accept(2)` can answer, and what it says. +struct AcceptErrorRow { + int err; ///< The `errno` value. + AcceptFailure failure; ///< What an accept loop does about it. +}; + +/// @brief The `errno` values `TcpSocket::tryAccept()` and +/// `SocketServer`'s accept loop tell apart. +/// +/// `NoConnection` is Linux `accept(2)`'s own instruction: it hands back a +/// network error already pending on the new connection from `accept()` +/// itself, and says to treat the TCP/IP ones like `EAGAIN` by retrying. +/// `EPERM` is Linux refusing that one connection by firewall rule. +/// `EOPNOTSUPP` is on the pending-error list too; it would also be the answer +/// for a listener that is not `SOCK_STREAM`, which `listen()` never creates. +/// +/// `ECONNABORTED` has no row. A connection reset while pending is still +/// accepted on Linux, and the reset surfaces on the first `recvSome()`, which +/// reads it as an orderly close; the `accept()` comment carries the +/// measurement. +/// +/// An `errno` no row names is `ListenerUnusable` (see `acceptFailureOf`). +inline constexpr std::array kAcceptErrors{ + AcceptErrorRow{.err = ENETDOWN, .failure = AcceptFailure::NoConnection}, + AcceptErrorRow{.err = EPROTO, .failure = AcceptFailure::NoConnection}, + AcceptErrorRow{.err = ENOPROTOOPT, .failure = AcceptFailure::NoConnection}, + AcceptErrorRow{.err = EHOSTDOWN, .failure = AcceptFailure::NoConnection}, +#ifdef ENONET + AcceptErrorRow{.err = ENONET, .failure = AcceptFailure::NoConnection}, +#endif + AcceptErrorRow{.err = EHOSTUNREACH, .failure = AcceptFailure::NoConnection}, + AcceptErrorRow{.err = EOPNOTSUPP, .failure = AcceptFailure::NoConnection}, + AcceptErrorRow{.err = ENETUNREACH, .failure = AcceptFailure::NoConnection}, + AcceptErrorRow{.err = EPERM, .failure = AcceptFailure::NoConnection}, + AcceptErrorRow{.err = EMFILE, .failure = AcceptFailure::Exhausted}, + AcceptErrorRow{.err = ENFILE, .failure = AcceptFailure::Exhausted}, + AcceptErrorRow{.err = ENOBUFS, .failure = AcceptFailure::Exhausted}, + AcceptErrorRow{.err = ENOMEM, .failure = AcceptFailure::Exhausted}, + AcceptErrorRow{.err = EBADF, .failure = AcceptFailure::ListenerUnusable}, + AcceptErrorRow{.err = ENOTSOCK, .failure = AcceptFailure::ListenerUnusable}, + AcceptErrorRow{.err = EINVAL, .failure = AcceptFailure::ListenerUnusable}, + AcceptErrorRow{.err = EFAULT, .failure = AcceptFailure::ListenerUnusable}, +}; + +/// @brief Looks up @p err in `kAcceptErrors`. +/// @param err An `errno` value `accept(2)` answered. +/// @return Its row's `AcceptFailure`; `ListenerUnusable` for a value no row +/// names, so an answer nobody classified ends an accept loop rather +/// than being retried as though it passed. +[[nodiscard]] inline AcceptFailure acceptFailureOf(int err) noexcept { + for (auto const& row : kAcceptErrors) { + if (row.err == err) { + return row.failure; + } + } + return AcceptFailure::ListenerUnusable; +} + /// @brief RAII wrapper around a POSIX (BSD sockets) TCP file descriptor. /// /// Linux/macOS only today — see `docs/spec/core/backend.md`'s `morph::net` @@ -333,10 +404,13 @@ class TcpSocket { /// macOS/BSD's inheritance of the listener's flag reaching `recvSome()` /// Any rewrite of this function has to keep going through that /// constructor, or do the reset itself. - /// @return The accepted `TcpSocket`, or `std::nullopt` when no connection - /// was pending — a readiness report that went stale before the - /// `accept`, which the caller answers by waiting again. - /// @throws std::runtime_error if `::accept` fails for any other reason. + /// @return The accepted `TcpSocket`, or `std::nullopt` when there is no + /// connection to take: none was pending — a readiness report that + /// went stale before the `accept` — or the pending one failed + /// first (`AcceptFailure::NoConnection` in `kAcceptErrors`). The + /// caller answers both by waiting again. + /// @throws std::system_error carrying the `errno`, if `::accept` fails for + /// any other reason; `acceptFailureOf()` says what it means. // Non-const to match accept(): taking a connection consumes it from the // listener's queue, which is state this object owns. // NOLINTNEXTLINE(readability-make-member-function-const) @@ -350,10 +424,10 @@ class TcpSocket { if (err == EINTR) { // retried for the same reason accept() retries it continue; } - if (wouldBlock(err)) { + if (wouldBlock(err) || acceptFailureOf(err) == AcceptFailure::NoConnection) { return std::nullopt; } - throw std::runtime_error("TcpSocket::tryAccept: " + errnoMessage(err)); + throw std::system_error(err, std::system_category(), "TcpSocket::tryAccept"); } } diff --git a/include/morph/net/socket_server.hpp b/include/morph/net/socket_server.hpp index 4724eb9b..69e0e354 100644 --- a/include/morph/net/socket_server.hpp +++ b/include/morph/net/socket_server.hpp @@ -3,6 +3,7 @@ #pragma once #include +#include #include #include #include @@ -11,10 +12,12 @@ #include #include #include +#include #include #include #include #include +#include #include #include @@ -256,6 +259,61 @@ class SocketServer { } }; + /// First wait after the process runs out of something `accept(2)` needs; + /// each further failure in the same run doubles it. + static constexpr std::chrono::milliseconds kAcceptBackoffFirst{10}; + /// The longest wait: how late a recovered process takes its next + /// connection. `close()` is not held by it -- the wait ends on the wakeup. + static constexpr std::chrono::milliseconds kAcceptBackoffMax{1000}; + + /// Waits out an accept backoff on the wakeup alone: the listener stays + /// readable while the connection that failed is still queued, so polling + /// it too would end the wait at once and spin. + /// @return `false` when the loop must end instead: `close()` signalled the + /// wakeup, or `poll()` itself failed. + static bool waitOutBackoff(::core::platform::NativeHandle wakeupHandle, std::chrono::milliseconds backoff) { + pollfd wakeupOnly{}; + wakeupOnly.fd = wakeupHandle; + wakeupOnly.events = POLLIN; + int const ready = ::poll(&wakeupOnly, 1, static_cast(backoff.count())); + if (ready > 0) { + return false; // close() signalled the wakeup + } + return ready == 0 || errno == EINTR; + } + + /// Takes the pending connection, if there is one, into @p clientSocket, + /// and sorts a failure by `acceptFailureOf`: running out of something sets + /// @p backoff, anything else ends the loop. + /// @return `false` when the loop must end. + bool acceptOrBackOff(std::optional<::morph::net::detail::TcpSocket>& clientSocket, + std::chrono::milliseconds& backoff) { + try { + clientSocket = _listenSocket.tryAccept(); + } catch (const std::system_error& failure) { + if (::morph::net::detail::acceptFailureOf(failure.code().value()) == + ::morph::net::detail::AcceptFailure::Exhausted) { + // Said once per run of failures, not once per doubling. + if (backoff.count() == 0) { + ::morph::log::logWarn(std::string{"[net::SocketServer] accept failed, backing off: "} + + failure.what()); + } + backoff = backoff.count() == 0 ? kAcceptBackoffFirst : std::min(backoff * 2, kAcceptBackoffMax); + return true; + } + // Not a condition of the moment: a loop still polling this + // listener would get the same answer forever. Said, because the + // port stays bound and a client would otherwise simply hang. + ::morph::log::logWarn(std::string{"[net::SocketServer] accept loop stopped: "} + failure.what()); + return false; + } catch (const std::exception& failure) { + ::morph::log::logWarn(std::string{"[net::SocketServer] accept loop stopped: "} + failure.what()); + return false; + } + backoff = {}; + return true; + } + void acceptLoop() { // listen() opens the wakeup before it starts this loop, and nothing // resets it while the loop runs. @@ -263,7 +321,13 @@ class SocketServer { return; } auto const wakeupHandle = _wakeup->nativeHandle(); + // Zero, or how long to wait before the next accept after the process + // ran out of something accept(2) needs. + std::chrono::milliseconds backoff{}; for (;;) { + if (backoff.count() > 0 && !waitOutBackoff(wakeupHandle, backoff)) { + return; + } std::array fds{}; pollfd& listenPfd = fds.front(); pollfd& wakeupPfd = fds.back(); @@ -286,13 +350,11 @@ class SocketServer { continue; } std::optional<::morph::net::detail::TcpSocket> clientSocket; - try { - clientSocket = _listenSocket.tryAccept(); - } catch (const std::exception&) { - return; // the listener is unusable (server closing) + if (!acceptOrBackOff(clientSocket, backoff)) { + return; } if (!clientSocket) { - continue; // readiness went stale before the accept; wait again + continue; // nothing to take this time, or backing off; wait again } if (_closing.load()) { return; diff --git a/scripts/branch_partial_allowlist.json b/scripts/branch_partial_allowlist.json index e2bbb112..27098ead 100644 --- a/scripts/branch_partial_allowlist.json +++ b/scripts/branch_partial_allowlist.json @@ -127,22 +127,16 @@ }, { "file": "include/morph/net/socket_server.hpp", - "line": 210, + "line": 213, "source": "if (t.joinable()) {", "reason": "Unreachable by construction (net audit, `socket_server.hpp` finding #6, verified against a `close()` whose whole body is serialized under `_closeMtx`). `_clientThreads` has exactly one push site (`acceptLoop()`, always a freshly-constructed, running `std::thread`) and this loop is the only place any entry is ever joined or detached. With `close()`'s entire body serialized by `_closeMtx`, only one caller's `close()` can ever reach this loop: a second, later call observes `wasAlreadyClosing == true` and `!_acceptThread.joinable()` (already joined by the winner) and takes the early return above, before ever reaching the client-thread swap-and-join section this line is in. So every `std::thread` this loop iterates over is a fresh entry pushed by `acceptLoop()` that nothing has touched yet -- `joinable()` cannot be false here." }, { "file": "include/morph/net/socket_server.hpp", - "line": 245, + "line": 248, "source": "if (closed.load() || !socket.valid()) {", "reason": "The `!socket.valid()` disjunct is unreachable by construction (net audit, `socket_server.hpp` finding #7). `ClientConnection::socket` is set once at construction and never moved from or reassigned anywhere in this file (only method calls on it, never an assignment or `std::move`); `TcpSocket::valid()` is `_fd >= 0`, and `_fd` only becomes -1 in the move constructor/assignment and the destructor, neither of which can run while `sendText()` holds a `shared_ptr`. `shutdownBoth()` does not touch `_fd`. `closed.store(true)` is what every real teardown path sets first, so the `closed.load()` disjunct alone accounts for all of them. This entry survives a gate failure that looks like it retires it, so the artifact is recorded here: without -fprofile-update=atomic, a coverage run can report this disjunct as *taken* and fail the gate with \"include/morph/net/socket_server.hpp:242 is allowlisted as an uncoverable partial branch, but it is not a partial line in this report\", while four neighbouring reports of the same unchanged code all report it untaken (11 partial lines in this file, each time). The cause: llvm-cov derives the second operand of a short-circuit || by subtracting counters rather than counting it, so with non-atomic counters a concurrent update makes that subtraction go negative and wrap to a huge \"taken\" count. Demonstrated with a controlled probe: 12 threads over `if (flag.load() || !alwaysTrue())` reported `True: 18.4E` for the never-taken arm in 8 runs of 8; the same binary single-threaded reported `True: 0` in 3 of 3, and the same 12 threads built with -fprofile-update=atomic reported `True: 0` in 3 of 3. cmake/compiler_options.cmake's apply_coverage() passes that flag, which is what makes this entry stable rather than flaky. Re-measured with it: `Branch (242:17): [True: 69, False: 1.52k]` and `Branch (242:34): [True: 0, False: 1.52k]`. What would make it reachable, and so retire this entry: any code that move-assigns or destroys ClientConnection::socket while another thread can be inside sendText() -- replacing the connection's socket on a reconnect, say, or dropping the shared_ptr discipline. If this gate reports the line non-partial again on a build carrying -fprofile-update=atomic, that is a real change and the entry should be deleted rather than argued with." }, - { - "file": "include/morph/net/socket_server.hpp", - "line": 294, - "source": "if (!clientSocket) {", - "reason": "Real, reachable race (`tryAccept()` returning nullopt because the pending connection went away before it was taken), but accepted as documented rather than forced with a flaky test after extensive attempts (net audit, `socket_server.hpp` finding #10). Three different techniques were tried: a single real `TcpSocket::connect()` immediately followed by an abortive (`SO_LINGER{1,0}`) close (0/150 hits); a burst of many such attempts to build backlog depth (still 0 hits); and a burst of bare non-blocking `::connect()`+abort attempts skipping `TcpSocket::connect()`'s `getaddrinfo()`/poll overhead (960 attempts across 15 bursts, still 0 hits, with most connections resetting before the TCP handshake progressed far enough to make the listener readable at all, rather than after). No way was found, from outside the process, to reliably land in the specific narrow window this branch requires on this machine. Reported as attempted-and-left-open rather than forcing something flakier. RE-READ under -fprofile-update=atomic, over several runs rather than one, because a single figure cannot be told apart from run order. Five CI-equivalent coverage runs, same binaries, same 2962 tests, atomic counters: `Branch (285:17): [True: 0, False: 3.80k / 3.80k / 3.82k / 3.75k / 3.78k]`, with line 286 (`continue`) at 0 executions in every run. The denominator moves with how many accepts the suite happens to perform -- 3.75k to 3.82k across the five, and 576 on another machine -- and the numerator does not move at all: tens of thousands of accepts across five runs, none of them nullopt. The disposition stands, and the figure it quotes is one no concurrent counter update can inflate. What would retire it: a True count above 0 in any run." - }, { "file": "include/morph/net/socket_backend.hpp", "line": 145, diff --git a/tests/net/fd_limit_clamp.hpp b/tests/net/fd_limit_clamp.hpp new file mode 100644 index 00000000..5ed31909 --- /dev/null +++ b/tests/net/fd_limit_clamp.hpp @@ -0,0 +1,137 @@ +// SPDX-License-Identifier: Apache-2.0 + +#pragma once + +#include +#include +#include + +#include +#include +#include +#include +#include + +namespace morph::testing { + +// What a scan of this process's fd table found, up to some ceiling: the +// highest fd open, and how many fds in that range are open at all. Same +// technique as tests/net/test_socket_server.cpp's own `highestOpenFd()`, +// with the count added -- `highest + 1 - open` is the size of the gap that +// `FdLimitClamp` exists to fill, and it is *reported* rather than assumed +// away. +struct FdScan { + int highest = -1; + int open = 0; +}; + +inline FdScan scanOpenFds(int limit) { + FdScan scan; + for (int fd = 0; fd < limit; ++fd) { + if (::fcntl(fd, F_GETFD) != -1) { + scan.highest = fd; + ++scan.open; + } + } + return scan; +} + +// RAII guard: forces the fd table to genuinely zero headroom and restores the +// original state on destruction. +// +// Lowering RLIMIT_NOFILE to `highestOpenFd()+1` assumes fd allocation so +// far has been gap-free, which nothing guarantees: a sanitizer runtime opens +// and closes descriptors of its own at startup, and an earlier TEST_CASE can +// open and close a *lower*- +// numbered fd (e.g. a transient connect() attempt) while a *higher*-numbered +// one stays permanently open (this process's getaddrinfo() call opens a +// long-lived resolver connection on first use, on this platform), leaving a +// gap below the computed "highest". The very next fd-allocating syscall then +// silently reuses that gap -- allowed by RLIMIT_NOFILE, since the gap's fd +// number is still under the limit -- and the intended EMFILE never fires. +// Confirmed empirically: the naive `highest+1` version of this guard passed +// this file's [tcp] tag roughly half the time and failed the other half. +// +// This version is self-verifying instead of computed: it lowers the limit, +// then actually opens `/dev/null` repeatedly until `open()` itself fails, +// filling any such gap for real rather than assuming there isn't one. Only +// once real exhaustion has been *observed* does the syscall under test run. +// +// This test fails under concurrent machine load if the clamp does not hold, so +// the clamp *says* what it measured rather than leaving the next reader +// guessing. `exhausted()` and `summary()` are that: every call site asserts +// `exhausted()` before the syscall under test -- so a clamp that did not bite +// fails on its own terms instead of being mistaken for a bug in `accept()` -- +// and `INFO(summary())` puts the whole measurement (ambient fd table, limit +// applied, fds it took to fill, and the errno the fill loop stopped on) into +// the failure output of whichever assertion follows. +// +// The `exhausted()` check is deliberately *not* a REQUIRE inside this +// constructor: a Catch2 assertion failure there throws out of a half-built +// object, whose destructor never runs, leaving the process clamped and the +// filler fds leaked for every test after it. +class FdLimitClamp { +public: + FdLimitClamp() { + REQUIRE(::getrlimit(RLIMIT_NOFILE, &_original) == 0); + _ambient = scanOpenFds(_original.rlim_cur < static_cast(65536) ? static_cast(_original.rlim_cur) + : 65536); + REQUIRE(_ambient.highest >= 0); + // A little headroom above `highest` so the fill loop below has a + // small, bounded number of fds to open rather than racing to a huge + // platform-default ceiling. + rlimit constrained = _original; + constrained.rlim_cur = static_cast(_ambient.highest + 17); + _clampedTo = constrained.rlim_cur; + REQUIRE(::setrlimit(RLIMIT_NOFILE, &constrained) == 0); + + for (;;) { + int const fd = ::open("/dev/null", O_RDONLY); + if (fd < 0) { + _fillErrno = errno; + break; + } + _dummyFds.push_back(fd); + } + // Genuinely exhausted now (confirmed by the loop above observing + // open() itself fail), regardless of any gap in what was already open. + } + FdLimitClamp(const FdLimitClamp&) = delete; + FdLimitClamp& operator=(const FdLimitClamp&) = delete; + FdLimitClamp(FdLimitClamp&&) = delete; + FdLimitClamp& operator=(FdLimitClamp&&) = delete; + ~FdLimitClamp() { + for (int const fd : _dummyFds) { + ::close(fd); + } + ::setrlimit(RLIMIT_NOFILE, &_original); + } + + /// `true` only if the fill loop stopped because the fd table was full. + /// Any other stopping errno means the syscall under test is about to run + /// against an fd table that still has headroom, so whatever it does next + /// measures nothing. + [[nodiscard]] bool exhausted() const { return _fillErrno == EMFILE; } + + /// Everything the clamp measured, on one line, for `INFO()` at a call + /// site: the ambient fd table it found, the limit it applied, how many + /// fds it had to open to fill the table, and why the fill loop stopped. + [[nodiscard]] std::string summary() const { + return "FdLimitClamp: ambient highest fd=" + std::to_string(_ambient.highest) + + " open fds=" + std::to_string(_ambient.open) + + " gap=" + std::to_string(_ambient.highest + 1 - _ambient.open) + "; RLIMIT_NOFILE soft " + + std::to_string(_original.rlim_cur) + " -> " + std::to_string(_clampedTo) + + "; filler fds opened=" + std::to_string(_dummyFds.size()) + + "; fill loop stopped on errno=" + std::to_string(_fillErrno) + " (" + + std::system_category().message(_fillErrno) + ")"; + } + +private: + rlimit _original{}; + FdScan _ambient{}; + rlim_t _clampedTo = 0; + int _fillErrno = 0; + std::vector _dummyFds; +}; + +} // namespace morph::testing diff --git a/tests/net/test_socket_server.cpp b/tests/net/test_socket_server.cpp index 4e7d851d..59efe3c3 100644 --- a/tests/net/test_socket_server.cpp +++ b/tests/net/test_socket_server.cpp @@ -1,10 +1,12 @@ // SPDX-License-Identifier: Apache-2.0 #include +#include #include #include #include +#include #include #include #include @@ -12,6 +14,7 @@ #include #include #include +#include #include #include #include @@ -22,10 +25,12 @@ #include #include #include +#include #include #include #include "../test_support.hpp" +#include "fd_limit_clamp.hpp" // ── Test model, registered process-wide (same pattern as tests/qt/test_qt_websocket.cpp) ── // Deliberately NOT in an anonymous namespace: glaze's reflection-based @@ -214,6 +219,62 @@ int highestOpenFd(int limit) { return highest; } +// Waits until `fd` reports any of `events`, or `deadline` passes. Returns +// whether it did. +bool pollUntil(int fd, short events, std::chrono::steady_clock::time_point deadline) { + for (;;) { + auto const left = + std::chrono::duration_cast(deadline - std::chrono::steady_clock::now()); + if (left.count() <= 0) { + return false; + } + pollfd pfd{.fd = fd, .events = events, .revents = 0}; + int const rc = ::poll(&pfd, 1, static_cast(left.count())); + if (rc > 0) { + return true; + } + if (rc < 0 && errno != EINTR) { + return false; + } + } +} + +// Sends a WebSocket Upgrade request on `fd`, a non-blocking socket already +// connecting to 127.0.0.1:port, and reports whether the server answered it +// with `101 Switching Protocols` before `budget` ran out. Never blocks past +// the budget: a server whose accept loop has stopped leaves the connection +// in the listen backlog, where nothing will ever answer it. +bool upgradeAnsweredWithin(int fd, LoopbackPort port, std::chrono::milliseconds budget) { + auto const deadline = std::chrono::steady_clock::now() + budget; + if (!pollUntil(fd, POLLOUT, deadline)) { + return false; + } + int soError = 0; + socklen_t len = sizeof(soError); + if (::getsockopt(fd, SOL_SOCKET, SO_ERROR, &soError, &len) != 0 || soError != 0) { + return false; + } + morph::net::detail::ParsedWsUrl const url{.host = "127.0.0.1", .port = port.value, .path = "/"}; + std::string const request = + morph::net::detail::buildClientHandshakeRequest(url, morph::net::detail::generateClientKey()); + if (::send(fd, request.data(), request.size(), MSG_NOSIGNAL) != static_cast(request.size())) { + return false; + } + std::string response; + while (response.size() < std::string_view{"HTTP/1.1 101"}.size()) { + if (!pollUntil(fd, POLLIN, deadline)) { + return false; + } + std::array buf{}; + ssize_t const got = ::recv(fd, buf.data(), buf.size(), 0); + if (got <= 0) { + return false; + } + response.append(buf.data(), static_cast(got)); + } + return response.starts_with("HTTP/1.1 101"); +} + } // namespace TEST_CASE("SocketServer: register -> execute -> reply round trip with a raw client", "[net][socket_server]") { @@ -793,11 +854,12 @@ TEST_CASE("SocketServer: acceptLoop's _closing checks observe a concurrent close TEST_CASE("SocketServer: acceptLoop retries when a pending connection is aborted before accept() runs", "[net][socket_server]") { - // tryAccept() returning nullopt (EAGAIN/EWOULDBLOCK/ECONNABORTED) means - // "the pending connection went away before we took it" -- never exercised - // elsewhere. A single sequential connect-then-abort essentially always - // loses this race on this machine: the listener's own accept() is simpler - // and faster than our own connect()+setsockopt()+close() round trip + // tryAccept() returning nullopt (EAGAIN/EWOULDBLOCK, or a connection + // that failed while pending) means "the pending connection went away + // before we took it" -- never exercised elsewhere. A single sequential + // connect-then-abort essentially always loses this race on this machine: + // the listener's own accept() is simpler and faster than our own + // connect()+setsockopt()+close() round trip // (confirmed empirically -- 0/150 with one real, fully-established // TcpSocket::connect() per attempt, sequential or threaded). Firing many // *non-blocking* connects back-to-back on one thread (skipping both the @@ -835,27 +897,35 @@ TEST_CASE("SocketServer: acceptLoop retries when a pending connection is aborted REQUIRE(reg.kind == "ok"); } -// ── Undocumented extra beyond the findings doc's 15 numbered gaps ────────── -// acceptLoop()'s `catch` around `tryAccept()` itself (lines 182-186, "listener -// is unusable (server closing)") is a *different* branch from finding #10's -// nullopt case just above: tryAccept() only returns nullopt for -// EAGAIN/EWOULDBLOCK/ECONNABORTED, but *throws* for any other accept(2) -// failure. None of the findings doc's 15 write-ups mention this catch. Unlike -// the OS-scheduling races above, EMFILE is deterministic: accept(2) checks -// the process's fd-table limit inside the kernel before returning a new fd, -// independent of any TCP-level timing -- exactly finding #1's fault-injection -// technique (RLIMIT_NOFILE), reused here against accept() instead of pipe(). -TEST_CASE("SocketServer: acceptLoop's tryAccept() exception path is caught when accept() runs out of fds", - "[net][socket_server]") { +// Running out of descriptors is a condition of the moment, not of the +// listener: `accept(2)` answers EMFILE while the process is at its limit and +// succeeds again once a descriptor is free. So the accept loop backs off and +// keeps accepting, and a connection that queued while the process was +// exhausted is served once it is not. EMFILE is deterministic here, unlike +// the scheduling races above: the kernel checks the fd-table limit inside +// `accept(2)`, independent of any TCP timing, so RLIMIT_NOFILE produces it on +// demand. +TEST_CASE("SocketServer: an accept loop that ran out of fds serves again once it has them", "[net][socket_server]") { + // The loop says it is backing off, which is also what tells this test + // that it has met the exhaustion rather than merely not got there yet. + auto sawExhaustion = std::make_shared>(false); + morph::log::ScopedLoggerOverride const logGuard{ + [sawExhaustion](morph::log::LogLevel level, std::string_view msg) { + if (level == morph::log::LogLevel::warn && + msg.starts_with("[net::SocketServer] accept failed, backing off")) { + sawExhaustion->store(true); + } + }, + morph::log::LogLevel::warn}; + morph::exec::ThreadPoolExecutor pool{1}; auto server = std::make_shared(pool); morph::net::SocketServer wsServer{*server, 0}; REQUIRE(wsServer.listen()); std::uint16_t const port = wsServer.port(); - // Pre-create several plain (not-yet-connected) sockets *before* touching - // the limit -- ::socket() needs fd headroom, which the constrained limit - // below would refuse. + // Created before the clamp: ::socket() needs a new fd, a later + // ::connect() on an existing one does not. constexpr int kPending = 5; std::vector clientFds; clientFds.reserve(kPending); @@ -865,60 +935,27 @@ TEST_CASE("SocketServer: acceptLoop's tryAccept() exception path is caught when clientFds.push_back(fd); } - rlimit original{}; - REQUIRE(::getrlimit(RLIMIT_NOFILE, &original) == 0); - rlim_t const scanLimit = - original.rlim_cur < static_cast(65536) ? original.rlim_cur : static_cast(65536); - int const highest = highestOpenFd(static_cast(scanLimit)); - REQUIRE(highest >= 0); - - struct RlimitGuard { - explicit RlimitGuard(rlimit savedIn) : saved{savedIn} {} - RlimitGuard(const RlimitGuard&) = delete; - RlimitGuard& operator=(const RlimitGuard&) = delete; - RlimitGuard(RlimitGuard&&) = delete; - RlimitGuard& operator=(RlimitGuard&&) = delete; - ~RlimitGuard() { ::setrlimit(RLIMIT_NOFILE, &saved); } - - rlimit saved; - } const guard{original}; - - // Zero headroom for a *new* fd. Note the pre-created sockets above are - // *not* connected yet, so nothing is in the backlog for the accept loop - // to race us for -- unlike TcpSocket::connect(), a raw ::connect() on an - // already-open fd needs no new fd of its own, so it is safe to issue - // *after* the constraint is already active, closing the window that - // defeated the naive "connect first, then constrain" ordering (the - // accept loop, running continuously in the background, would otherwise - // almost always drain the backlog before this thread even finishes - // computing the rlimit to set). - rlimit constrained = original; - constrained.rlim_cur = static_cast(highest + 1); - REQUIRE(::setrlimit(RLIMIT_NOFILE, &constrained) == 0); - - for (int const fd : clientFds) { - beginConnect(fd, LoopbackPort{port}); + // Phase 1: the loop meets EMFILE and reports it. The clamp fills every + // free descriptor rather than only lowering the limit, so accept(2) + // cannot take a hole below the highest open fd; the connects are issued + // only once it holds, so the loop cannot take one before it does. + bool reported = false; + { + morph::testing::FdLimitClamp const clamp; + INFO(clamp.summary()); + REQUIRE(clamp.exhausted()); + for (int const fd : clientFds) { + beginConnect(fd, LoopbackPort{port}); + } + reported = morph::testing::waitUntil([sawExhaustion] { return sawExhaustion->load(); }, + morph::testing::WaitBudget{std::chrono::seconds{5}}); } - // Let the accept loop's *own* poll()/tryAccept() cycle discover and fail - // on the pending connections on its own -- deliberately not calling - // close() yet. close()'s _wakeup.signal() would otherwise race the - // still-pending connections' own readiness for which one poll() reports - // first (acceptLoop() checks the wake fd's revents before the - // listener's, so it wins whenever both are ready), and calling close() immediately - // after firing the connects lets it win essentially every time, returning - // via the ordinary "close() signalled" path before tryAccept() is ever - // attempted. There is no public signal to poll for "the accept thread - // gave up" instead, so this waits a fixed, generous duration -- EMFILE is - // a persistent *state* here (the limit stays constrained the whole time), - // not a narrow instant, so any reasonable wait reaches it. - std::this_thread::sleep_for(std::chrono::milliseconds{300}); - - // acceptLoop() should have observed EMFILE and returned on its own by - // now (see the comment in socket_server.hpp: "listener is unusable"). - // Confirm the accept thread actually terminated, rather than assuming it: - // close() should complete promptly since nothing is left parked in - // poll(). + // Phase 2: with descriptors available again, a connection that arrived + // during the exhaustion completes its handshake. + bool const served = upgradeAnsweredWithin(clientFds.front(), LoopbackPort{port}, std::chrono::milliseconds{5000}); + + // close() still ends a loop that has been backing off, promptly. auto closed = std::make_shared>(false); std::thread closer([&wsServer, closed] { wsServer.close(); @@ -926,17 +963,17 @@ TEST_CASE("SocketServer: acceptLoop's tryAccept() exception path is caught when }); bool const finishedPromptly = morph::testing::waitUntil([closed] { return closed->load(); }, morph::testing::WaitBudget{std::chrono::seconds{5}}); - - ::setrlimit(RLIMIT_NOFILE, &original); // restore before any further fd use, including cleanup below for (int const fd : clientFds) { ::close(fd); } - if (!finishedPromptly) { closer.detach(); - FAIL("close() did not complete within 5s after tryAccept() should have hit EMFILE and exited the loop"); + FAIL("close() did not complete within 5s of a loop that had been backing off"); } closer.join(); + + CHECK(reported); + CHECK(served); } TEST_CASE("SocketServer: sendText() catches a send failure when the peer resets mid-reply", "[net][socket_server]") { diff --git a/tests/net/test_tcp_socket.cpp b/tests/net/test_tcp_socket.cpp index 5b72059b..42bd7a1f 100644 --- a/tests/net/test_tcp_socket.cpp +++ b/tests/net/test_tcp_socket.cpp @@ -6,6 +6,7 @@ #include #include +#include #include #include #include @@ -21,6 +22,8 @@ #include #include +#include "fd_limit_clamp.hpp" + using morph::net::detail::TcpSocket; namespace { @@ -40,129 +43,7 @@ bool makeNonBlocking(int rawFd) { } // namespace -namespace { - -// What a scan of this process's fd table found, up to some ceiling: the -// highest fd open, and how many fds in that range are open at all. Same -// technique as tests/net/test_socket_server.cpp's own `highestOpenFd()`, -// with the count added -- `highest + 1 - open` is the size of the gap that -// `FdLimitClamp` exists to fill, and it is *reported* rather than assumed -// away. -struct FdScan { - int highest = -1; - int open = 0; -}; - -FdScan scanOpenFds(int limit) { - FdScan scan; - for (int fd = 0; fd < limit; ++fd) { - if (::fcntl(fd, F_GETFD) != -1) { - scan.highest = fd; - ++scan.open; - } - } - return scan; -} - -// RAII guard: forces the fd table to genuinely zero headroom and restores the -// original state on destruction. -// -// Lowering RLIMIT_NOFILE to `highestOpenFd()+1` (as -// tests/net/test_socket_server.cpp's own EMFILE tests do) assumes fd -// allocation so far has been gap-free -- true there, but not safe to assume -// in this file: an earlier TEST_CASE here can open and close a *lower*- -// numbered fd (e.g. a transient connect() attempt) while a *higher*-numbered -// one stays permanently open (this process's getaddrinfo() call opens a -// long-lived resolver connection on first use, on this platform), leaving a -// gap below the computed "highest". The very next fd-allocating syscall then -// silently reuses that gap -- allowed by RLIMIT_NOFILE, since the gap's fd -// number is still under the limit -- and the intended EMFILE never fires. -// Confirmed empirically: the naive `highest+1` version of this guard passed -// this file's [tcp] tag roughly half the time and failed the other half. -// -// This version is self-verifying instead of computed: it lowers the limit, -// then actually opens `/dev/null` repeatedly until `open()` itself fails, -// filling any such gap for real rather than assuming there isn't one. Only -// once real exhaustion has been *observed* does the syscall under test run. -// -// This test fails under concurrent machine load if the clamp does not hold, so -// the clamp *says* what it measured rather than leaving the next reader -// guessing. `exhausted()` and `summary()` are that: every call site asserts -// `exhausted()` before the syscall under test -- so a clamp that did not bite -// fails on its own terms instead of being mistaken for a bug in `accept()` -- -// and `INFO(summary())` puts the whole measurement (ambient fd table, limit -// applied, fds it took to fill, and the errno the fill loop stopped on) into -// the failure output of whichever assertion follows. -// -// The `exhausted()` check is deliberately *not* a REQUIRE inside this -// constructor: a Catch2 assertion failure there throws out of a half-built -// object, whose destructor never runs, leaving the process clamped and the -// filler fds leaked for every test after it. -class FdLimitClamp { -public: - FdLimitClamp() { - REQUIRE(::getrlimit(RLIMIT_NOFILE, &_original) == 0); - _ambient = scanOpenFds(_original.rlim_cur < static_cast(65536) ? static_cast(_original.rlim_cur) - : 65536); - REQUIRE(_ambient.highest >= 0); - // A little headroom above `highest` so the fill loop below has a - // small, bounded number of fds to open rather than racing to a huge - // platform-default ceiling. - rlimit constrained = _original; - constrained.rlim_cur = static_cast(_ambient.highest + 17); - _clampedTo = constrained.rlim_cur; - REQUIRE(::setrlimit(RLIMIT_NOFILE, &constrained) == 0); - - for (;;) { - int const fd = ::open("/dev/null", O_RDONLY); - if (fd < 0) { - _fillErrno = errno; - break; - } - _dummyFds.push_back(fd); - } - // Genuinely exhausted now (confirmed by the loop above observing - // open() itself fail), regardless of any gap in what was already open. - } - FdLimitClamp(const FdLimitClamp&) = delete; - FdLimitClamp& operator=(const FdLimitClamp&) = delete; - FdLimitClamp(FdLimitClamp&&) = delete; - FdLimitClamp& operator=(FdLimitClamp&&) = delete; - ~FdLimitClamp() { - for (int const fd : _dummyFds) { - ::close(fd); - } - ::setrlimit(RLIMIT_NOFILE, &_original); - } - - /// `true` only if the fill loop stopped because the fd table was full. - /// Any other stopping errno means the syscall under test is about to run - /// against an fd table that still has headroom, so whatever it does next - /// measures nothing. - [[nodiscard]] bool exhausted() const { return _fillErrno == EMFILE; } - - /// Everything the clamp measured, on one line, for `INFO()` at a call - /// site: the ambient fd table it found, the limit it applied, how many - /// fds it had to open to fill the table, and why the fill loop stopped. - [[nodiscard]] std::string summary() const { - return "FdLimitClamp: ambient highest fd=" + std::to_string(_ambient.highest) + - " open fds=" + std::to_string(_ambient.open) + - " gap=" + std::to_string(_ambient.highest + 1 - _ambient.open) + "; RLIMIT_NOFILE soft " + - std::to_string(_original.rlim_cur) + " -> " + std::to_string(_clampedTo) + - "; filler fds opened=" + std::to_string(_dummyFds.size()) + - "; fill loop stopped on errno=" + std::to_string(_fillErrno) + " (" + - std::system_category().message(_fillErrno) + ")"; - } - -private: - rlimit _original{}; - FdScan _ambient{}; - rlim_t _clampedTo = 0; - int _fillErrno = 0; - std::vector _dummyFds; -}; - -} // namespace +using morph::testing::FdLimitClamp; TEST_CASE("TcpSocket: listen on port 0 gets an OS-assigned port", "[net][tcp]") { auto listener = TcpSocket::listen(0); @@ -564,6 +445,75 @@ TEST_CASE("TcpSocket::accept: throws when accept() itself runs out of file descr REQUIRE_THROWS_AS(listener.accept(), std::runtime_error); } +TEST_CASE("TcpSocket::tryAccept: running out of file descriptors throws the errno an accept loop backs off on", + "[net][tcp]") { + // SocketServer's accept loop tells a process that is out of descriptors + // from a listener that is gone by the errno this carries, so it has to + // arrive intact rather than only as text. + auto listener = TcpSocket::listen(0); + std::uint16_t const port = listener.boundPort(); + REQUIRE(listener.setNonBlocking()); + auto clientSide = TcpSocket::connect("127.0.0.1", port, std::chrono::milliseconds{2000}); + + // Queued before the clamp, as in the accept() case above, so the accept + // below meets EMFILE rather than an empty queue. + pollfd listenerReady{}; + listenerReady.fd = listener.nativeHandle(); + listenerReady.events = POLLIN; + int const queued = ::poll(&listenerReady, 1, 5000); + INFO("poll() on the listener returned " << queued << " (revents=" << listenerReady.revents << ")"); + REQUIRE(queued == 1); + + int thrownErrno = 0; + { + FdLimitClamp const clamp; + INFO(clamp.summary()); + REQUIRE(clamp.exhausted()); + try { + static_cast(listener.tryAccept()); + } catch (const std::system_error& failure) { + thrownErrno = failure.code().value(); + } + } + CHECK(thrownErrno == EMFILE); + CHECK(morph::net::detail::acceptFailureOf(thrownErrno) == morph::net::detail::AcceptFailure::Exhausted); +} + +TEST_CASE("TcpSocket: every accept errno is sorted by one row and an unnamed one ends the loop", "[net][tcp]") { + using morph::net::detail::AcceptErrorRow; + using morph::net::detail::AcceptFailure; + using morph::net::detail::acceptFailureOf; + using morph::net::detail::kAcceptErrors; + + for (auto const& row : kAcceptErrors) { + CAPTURE(row.err); + CHECK(std::ranges::count(kAcceptErrors, row.err, &AcceptErrorRow::err) == 1); + } + + // What Linux accept(2) hands back for a connection that failed while + // pending, and a firewall's refusal of one connection: nothing to take, + // like EAGAIN. + for (int const err : {ENETDOWN, EPROTO, ENOPROTOOPT, EHOSTDOWN, EHOSTUNREACH, EOPNOTSUPP, ENETUNREACH, EPERM}) { + CAPTURE(err); + CHECK(acceptFailureOf(err) == AcceptFailure::NoConnection); + } +#ifdef ENONET + CHECK(acceptFailureOf(ENONET) == AcceptFailure::NoConnection); +#endif + for (int const err : {EMFILE, ENFILE, ENOBUFS, ENOMEM}) { + CAPTURE(err); + CHECK(acceptFailureOf(err) == AcceptFailure::Exhausted); + } + for (int const err : {EBADF, ENOTSOCK, EINVAL, EFAULT}) { + CAPTURE(err); + CHECK(acceptFailureOf(err) == AcceptFailure::ListenerUnusable); + } + // No row, so the default: ECONNABORTED deliberately, and a value nothing + // could answer. + CHECK(acceptFailureOf(ECONNABORTED) == AcceptFailure::ListenerUnusable); + CHECK(acceptFailureOf(0) == AcceptFailure::ListenerUnusable); +} + TEST_CASE("TcpSocket::recvSome: throws for a real socket error distinct from ECONNRESET", "[net][tcp]") { auto listener = TcpSocket::listen(0); std::uint16_t const port = listener.boundPort();