Hi gniibe-- Thanks for this code review, it's much appreciated!
On Mon 2017-01-16 23:21:15 -0500, NIIBE Yutaka wrote:
> For me, it is a bit difficult to apply the fourth patch only. So, I
> seek the update of the patch:
>
> 0003-agent-Avoid-tight-timer-tick-when-possible.patch
>
> How about changing the need_tick function, instead? My intention is to
> make the behavior of gpg-agent as similar as upstream version.
fwiw, i don't want the behavior to be exactly the same as upstream -- i
don't want gpg-agent to wake up every few seconds on platforms where it
shouldn't need to, for example :/
but the change i think you're proposing might be OK -- if it does the
frequent wakeups when it's trying to shut down...
> I mean, changing the first hunk of the patch of gnupg/agent/gpg-agent.c,
> like this (adding the check against shutdown_pending).
>
> --- gnupg.orig/agent/gpg-agent.c
> +++ gnupg/agent/gpg-agent.c
> @@ -2267,6 +2267,29 @@ create_directories (void)
> }
>
>
> +static int
> +need_tick (void)
> +{
[…]
> + /* if a shutdown was requested, we wait all connections closing. */
> + if (shutdown_pending)
> + return 1;
You're just talking about adding this one test, right?
It seems to me like this shouldn't be necessary, since i'd have thought
the child channel (chan_9 in your example) receiving eof would make the
main thread wake back up.
can you explain why that wouldn't be the case? is there some way to
cause the main thread to trigger a loop when the child channel closes?
Ideally, we don't want to wait a timer tick (up to 2 seconds) before
shutting down. if we know we're shutting down and the last client has
closed, we should just fall into the main loop itself, right? The
entire time between when the shutdown is requested and when we finally
shut down is a time that socket is locked and new clients can't
effectively connect, right?
i'm happy to apply your proposed change if there's no better way (it's
certainly better than the indefinite hang you've caught), but it still
feels sloppier than i'd want in general.
any thoughts?
--dkg
signature.asc
Description: PGP signature
