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

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

Hi Junio,

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

> 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