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: git@vger.kernel.org,
	Johannes Schindelin <johannes.schindelin@gmx.de>,
	Jeff King <peff@peff.net>
Subject: Re: [PATCH 1/2] daemon: use timeout for uninterruptible poll
Date: Mon, 16 Apr 2018 06:54:08 +0900	[thread overview]
Message-ID: <xmqq36zw16gv.fsf@gitster-ct.c.googlers.com> (raw)
In-Reply-To: <20180412210757.7792-2-kgybels@infogroep.be> (Kim Gybels's message of "Thu, 12 Apr 2018 23:07:56 +0200")

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.

The ideal solution would be to fix the emulation so that it also
properly works for reaping a dead child process, but if that is not
possible, another solution that does not break the API layering
would probably be to introduce our own version of something similar
to poll() that helps various platforms that cannot implement the
real poll() faithfully for whatever reason.  Such an xpoll() API
function we introduce (and implement in compat/poll.c) may take, in
addition to the usual parameters to reall poll(), the value of
live_children we have at this call site.  With that

 - On platforms whose poll() does work correctly for culling dead
   children will just ignore the live_children paramater in its
   implementation of xpoll()

 - On other platforms, it will shorten the timeout depending on the
   need to cull dead children, just like your patch did.

Thanks.


>
> Signed-off-by: Kim Gybels <kgybels@infogroep.be>
> ---
>  daemon.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/daemon.c b/daemon.c
> index fe833ea7de..6dc95c1b2f 100644
> --- a/daemon.c
> +++ b/daemon.c
> @@ -1147,6 +1147,7 @@ static int service_loop(struct socketlist *socklist)
>  {
>  	struct pollfd *pfd;
>  	int i;
> +	int poll_timeout = -1;
>  
>  	pfd = xcalloc(socklist->nr, sizeof(struct pollfd));
>  
> @@ -1161,8 +1162,13 @@ static int service_loop(struct socketlist *socklist)
>  		int i;
>  
>  		check_dead_children();
> -
> -		if (poll(pfd, socklist->nr, -1) < 0) {
> +#ifdef NO_POLL
> +		poll_timeout = live_children ? 100 : -1;
> +#endif
> +		int ret = poll(pfd, socklist->nr, poll_timeout);
> +		if  (ret == 0) {
> +			continue;
> +		} else if (ret < 0) {
>  			if (errno != EINTR) {
>  				logerror("Poll failed, resuming: %s",
>  				      strerror(errno));

  parent reply	other threads:[~2018-04-15 21:54 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 [this message]
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
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=xmqq36zw16gv.fsf@gitster-ct.c.googlers.com \
    --to=gitster@pobox.com \
    --cc=git@vger.kernel.org \
    --cc=johannes.schindelin@gmx.de \
    --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).