git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: Kim Gybels <kgybels@infogroep.be>
Cc: Johannes Schindelin <Johannes.Schindelin@gmx.de>,
	git@vger.kernel.org, Jeff King <peff@peff.net>
Subject: Re: [PATCH 1/2] daemon: use timeout for uninterruptible poll
Date: Fri, 20 Apr 2018 08:18:00 +0900	[thread overview]
Message-ID: <xmqq1sfaeqfr.fsf@gitster-ct.c.googlers.com> (raw)
In-Reply-To: <20180419213322.GA19500@infogroep.be> (Kim Gybels's message of "Thu, 19 Apr 2018 23:33:22 +0200")

Kim Gybels <kgybels@infogroep.be> 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.

FWIW, I didn't mean to scold, either.

Rather I was pointing out that the code already maintains the number
of remaining children, which means that a more portable abstraction
than poll(), if we desired to have one, would merely be one step
away, as we already know that at least need that information to help
Windows.

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

Good analysis.  That consideration may mean that xpoll() as I
suggested is useless as a possible more portable abstraction (or,
"an abstraction that is implementable easily in both POSIX and
non-POSIX world"), but I suspect we would still want to have an
internal "portable" API that serves the purpose similar to how we
wanted POSIX poll() to serve.  The place the patch is touching is
not the only place poll() is used in the codebase, and other places
(and future ones we would add) may benefit from having one.

Thanks for being constructive.



  reply	other threads:[~2018-04-19 23:18 UTC|newest]

Thread overview: 15+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-04-12 21:07 [PATCH 0/2] Fix early EOF with GfW daemon Kim Gybels
2018-04-12 21:07 ` [PATCH 1/2] daemon: use timeout for uninterruptible poll Kim Gybels
2018-04-13 12:36   ` Johannes Schindelin
2018-04-15 17:08     ` Kim Gybels
2018-04-18 21:16       ` Johannes Schindelin
2018-04-15 21:54   ` Junio C Hamano
2018-04-15 22:16     ` Junio C Hamano
2018-04-18 21:07     ` Johannes Schindelin
2018-04-18 21:51       ` Junio C Hamano
2018-04-19 21:33         ` Kim Gybels
2018-04-19 23:18           ` Junio C Hamano [this message]
2018-04-12 21:07 ` [PATCH 2/2] daemon: graceful shutdown of client connection Kim Gybels
2018-04-13 13:03   ` Johannes Schindelin
2018-04-15 20:21     ` Kim Gybels
2018-04-18 21:48       ` Johannes Schindelin

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=xmqq1sfaeqfr.fsf@gitster-ct.c.googlers.com \
    --to=gitster@pobox.com \
    --cc=Johannes.Schindelin@gmx.de \
    --cc=git@vger.kernel.org \
    --cc=kgybels@infogroep.be \
    --cc=peff@peff.net \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).