Re: [PATCHv1 0/6] git-p4: wait() for child processes better

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Jan 30, 2020, 10:52 UTC
Message-ID: <nycvar.QRO.7.76.6.2001301131280.46@tvgsbejvaqbjf.bet>
In-Reply-To: <20200129111246.12196-1-luke@diamand.org>

Hi Luke,

On Wed, 29 Jan 2020, Luke Diamand wrote:

Show 6 quoted lines

> git-p4 handles most errors by calling die(). This can leave child
> processes still running, orphaned.
>
>     https://public-inbox.org/git/20190227094926.GE19739@szeder.dev/
>
> This is not a problem for humans, but for CI, it is.

Just to make sure that we're talking about the same thing. Looking at

https://dev.azure.com/git/git/_test/analytics?definitionId=11&contextType=build

I am not even sure that `git-p4` is our biggest problem there. In that list, the only flaky `git-p4` test I see is t9806.5. And it failed only once recently: https://dev.azure.com/git/git/_build/results?buildId=1640&view=ms.vss-test-web.build-test-results-tab

The log looks like this:

-- snip --
[...]
expecting success of 9806.5 'sync when no master branch prints a nice error':
	test_when_finished cleanup_git &&
	git p4 clone --branch=refs/remotes/p4/sb --dest="$git" //depot@2 &&
	(
		cd "$git" &&
		test_must_fail git p4 sync 2>err &&
		grep "Error: no branch refs/remotes/p4/master" err
	)

+ test_when_finished cleanup_git
+ test 0 = 0
+ test_cleanup={ cleanup_git
		} && (exit "$eval_ret"); eval_ret=$?; :
+ git p4 clone --branch=refs/remotes/p4/sb --dest=/home/vsts/work/1/s/t/trash directory.t9806-git-p4-options/git //depot@2
Initialized empty Git repository in /home/vsts/work/1/s/t/trash
directory.t9806-git-p4-options/git/.git/
Perforce db files in '.' will be created if missing...
Perforce db files in '.' will be created if missing...
Perforce db files in '.' will be created if missing...
Perforce db files in '.' will be created if missing...
Perforce db files in '.' will be created if missing...
Importing from //depot@2 into /home/vsts/work/1/s/t/trash directory.t9806-git-p4-options/git
Doing initial import of //depot/ from revision @2 into refs/remotes/p4/sb
+ cd /home/vsts/work/1/s/t/trash directory.t9806-git-p4-options/git
+ test_must_fail git p4 sync
+ _test_ok=
+ git p4 sync
Performing incremental import into refs/remotes/p4/master git branch
Depot paths: //depot/
+ exit_code=1
+ test 1 -eq 0
+ test_match_signal 13 1
+ test 1 = 141
+ test 1 = 269
+ return 1
+ test 1 -gt 129
+ test 1 -eq 127
+ test 1 -eq 126
+ return 0
+ grep Error: no branch refs/remotes/p4/master err
Error: no branch refs/remotes/p4/master; perhaps specify one with --branch.
+ cleanup_git
+ retry_until_success rm -r /home/vsts/work/1/s/t/trash directory.t9806-git-p4-options/git
+ nr_tries_left=60
+ rm -r /home/vsts/work/1/s/t/trash directory.t9806-git-p4-options/git
+ test_path_is_missing /home/vsts/work/1/s/t/trash directory.t9806-git-p4-options/git
+ test -e /home/vsts/work/1/s/t/trash directory.t9806-git-p4-options/git
+ echo Path exists:
Path exists:
+ ls -ld /home/vsts/work/1/s/t/trash directory.t9806-git-p4-options/git
drwxr-xr-x 3 vsts docker 4096 Jan 24 22:20 /home/vsts/work/1/s/t/trash directory.t9806-git-p4-options/git
+ test 1 -ge 1
+ echo /home/vsts/work/1/s/t/trash directory.t9806-git-p4-options/git /home/vsts/work/1/s/t/trash directory.t9806-git-p4-options/git
+ false
+ eval_ret=1
+ :
-- snap --

FWIW I see the same test being flaky in Git for Windows (but it _also_ only happened once in the past 14 days): https://dev.azure.com/git-for-windows/git/_test/analytics?definitionId=17&contextType=build and https://dev.azure.com/git-for-windows/git/_build/results?buildId=49174&view=ms.vss-test-web.build-test-results-tab

The log suggests to me that there is a path that has not been cleaned up, and _maybe_ it is timing-related, but it is also possible that a child process is still running that should have cleaned it up.

Are your patches about this failure?

I see that PATCH 5/6 talks about Gábor's analysis of t9800.6 in https://public-inbox.org/git/20190227094926.GE19739@szeder.dev/ which _looks_ similar.

FWIW I looked over your patches and they seem relatively obvious, even for somebody like me who barely gets to code in Python anymore.

Thanks, Dscho

Show 27 quoted lines

>
> This change improves things by raising an exception and cleaning up
> further up the stack, rather than simply calling die().
>
> This is only done in a few places, such that the tests pass with the changes
> suggested in the link (adding sleep strategically) but there are still
> plenty of places where git-p4 calls die().
>
> This also adds some pylint disables, so that we can start to run pylint
> on git-p4.
>
> Luke Diamand (6):
>   git-p4: make closeStreams() idempotent
>   git-p4: add P4CommandException to report errors talking to Perforce
>   git-p4: disable some pylint warnings, to get pylint output to
>     something manageable
>   git-p4: create helper function importRevisions()
>   git-p4: cleanup better on error exit
>   git-p4: check for access to remote host earlier
>
>  git-p4.py | 180 +++++++++++++++++++++++++++++++++---------------------
>  1 file changed, 109 insertions(+), 71 deletions(-)
>
> --
> 2.20.1.390.gb5101f9297
>
>