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

From: Carlo Marcelo Arenas Belón <carenas@gmail.com>
Date: Jun 28, 2025, 06:19 UTC
Message-ID: <zy6xtfqnncs3kuipgvdb7jiu7ynodbf7mld4r2ojy3jwkkthm6@jtnzecikxda2>
In-Reply-To: <59087d2d-6034-44d4-9fa0-c51d4bd60683@gmail.com>

On Fri, Jun 27, 2025 at 09:38:36AM -0800, Phillip Wood wrote:

Show 8 quoted lines

> 
> On 26/06/2025 19:24, Carlo Marcelo Arenas Belón wrote:
> > 
> > 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.

I don't either, but frankly don't think it is as much of an issue, if we consider that all the children are known internal processes we also own.

Was hoping that by producing a working (albeit ugly in purpose so changes to the current code would be minimized) prototype, we could have a discussion of the potential issues/benefits that would be a little more verbose.

Indeed I posted this an the other series both as RFC, and as replies of each other hoping to give them both a chance to be evaluated as alternatives together.

Show 23 quoted lines

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

It really depends on which flags the signal was expected to use. For maximum portability it would seem better to have it at the beginning to minimize the inherent race condition from when SA_RESETHAND is on effect (which was more of a SysV signal() tradition).

Will move it to the end, regardless, as it won't be needed once the call is setup using sigaction().

> As I said above we could add an additional commit that moves
> this into the event loop to fix the infinite recursion on AIX.

Not sure I understant what you mean by this. I think Chris made clear that the AIX issue comes from the handler being expected to do the wait() calls, so the only way to "solve" it would be to move the `check_dead_children()` logic back to the handler (which will require a lot more changes), using sigaction() seems like a simpler solution.

Show 8 quoted lines

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

EINTR shouldn't be possible here from SIGCHLD, because that signal is blocked here, I wrote it this way because it was only meant to be used as a fallback to the EINTR being triggered in poll() and so it wouldn't matter if it was lost (for example because of a SIGPIPE).

Once we move to sigaction, SIGPIPE could be added to the mask for this handler, but that code can't be implemented on the current base.

Show 14 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().

True, but since this is "optional", there is no harm on letting this continue even in failure.

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

Why?, worst case is that we leak a pipe to the children, that wouldn't know of it and woule be able to work either way.

Carlo