Subject: Re: [RFC PATCH] daemon: add a self pipe to trigger reaping of children

From: Phillip Wood <[email-masked]>
Date: Fri, 27 Jun 2025 09:38:36 +0100

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 <[email-masked]>

s/=/-/

> Signed-off-by: Carlo Marcelo Arenas Belón <[email-masked]>
> ---
> 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.

> 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.

>   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.

> +
> +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.

>   }
>   
>   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.

> +			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.

> +
> +			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.

		}

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

> +				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--;
>   			}
>   		}
>   	}