Re: [RFC PATCH] daemon: add a self pipe to trigger reaping of children
- From
- Phillip Wood <phillip.wood123@gmail.com>
- Date
- Jun 27, 2025, 08:38 UTC
- Message-ID
- <59087d2d-6034-44d4-9fa0-c51d4bd60683@gmail.com>
- In-Reply-To
- <20250626182432.87523-1-carenas@gmail.com>
Hi Carlo
On 26/06/2025 19:24, Carlo Marcelo Arenas Belón wrote:
> There has always been a small race condition between the time a > children status is checked, and the arrival of a SIGCHLD that > would alert us of their demise, hopefully interrupt poll().
The race was introduced by 695605b5080 (git-daemon: Simplify dead-children reaping logic, 2008-08-14). Before that children were reaped inside the signal handler so there was no race.
> To close the gap, add the reading side of a pipe to the poll and > use the signal handler to write to it for each child.
As well as closing the race this fixes the issue with poll() not being interrupted when a child exits. It also allows us to move the re-arming of the handler into the event loop which would fix the infinite recursion on AIX.
> Suggested=by: Phillip Wood <phillip.wood123@gmail.com>
s/=/-/
> Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com> > --- > Implements the "self pipe" trick Hillip suggested.
I've never been called that before :-/
> > I tried to make self healing and optional, as I also presume the race > condition it is meant to address is very unlikely.
I see this as an alternative fix for the other problems you've been working on as well.
Show 11 quoted lines
> An obvious disadvantage (at least of this implementation), is that it > actually doubles the number of events that need to be handled for each > children process on most cases (ex: when `poll()` gets interrupted) > > I suspect that if fixing that last race condition is so important with > the current foundation, it might be better to reintroduce some sort of > timeout to poll(), so that they will be cleared periodically. > > I had a prototype (only the bare minimum) that I thought was more > efficient and that would instead remove completely the need for a > signal handler which I would post (only for RFC) later.
I'm not sure injecting an fd into each child process is a good direction.
Show 13 quoted lines
> daemon.c | 55 ++++++++++++++++++++++++++++++++++++++++++++++---------
> 1 file changed, 46 insertions(+), 9 deletions(-)
>
> diff --git a/daemon.c b/daemon.c
> index d1be61fd57..d3b9421575 100644
> --- a/daemon.c
> +++ b/daemon.c
> @@ -912,14 +912,17 @@ static void handle(int incoming, struct sockaddr *addr, socklen_t addrlen)
> add_child(&cld, addr, addrlen);
> }
>
> -static void child_handler(int signo UNUSED)
> +int poll_pipe[2] = { -1, -1 };Maybe call this signal_pipe? I'm not sure what poll_pipe means.
Show 9 quoted lines
> +
> +static void child_handler(int signo)
> {
> /*
> - * Otherwise empty handler because systemcalls will get interrupted
> - * upon signal receipt
> * SysV needs the handler to be rearmed
> */
> signal(SIGCHLD, child_handler);I think from Chris' email that it is conventional to do this at the end of the handler. As I said above we could add an additional commit that moves this into the event loop to fix the infinite recursion on AIX.
> + > + if (poll_pipe[1] >= 0) > + write(poll_pipe[1], &signo, 1);
write() might fail so we should save errno around it. Conventionally one would re-try on EINTR as well though in this case the most likely reason for that is another child exiting which means the pipe would be written to anyway.
Show 11 quoted lines
> }
>
> static int set_reuse_addr(int sockfd)
> @@ -1121,20 +1124,43 @@ static void socksetup(struct string_list *listen_addr, int listen_port, struct s
> static int service_loop(struct socketlist *socklist)
> {
> struct pollfd *pfd;
> + unsigned long nfds = 1 + socklist->nr;
> +
> + ALLOC_ARRAY(pfd, nfds);
> + if (!pipe(poll_pipe)) {If we cannot create a pipe here then things have gone pretty badly wrong and I think it is unlikely we're going to be able to accept incoming connections so it would be best to die(). Relying on the signal pipe would solve the problem of children not being collected until we receive an new connection as well.
> + for (int i = 0; i < 2; i++) {The body of this loop is quite indented - I think it would be better to turn it into a function.
Show 5 quoted lines
> + int flags; > + > + flags = fcntl(poll_pipe[i], F_GETFD, 0); > + if (flags >= 0) > + fcntl(poll_pipe[i], F_SETFD, flags | FD_CLOEXEC);
I think we should probably close the pipes if we do not set FD_CLOEXEC.
Show 35 quoted lines
> +
> + flags = fcntl(poll_pipe[i], F_GETFL, 0);
> + if (flags < 0 || fcntl(poll_pipe[i], F_SETFL,
> + flags | O_NONBLOCK) == -1) {
> + close(poll_pipe[0]);
> + close(poll_pipe[1]);
> + poll_pipe[0] = poll_pipe[1] = -1;
> + break;
> + }
> + }
> + }
> + pfd[0].fd = poll_pipe[0];
> + pfd[0].events = POLLIN;
>
> - CALLOC_ARRAY(pfd, socklist->nr);
> -
> - for (size_t i = 0; i < socklist->nr; i++) {
> - pfd[i].fd = socklist->list[i];
> + for (size_t i = 1; i < nfds; i++) {
> + pfd[i].fd = socklist->list[i - 1];
> pfd[i].events = POLLIN;
> }
>
> signal(SIGCHLD, child_handler);
>
> for (;;) {
> + int nevents, scratch;
> +
> check_dead_children();
>
> - if (poll(pfd, socklist->nr, -1) < 0) {
> + if ((nevents = poll(pfd, nfds, -1)) <= 0) {
> if (errno != EINTR) {
> logerror("Poll failed, resuming: %s",
> strerror(errno));sleep(1); } continue;
We need to drain the signal pipe on EINTR. I think it would be best to do that just before calling check_dead_childern() above. We can avoid calling check_dead_children() if we don't read anything.
}
Show 7 quoted lines
> @@ -1143,7 +1169,17 @@ static int service_loop(struct socketlist *socklist)
> continue;
> }
>
> - for (size_t i = 0; i < socklist->nr; i++) {
> + if ((pfd[0].revents & POLLIN) && pfd[0].fd != -1) {
> + if (nevents == 1 && read(pfd[0].fd, &scratch, 1) > 0)There can be more than one byte to read from the signal_pipe - we should drain it until we see EAGAIN. As I said above I think it would be better to do that just before calling check_dead_children(). Here we can just decrement nevents if poll tells us the signal_pipe is readable.
Thanks
Phillip
Show 20 quoted lines
> + continue;
> + else if (!read(pfd[0].fd, &scratch, 1)) {
> + close(pfd[0].fd);
> + pfd[0].fd = -1;
> + }
> + nevents--;
> + }
> +
> + for (size_t i = 1; nevents && i < nfds; i++) {
> if (pfd[i].revents & POLLIN) {
> union {
> struct sockaddr sa;
> @@ -1165,6 +1201,7 @@ static int service_loop(struct socketlist *socklist)
> }
> }
> handle(incoming, &ss.sa, sslen);
> + nevents--;
> }
> }
> }