Re: [PATCH] git daemon: avoid calling syslog() from a signal handler
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Jul 6, 2008, 06:08 UTC
- Message-ID
- <7vmykvo87w.fsf@gitster.siamese.dyndns.org>
- In-Reply-To
- <alpine.DEB.1.00.0807060337480.3557@eeepc-johanness>
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
Show 12 quoted lines
>> The question was not about the millisecond typo, but about why time-out >> at all. > > Because I do not want to change the semantics! > > ATM, in those cases where it works (as opposed to hanging!), git-daemon > --verbose reports in the syslog when a client disconnected, possibly with > an error. It does so with a timestamp so that you can see how long the > connection lasted. That is what logs are useful for. > > Now, syslog has timestamps at second-resolution (at least here it does), > and I wanted to imitate that.
Yes, we do not want to change the semantics, but that is not a reason for an unused daemon to wake up every second, isn't it?
Show 8 quoted lines
> The alternative would be to deprive all users of an (mostly) accurate > timestamp of the disconnect time. > >> Another way would be to set up a pipe to ourself that is included in the >> poll() and write a byte to the pipe from the signal handler. > > It still would need to break out of the poll(), in which case the effect > would be _exactly_ the same,...
I do not think so.
In the solution I suggested, you would set up a pipe to yourself, and give the read end of the pipe and the accepting socket to poll() with infinite timeout. And when you do reap in the signal handler, you write a byte to the write end of the pipe (and that would be the only codepath that would write to that pipe). That would make the pipe you are polling redable, and allow you to break out of poll(). When poll() returns thusly, you notice you are in that condition and read out that byte to keep the pipe clean, and do the check_dead_children() thing.
That way, you would wake up only when you actually have something useful to do. Otherwise you will stay dormant, waiting for something to happen, either by socket becoming ready to accept, or child dying and raising SIGCHLD.
I agree that it is a bit too elaborate change that we do not want to have in 'maint'.
Even then, for 'maint', we probably would at least want something like this on top of your patch, so that when we know we have absolutely nothing to do, we do not have to spin.
daemon.c | 4 +++- 1 files changed, 3 insertions(+), 1 deletions(-)
diff --git a/daemon.c b/daemon.c index 6dc96aa..ce3a6f5 100644 --- a/daemon.c +++ b/daemon.c @@ -944,6 +944,7 @@ static int service_loop(int socknum, int *socklist) for (;;) { int i; + int timeout; /* * This 1-sec timeout could lead to idly looping but it is @@ -952,7 +953,8 @@ static int service_loop(int socknum, int *socklist) * to ourselves that we poll, and write to the fd from child_handler() * to wake us up (and consume it when the poll() returns... */ - i = poll(pfd, socknum, 1000); + timeout = (children_spawned != children_deleted) ? 1000 : -1; + i = poll(pfd, socknum, timeout); if (i < 0) { if (errno != EINTR) { error("poll failed, resuming: %s",