From: Junio C Hamano <gitster@pobox•com>
To: Lars Schneider <larsxschneider@gmail•com>
Cc: "Torsten Bögershausen" <tboegi@web•de>, git <git@vger•kernel.org>,
"Jeff King" <peff@peff•net>, "Stefan Beller" <sbeller@google•com>,
"Jakub Narębski" <jnareb@gmail•com>,
"Martin-Louis Bright" <mlbright@gmail•com>,
ramsay@ramsayjones•plus.com
Subject: Re: [PATCH v8 00/11] Git filter protocol
Date: Mon, 03 Oct 2016 10:02:14 -0700 [thread overview]
Message-ID: <xmqqvax974dl.fsf@gitster.mtv.corp.google.com> (raw)
In-Reply-To: <C53500E8-7352-4AAC-9F53-40CCFA7F1418@gmail.com> (Lars Schneider's message of "Sat, 1 Oct 2016 20:59:48 +0200")
Lars Schneider <larsxschneider@gmail•com> writes:
>> If the filter process refuses to die forever when Git told it to
>> shutdown (by closing the pipe to it, for example), that filter
>> process is simply buggy. I think we want users to become aware of
>> that, instead of Git leaving it behind, which essentially is to
>> sweep the problem under the rug.
>>
>> I agree with what Peff said elsewhere in the thread; if a filter
>> process wants to take time to clean things up while letting Git
>> proceed, it can do its own process management, but I think it is
>> sensible for Git to wait the filter process it directly spawned.
>
> To realize the approach above I prototyped the run-command patch below:
>
> I added an "exit_timeout" variable to the "child_process" struct.
> On exit, Git will close the pipe to the process and wait "exit_timeout"
> seconds until it kills the child process. If "exit_timeout" is negative
> then Git will wait until the process is done.
> If we use that in the long running filter process, then we could make
> the timeout even configurable. E.g. with "filter.<driver>.process-timeout".
>
> What do you think about this solution?
Is such a configuration (or timeout in general) necessary? I
suspect that a need for timeout, especially needing timeout and
needing to get killed that happens so often to require a
configuration variable, is a sign of something else seriously wrong.
What's the justification for a filter to _require_ getting killed
all the time when it is spawned? Otherwise you wouldn't configure
"this driver does not die when told, so we need a timeout" variable.
Is it a sign of the flaw in the protocol to talk to it? e.g. Git
has a way to tell it to die, but it somehow is very hard to hear
from filter's end and honor that request?
I think that we would need some timeout in the mechanism, but not to
be used for "killing".
You would decide to "kill" an filter process in two cases: the
filter is buggy and refuses to die when Git tells it to exit, or the
code in Git waiting for its death is somehow miscounting its
children, and thought it told to die one process but in fact it
didn't (perhaps it told somebody else to die), or it thought it
hasn't seen the child die when in fact it already did.
Calling kill(2) and exiting would hide these two kind of bugs from
end users. Not doing so would give the end users a hung Git, which
is a VERY GOOD thing. Otherwise you would not notice bugs and lose
the opportunity to diagnose and fix it.
The timeout would be good for you to give a message "filter process
running the script '%s' is not exiting; I am waiting for it". The
user is still left with a hung Git, and can then see if that process
is hanging around. If it is, then we found a buggy filter. Or we
found a buggy Git. Either needs to be fixed. I do not think it
would help anybody by doing a kill(2) to sweep possible bugs under
the rug.
Thanks.
next prev parent reply other threads:[~2016-10-03 17:02 UTC|newest]
Thread overview: 71+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-09-20 19:02 [PATCH v8 00/11] Git filter protocol larsxschneider
2016-09-20 19:02 ` [PATCH v8 01/11] pkt-line: rename packet_write() to packet_write_fmt() larsxschneider
2016-09-24 21:14 ` Jakub Narębski
2016-09-26 18:49 ` Lars Schneider
2016-09-28 23:15 ` Jakub Narębski
2016-09-20 19:02 ` [PATCH v8 02/11] pkt-line: extract set_packet_header() larsxschneider
2016-09-24 21:22 ` Jakub Narębski
2016-09-26 18:53 ` Lars Schneider
2016-09-20 19:02 ` [PATCH v8 03/11] run-command: move check_pipe() from write_or_die to run_command larsxschneider
2016-09-24 22:12 ` Jakub Narębski
2016-09-26 16:13 ` Lars Schneider
2016-09-26 16:21 ` Jakub Narębski
2016-09-20 19:02 ` [PATCH v8 04/11] pkt-line: add packet_write_fmt_gently() larsxschneider
2016-09-24 22:27 ` Jakub Narębski
2016-09-20 19:02 ` [PATCH v8 05/11] pkt-line: add packet_flush_gently() larsxschneider
2016-09-24 22:56 ` Jakub Narębski
2016-09-20 19:02 ` [PATCH v8 06/11] pkt-line: add packet_write_gently() larsxschneider
2016-09-25 11:26 ` Jakub Narębski
2016-09-26 19:21 ` Lars Schneider
2016-09-27 8:39 ` Jeff King
2016-09-27 19:33 ` Jakub Narębski
2016-09-20 19:02 ` [PATCH v8 07/11] pkt-line: add functions to read/write flush terminated packet streams larsxschneider
2016-09-25 13:46 ` Jakub Narębski
2016-09-26 20:23 ` Lars Schneider
2016-09-27 8:14 ` Lars Schneider
2016-09-27 9:00 ` Jeff King
2016-09-27 12:10 ` Lars Schneider
2016-09-27 12:13 ` Jeff King
2016-09-20 19:02 ` [PATCH v8 08/11] convert: quote filter names in error messages larsxschneider
2016-09-25 14:03 ` Jakub Narębski
2016-09-20 19:02 ` [PATCH v8 09/11] convert: modernize tests larsxschneider
2016-09-25 14:43 ` Jakub Narębski
2016-09-20 19:02 ` [PATCH v8 10/11] convert: make apply_filter() adhere to standard Git error handling larsxschneider
2016-09-25 14:47 ` Jakub Narębski
2016-09-20 19:02 ` [PATCH v8 11/11] convert: add filter.<driver>.process option larsxschneider
2016-09-26 22:41 ` Jakub Narębski
2016-09-30 18:56 ` Lars Schneider
2016-10-04 20:50 ` Jakub Narębski
2016-10-06 13:16 ` Lars Schneider
2016-09-27 15:37 ` Jakub Narębski
2016-09-30 19:38 ` Lars Schneider
2016-10-04 21:00 ` Jakub Narębski
2016-10-06 21:27 ` Lars Schneider
2016-09-28 23:14 ` Jakub Narębski
2016-10-01 15:34 ` Lars Schneider
2016-10-04 21:34 ` Jakub Narębski
2016-09-28 21:49 ` [PATCH v8 00/11] Git filter protocol Junio C Hamano
2016-09-29 10:28 ` Lars Schneider
2016-09-29 11:57 ` Torsten Bögershausen
2016-09-29 16:57 ` Junio C Hamano
2016-09-29 17:57 ` Lars Schneider
2016-09-29 18:18 ` Torsten Bögershausen
2016-09-29 18:38 ` Johannes Sixt
2016-09-29 21:27 ` Junio C Hamano
2016-10-01 18:59 ` Lars Schneider
2016-10-01 20:48 ` Jakub Narębski
2016-10-03 17:13 ` Lars Schneider
2016-10-04 19:04 ` Jakub Narębski
2016-10-06 13:13 ` Lars Schneider
2016-10-06 16:01 ` Jeff King
2016-10-06 17:17 ` Junio C Hamano
2016-10-03 17:02 ` Junio C Hamano [this message]
2016-10-03 17:35 ` Lars Schneider
2016-10-04 12:11 ` Jeff King
2016-10-04 16:47 ` Junio C Hamano
2016-09-29 18:02 ` Jeff King
2016-09-29 21:19 ` Junio C Hamano
2016-09-29 20:50 ` Lars Schneider
2016-09-29 21:12 ` Junio C Hamano
2016-09-29 20:59 ` Jakub Narębski
2016-09-29 21:17 ` Junio C Hamano
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
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=xmqqvax974dl.fsf@gitster.mtv.corp.google.com \
--to=gitster@pobox$(echo .)com \
--cc=git@vger$(echo .)kernel.org \
--cc=jnareb@gmail$(echo .)com \
--cc=larsxschneider@gmail$(echo .)com \
--cc=mlbright@gmail$(echo .)com \
--cc=peff@peff$(echo .)net \
--cc=ramsay@ramsayjones$(echo .)plus.com \
--cc=sbeller@google$(echo .)com \
--cc=tboegi@web$(echo .)de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox