Re: [PATCH v8 00/11] Git filter protocol

["Torsten Bögershausen" <tboegi@web.de>]

On 29/09/16 19:57, Lars Schneider wrote:
>> On 29 Sep 2016, at 18:57, Junio C Hamano <gitster@pobox.com> wrote:
>>
>> Torsten Bögershausen <tboegi@web.de> writes:
>>
>>>> 1) Git exits
>>>> 2) The filter process receives EOF and prints "STOP" to the log
>>>> 3) t0021 checks the content of the log
>>>>
>>>> Sometimes 3 happened before 2 which makes the test fail.
>>>> (Example: https://travis-ci.org/git/git/jobs/162660563 )
>>>>
>>>> I added a this to wait until the filter process terminates:
>>>>
>>>> +wait_for_filter_termination () {
>>>> +	while ps | grep -v grep | grep -F "/t0021/rot13-filter.pl" >/dev/null 2>&1
>>>> +	do
>>>> +		echo "Waiting for /t0021/rot13-filter.pl to finish..."
>>>> +		sleep 1
>>>> +	done
>>>> +}
>>>>
>>>> Does this look OK to you?
>>> Do we need the ps at all ?
>>> How about this:
>>>
>>> +wait_for_filter_termination () {
>>> +	while ! grep "STOP"  LOGFILENAME >/dev/null
>>> +	do
>>> +		echo "Waiting for /t0021/rot13-filter.pl to finish..."
>>> +		sleep 1
>>> +	done
>>> +}
>> Running "ps" and grepping for a command is not suitable for script
>> to reliably tell things, so it is out of question.  Compared to
>> that, your version looks slightly better, but what if the machinery
>> that being tested, i.e. the part that drives the filter process, is
>> buggy or becomes buggy and causes the filter process that writes
>> "STOP" to die before it actually writes that string?
>>
>> I have a feeling that the machinery being tested needs to be fixed
>> so that the sequence is always be:
>>
>>     0) Git spawns the filter process, as it needs some contents to
>>        be filtered.
>>
>>     1) Git did everything it needed to do and decides that is time
>>        to go.
>>
>>     2) Filter process receives EOF and prints "STOP" to the log.
>>
>>     3) Git waits until the filter process finishes.
>>
>>     4) t0021, after Git finishes, checks the log.
>>
>> Repeated sleep combined with grep is probably just sweeping the real
>> problem under the rug.  Do we have enough information to do the
>> above?
>>
>> An inspiration may be in the way we centrally clean all tempfiles
>> and lockfiles before exiting.  We have a central registry of these
>> files that need cleaning up and have a single atexit(3) handler to
>> clean them up.  Perhaps we need a registry that filter processes
>> spawned by the mechanism Lars introduces in this series, and have an
>> atexit(3) handler that closes the pipe to them (which signals the
>> filters that it is time for them to go) and wait(2) on them, or
>> something?  I do not think we want any kill(2) to be involved in
>> this clean-up procedure, but I do think we should wait(2) on what we
>> spawn, as long as these processes are meant to be shut down when the
>> main process of Git exits (this is different from things like
>> credential-cache daemon where they are expected to persist and meant
>> to serve multiple Git processes).
> We discussed that issue in v4 and v6:
> http://public-inbox.org/git/20160803225313.pk3tfe5ovz4y3i7l@sigill.intra.peff.net/
> http://public-inbox.org/git/xmqqbn0a3wy3.fsf@gitster.mtv.corp.google.com/
>
> My impression was that you don't want Git to wait for the filter process.
> If Git waits for the filter process - how long should Git wait?
>
> Thanks,
> Lars

Hm,
I would agree that  Git should not wait for the filter.
But does the test suite need to wait for the filter ?
May be, in this case we test the filter and Git, which is good.
Adding a 1 second delay, if, and only if, there is a racy condition,
is not that bad (or do we have better ways to check for a process to
be terminated ?)