Re: [PATCH 1/2] daemon: use timeout for uninterruptible poll

From: Kim Gybels <kgybels@infogroep.be>
Date: Apr 19, 2018, 21:33 UTC
Message-ID: <20180419213322.GA19500@infogroep.be>
In-Reply-To: <xmqqy3hkfais.fsf@gitster-ct.c.googlers.com>

On (19/04/18 06:51), Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> > In other words, you scolded Kim for something that this patch did not
> > introduce, but which was already there.

I didn't feel scolded, just Junio raising a concern about maintainability of the code.

Show 19 quoted lines

> > Unless I am misunderstanding violently what you say, that is, in which
> > case I would like to ask for a clarification why this patch (which does
> > not change a thing unless NO_POLL is defined!) must be rejected, and while
> > at it, I would like to ask you how introducing a layer of indirection with
> > a full new function that is at least moderately misleading (as it would be
> > named xpoll() despite your desire that it should do things that poll()
> > does *not* do) would be preferable to this here patch that changes but a
> > few lines to introduce a regular heartbeat check for platforms that
> 
> Our xwrite() and other xfoo() are to "fix" undesirable aspect of the
> underlying pure POSIX API to make it more suitable for our codebase.
> When pure POSIX poll() that requires the implementing or emulating
> platform pays attention to the children being waited on is not
> appropriate for the codepath we are using (i.e. the place where the
> patch is touching), it would be in line to introduce a "fixed" API
> that allows us to pass that information, so that we can build on top
> of that abstraction that is *not* pure POSIX abstraction, no?  After
> all, you are the one who constantly whine that Git is implemented on
> POSIX API and it is inconvenient for other platforms.

There is another issue with the existing code that this new "xpoll" will need to take into account. If a SIGCHLD arrives between the call to check_dead_children and poll, the poll will not be interupted by it, resulting in the child not being reaped until another child terminates or a client connects. Currently, the effect is just a zombie process for a longer time, however, the proposed patch (daemon: graceful shutdown of client connection) relies on the cleanup to close the client connection.

When I have time, I will reroll including a change to ppoll.

-Kim