From: Torsten Bögershausen Date: Thu, 29 Sep 2016 18:18:07 GMT Subject: Re: [PATCH v8 00/11] Git filter protocol Message-ID: <7f8ab626-ecdb-70a8-aa19-615c3c84148e@web.de> In-Reply-To: <1A8A9127-4DF9-44AD-9497-F8A630AB1193@gmail.com> On 29/09/16 19:57, Lars Schneider wrote: >> On 29 Sep 2016, at 18:57, Junio C Hamano wrote: >> >> Torsten Bögershausen 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 ?)