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

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Apr 18, 2018, 21:07 UTC
Message-ID: <nycvar.QRO.7.76.6.1804182251070.4241@ZVAVAG-6OXH6DA.rhebcr.pbec.zvpebfbsg.pbz>
In-Reply-To: <xmqq36zw16gv.fsf@gitster-ct.c.googlers.com>

Hi Junio,

On Mon, 16 Apr 2018, Junio C Hamano wrote:

Show 9 quoted lines

> Kim Gybels <kgybels@infogroep.be> writes:
> 
> > The poll provided in compat/poll.c is not interrupted by receiving
> > SIGCHLD. Use a timeout for cleaning up dead children in a timely manner.
> 
> I think you identified the problem and diagnosed it correctly, but I
> find that the change proposed here introduces a severe layering
> violation.  The code is still calling what is called poll(), which
> should not have such a broken semantics.

While I have sympathy for your desire to apply pure POSIX functionality, the reality is that we do not have this luxury. Not if we want to support Git on the still most prevalent development platform: Windows. On Windows, you simply do not have that poll() that you are looking for.

In particular, there is no signal handling of the type you seem to want to require.

As to the layering violation you mention, first a HN quote, just to loosen the mood, and to at least partially ease the blow delivered by your mail:

	There is no such thing as a layering violation. You should be
	immediately suspicious of anyone who claims that there are such
	things.

;-)

Seriously again. If you care to have a look at the patch, you will see that the loop (which will now benefit from Kim's timeout on platforms without POSIX signal handling) *already* contains that call to reap_dead_children().

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

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 require it?

Thank you, Dscho