mirror of
https://github.com/NixOS/nix
synced 2025-06-25 19:01:16 +02:00
MonitorFdHup
: replace pthread_cancel
trick with a notification pipe
On https://github.com/NixOS/nix/issues/8946, we faced a surprising behaviour wrt. exception when using pthread_cancel. In a nutshell when a thread is inside a catch block and it's getting pthread_cancel by another one, then the original exception is bubbled up and crashes the process. We now poll on the notification pipe from the thread and exit when the main thread closes its end. This solution does not exhibit surprising behaviour wrt. exceptions. Co-authored-by: Mic92 <joerg@thalheim.io> Fixes https://github.com/NixOS/nix/issues/8946 See also Lix https://gerrit.lix.systems/c/lix/+/1605 which is very similar by coincidence. Pulled a comment from that.
This commit is contained in:
parent
cb95791198
commit
1c636284a3
2 changed files with 49 additions and 11 deletions
18
src/libutil-tests/monitorfdhup.cc
Normal file
18
src/libutil-tests/monitorfdhup.cc
Normal file
|
@ -0,0 +1,18 @@
|
||||||
|
#include "util.hh"
|
||||||
|
#include "monitor-fd.hh"
|
||||||
|
|
||||||
|
#include <sys/file.h>
|
||||||
|
#include <gtest/gtest.h>
|
||||||
|
|
||||||
|
namespace nix {
|
||||||
|
TEST(MonitorFdHup, shouldNotBlock)
|
||||||
|
{
|
||||||
|
Pipe p;
|
||||||
|
p.create();
|
||||||
|
{
|
||||||
|
// when monitor gets destroyed it should cancel the
|
||||||
|
// background thread and do not block
|
||||||
|
MonitorFdHup monitor(p.readSide.get());
|
||||||
|
}
|
||||||
|
}
|
||||||
|
}
|
|
@ -18,27 +18,45 @@ class MonitorFdHup
|
||||||
{
|
{
|
||||||
private:
|
private:
|
||||||
std::thread thread;
|
std::thread thread;
|
||||||
|
Pipe notifyPipe;
|
||||||
|
|
||||||
public:
|
public:
|
||||||
MonitorFdHup(int fd)
|
MonitorFdHup(int fd)
|
||||||
{
|
{
|
||||||
thread = std::thread([fd]() {
|
notifyPipe.create();
|
||||||
|
thread = std::thread([this, fd]() {
|
||||||
while (true) {
|
while (true) {
|
||||||
/* Wait indefinitely until a POLLHUP occurs. */
|
|
||||||
constexpr size_t num_fds = 1;
|
|
||||||
struct pollfd fds[num_fds] = {
|
|
||||||
{
|
|
||||||
.fd = fd,
|
|
||||||
.events =
|
|
||||||
/* Polling for no specific events (i.e. just waiting
|
/* Polling for no specific events (i.e. just waiting
|
||||||
for an error/hangup) doesn't work on macOS
|
for an error/hangup) doesn't work on macOS
|
||||||
anymore. So wait for read events and ignore
|
anymore. So wait for read events and ignore
|
||||||
them. */
|
them. */
|
||||||
|
// FIXME(jade): we have looked at the XNU kernel code and as
|
||||||
|
// far as we can tell, the above is bogus. It should be the
|
||||||
|
// case that the previous version of this and the current
|
||||||
|
// version are identical: waiting for POLLHUP and POLLRDNORM in
|
||||||
|
// the kernel *should* be identical.
|
||||||
|
// https://github.com/apple-oss-distributions/xnu/blob/94d3b452840153a99b38a3a9659680b2a006908e/bsd/kern/sys_generic.c#L1751-L1758
|
||||||
|
//
|
||||||
|
// So, this needs actual testing and we need to figure out if
|
||||||
|
// this is actually bogus.
|
||||||
|
short hangup_events =
|
||||||
#ifdef __APPLE__
|
#ifdef __APPLE__
|
||||||
POLLRDNORM,
|
POLLRDNORM
|
||||||
#else
|
#else
|
||||||
0,
|
0
|
||||||
#endif
|
#endif
|
||||||
|
;
|
||||||
|
|
||||||
|
/* Wait indefinitely until a POLLHUP occurs. */
|
||||||
|
constexpr size_t num_fds = 2;
|
||||||
|
struct pollfd fds[num_fds] = {
|
||||||
|
{
|
||||||
|
.fd = fd,
|
||||||
|
.events = hangup_events,
|
||||||
|
},
|
||||||
|
{
|
||||||
|
.fd = notifyPipe.readSide.get(),
|
||||||
|
.events = hangup_events,
|
||||||
},
|
},
|
||||||
};
|
};
|
||||||
|
|
||||||
|
@ -48,7 +66,6 @@ public:
|
||||||
continue;
|
continue;
|
||||||
throw SysError("failed to poll() in MonitorFdHup");
|
throw SysError("failed to poll() in MonitorFdHup");
|
||||||
}
|
}
|
||||||
|
|
||||||
/* This shouldn't happen, but can on macOS due to a bug.
|
/* This shouldn't happen, but can on macOS due to a bug.
|
||||||
See rdar://37550628.
|
See rdar://37550628.
|
||||||
|
|
||||||
|
@ -62,6 +79,9 @@ public:
|
||||||
unix::triggerInterrupt();
|
unix::triggerInterrupt();
|
||||||
break;
|
break;
|
||||||
}
|
}
|
||||||
|
if (fds[1].revents & POLLHUP) {
|
||||||
|
break;
|
||||||
|
}
|
||||||
/* This will only happen on macOS. We sleep a bit to
|
/* This will only happen on macOS. We sleep a bit to
|
||||||
avoid waking up too often if the client is sending
|
avoid waking up too often if the client is sending
|
||||||
input. */
|
input. */
|
||||||
|
@ -72,7 +92,7 @@ public:
|
||||||
|
|
||||||
~MonitorFdHup()
|
~MonitorFdHup()
|
||||||
{
|
{
|
||||||
pthread_cancel(thread.native_handle());
|
close(notifyPipe.writeSide.get());
|
||||||
thread.join();
|
thread.join();
|
||||||
}
|
}
|
||||||
};
|
};
|
||||||
|
|
Loading…
Add table
Add a link
Reference in a new issue