On (19/04/18 06:51), Junio C Hamano wrote: > Johannes Schindelin <Johannes.Schindelin@xxxxxx> 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. > > 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
