From 7aec313597003ca4b09f408b34b99a1bbc030cea Mon Sep 17 00:00:00 2001 From: Jade Lovelace Date: Thu, 3 Apr 2025 18:41:22 -0700 Subject: [PATCH] MonitorFdHup: introduce a test Well, I was trying to figure out https://git.lix.systems/lix-project/lix/issues/729 in which this feature is clearly just broken on macOS, but frustratingly, it seems that it *does* work, except for the daemon. um........ sure. Change-Id: Iaa962f045c16fdfa82854151c90a03e5cf0eea47 --- lix/libutil/meson.build | 1 + lix/libutil/monitor-fd.cc | 85 ++++++++++++++++++++++++++++++++ lix/libutil/monitor-fd.hh | 85 +++----------------------------- tests/unit/libutil/monitor-fd.cc | 49 ++++++++++++++++++ tests/unit/meson.build | 1 + 5 files changed, 144 insertions(+), 77 deletions(-) create mode 100644 lix/libutil/monitor-fd.cc create mode 100644 tests/unit/libutil/monitor-fd.cc diff --git a/lix/libutil/meson.build b/lix/libutil/meson.build index 22798456f..bf6c66da8 100644 --- a/lix/libutil/meson.build +++ b/lix/libutil/meson.build @@ -25,6 +25,7 @@ libutil_sources = files( 'hilite.cc', 'json-utils.cc', 'logging.cc', + 'monitor-fd.cc', 'mount.cc', 'namespaces.cc', 'position.cc', diff --git a/lix/libutil/monitor-fd.cc b/lix/libutil/monitor-fd.cc new file mode 100644 index 000000000..9f35865e3 --- /dev/null +++ b/lix/libutil/monitor-fd.cc @@ -0,0 +1,85 @@ +#include "monitor-fd.hh" + +namespace nix { + +void MonitorFdHup::runThread(int watchFd, int terminateFd) +{ + setCurrentThreadName("MonitorFdHup"); + while (!quit) { + /* Wait indefinitely until a POLLHUP occurs. */ + struct pollfd fds[2]; + fds[0].fd = watchFd; + // There is a POSIX violation on macOS: you have to listen for + // at least POLLHUP to receive HUP events for a FD. POSIX says + // this is not so, and you should just receive them regardless, + // however, as of our testing on macOS 14.5, the events do not + // get delivered in such a case. + // + // This is allegedly filed as rdar://37537852. + // + // Relevant code, which backs this up: + // https://github.com/apple-oss-distributions/xnu/blob/94d3b452840153a99b38a3a9659680b2a006908e/bsd/kern/sys_generic.c#L1751-L1758 + fds[0].events = POLLHUP; + fds[1].fd = terminateFd; + fds[1].events = POLLIN; + + auto count = poll(fds, 2, -1); + if (count == -1) { + if (errno == EINTR || errno == EAGAIN) { + // These are best dealt with by just trying again. + continue; + } else { + throw SysError("in MonitorFdHup poll()"); + } + } + /* This shouldn't happen, but can on macOS due to a bug. + See rdar://37550628. + + This may eventually need a delay or further + coordination with the main thread if spinning proves + too harmful. + */ + if (count == 0) { + continue; + } + if (fds[0].revents & POLLHUP) { + callback(); + break; + } + // No reason to actually look at the pipe FD if that's what + // woke us, the only thing that actually matters is the quit + // flag. + if (quit) { + break; + } + // On macOS, it is possible (although not observed on macOS + // 14.5) that in some limited cases on buggy kernel versions, + // all the non-POLLHUP events for the socket get delivered. + // Sleeping avoids pointlessly spinning a thread on those. + // + // N.B. excessive delay on this can cause the daemon connection + // thread to live longer than the client and lead to + // synchronization problems if clients assume that the server + // thread has released its temporary gc roots, etc. + // See https://github.com/NixOS/nix/pull/12714#discussion_r2009265904 + usleep(1'000); + } +} + +MonitorFdHup::MonitorFdHup(int fd, std::function callback) : callback(callback) +{ + terminatePipe.create(); + int terminateFd = terminatePipe.readSide.get(); + thread = std::thread([this, fd, terminateFd]() { this->runThread(fd, terminateFd); }); +}; + +MonitorFdHup::~MonitorFdHup() +{ + quit = true; + // Poke the thread out of its poll wait + writeFull(terminatePipe.writeSide.get(), "*", false); + if (thread.joinable()) { + thread.join(); + } +} +} diff --git a/lix/libutil/monitor-fd.hh b/lix/libutil/monitor-fd.hh index 095717e09..97a914ab9 100644 --- a/lix/libutil/monitor-fd.hh +++ b/lix/libutil/monitor-fd.hh @@ -1,5 +1,6 @@ #pragma once -///@file +///@file Observes a file descriptor for hang-up events and notifies a +/// callback. #include #include @@ -15,7 +16,7 @@ namespace nix { - +/** Observes a file descriptor for hang-up events and notifies a callback. */ class MonitorFdHup { private: @@ -25,84 +26,14 @@ private: */ Pipe terminatePipe; std::atomic_bool quit = false; + std::function callback; + + void runThread(int watchFd, int terminateFd); public: - MonitorFdHup(int fd) - { - terminatePipe.create(); - auto &quit_ = this->quit; - int terminateFd = terminatePipe.readSide.get(); - thread = std::thread([fd, terminateFd, &quit_]() { - setCurrentThreadName("MonitorFdHup"); - while (!quit_) { - /* Wait indefinitely until a POLLHUP occurs. */ - struct pollfd fds[2]; - fds[0].fd = fd; - // There is a POSIX violation on macOS: you have to listen for - // at least POLLHUP to receive HUP events for a FD. POSIX says - // this is not so, and you should just receive them regardless, - // however, as of our testing on macOS 14.5, the events do not - // get delivered in such a case. - // - // This is allegedly filed as rdar://37537852. - // - // Relevant code, which backs this up: - // https://github.com/apple-oss-distributions/xnu/blob/94d3b452840153a99b38a3a9659680b2a006908e/bsd/kern/sys_generic.c#L1751-L1758 - fds[0].events = POLLHUP; - fds[1].fd = terminateFd; - fds[1].events = POLLIN; + MonitorFdHup(int fd, std::function callback = nix::triggerInterrupt); - auto count = poll(fds, 2, -1); - if (count == -1) { - if (errno == EINTR || errno == EAGAIN) { - // These are best dealt with by just trying again. - continue; - } else { - throw SysError("in MonitorFdHup poll()"); - } - } - /* This shouldn't happen, but can on macOS due to a bug. - See rdar://37550628. - - This may eventually need a delay or further - coordination with the main thread if spinning proves - too harmful. - */ - if (count == 0) continue; - if (fds[0].revents & POLLHUP) { - triggerInterrupt(); - break; - } - // No reason to actually look at the pipe FD if that's what - // woke us, the only thing that actually matters is the quit - // flag. - if (quit_) { - break; - } - // On macOS, it is possible (although not observed on macOS - // 14.5) that in some limited cases on buggy kernel versions, - // all the non-POLLHUP events for the socket get delivered. - // Sleeping avoids pointlessly spinning a thread on those. - // - // N.B. excessive delay on this can cause the daemon connection - // thread to live longer than the client and lead to - // synchronization problems if clients assume that the server - // thread has released its temporary gc roots, etc. - // See https://github.com/NixOS/nix/pull/12714#discussion_r2009265904 - usleep(1'000); - } - }); - }; - - ~MonitorFdHup() - { - quit = true; - // Poke the thread out of its poll wait - writeFull(terminatePipe.writeSide.get(), "*", false); - if (thread.joinable()) { - thread.join(); - } - } + ~MonitorFdHup(); }; diff --git a/tests/unit/libutil/monitor-fd.cc b/tests/unit/libutil/monitor-fd.cc new file mode 100644 index 000000000..b74c24bb1 --- /dev/null +++ b/tests/unit/libutil/monitor-fd.cc @@ -0,0 +1,49 @@ +#include "lix/libutil/monitor-fd.hh" +#include "lix/libutil/error.hh" +#include "lix/libutil/file-descriptor.hh" +#include +#include +#include +#include + +using namespace std::literals::chrono_literals; + +namespace nix { + +TEST(MonitorFdHup, works) +{ + int socks[2]; + int rv = socketpair(AF_UNIX, SOCK_STREAM, 0, socks); + if (rv) throw SysError("socketpair"); + auto sock1 = AutoCloseFD{socks[0]}; + auto sock2 = AutoCloseFD{socks[1]}; + + std::promise called; + + MonitorFdHup monitor(sock1.get(), [&called]() { + called.set_value(); + }); + + sock2.close(); + + // 30 seconds should certainly do it. + called.get_future().wait_for(10s); +} + +// Ensures that destroying the MonitorFdHup causes it to actually go away. +TEST(MonitorFdHup, destroys_safely) +{ + int socks[2]; + int rv = socketpair(AF_UNIX, SOCK_STREAM, 0, socks); + if (rv) throw SysError("socketpair"); + auto sock1 = AutoCloseFD{socks[0]}; + auto sock2 = AutoCloseFD{socks[1]}; + + { + MonitorFdHup monitor(sock1.get(), []() { + abort(); + }); + } +} + +} diff --git a/tests/unit/meson.build b/tests/unit/meson.build index 9da239e81..39828a074 100644 --- a/tests/unit/meson.build +++ b/tests/unit/meson.build @@ -62,6 +62,7 @@ libutil_tests_sources = files( 'libutil/json-utils.cc', 'libutil/logging.cc', 'libutil/lru-cache.cc', + 'libutil/monitor-fd.cc', 'libutil/paths-setting.cc', 'libutil/pool.cc', 'libutil/references.cc',