threads / discuss / 52711

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

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

## tl;dr

13 messages between Jan 29, 2020 and Jan 31, 2020.

replies: 12people: 4as markdown or json

Luke Diamand· Jan 29, 2020, 11:12 UTC · lore

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.

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
Luke Diamand· Jan 29, 2020, 11:12 UTC · re: Luke Diamand · lore

[PATCHv1 1/6] git-p4: make closeStreams() idempotent

Ensure that we can safely call self.closeStreams() multiple times, and can also call it even if there is no git fast-import stream at all.

Signed-off-by: Luke Diamand <luke@diamand.org>
---
 git-p4.py | 3 +++
 1 file changed, 3 insertions(+)
diff --git a/git-p4.py b/git-p4.py
index 40d9e7c594..23724defe8 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -3549,11 +3549,14 @@ def openStreams(self):
         self.gitError = self.importProcess.stderr
 
     def closeStreams(self):
+        if self.gitStream is None:
+            return
         self.gitStream.close()
         if self.importProcess.wait() != 0:
             die("fast-import failed: %s" % self.gitError.read())
         self.gitOutput.close()
         self.gitError.close()
+        self.gitStream = None
 
     def run(self, args):
         if self.importIntoRemotes:
-- 
2.20.1.390.gb5101f9297
Luke Diamand· Jan 29, 2020, 11:12 UTC · re: Luke Diamand · lore

[PATCHv1 2/6] git-p4: add P4CommandException to report errors talking to Perforce

Currently when there is a P4 error, git-p4 calls die() which just exits.

This then leaves the git-fast-import process still running, and can even leave p4 itself still running.

As a result, git-p4 fails to exit cleanly. This is a particular problem for people running the unit tests in regression.

Use this exception to report errors upwards, cleaning up as the error propagates.

Signed-off-by: Luke Diamand <luke@diamand.org>
---
 git-p4.py | 8 ++++++++
 1 file changed, 8 insertions(+)
diff --git a/git-p4.py b/git-p4.py
index 23724defe8..df2a956622 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -618,6 +618,14 @@ def __init__(self, exit_code, p4_result, limit):
         super(P4RequestSizeException, self).__init__(exit_code, p4_result)
         self.limit = limit
 
+class P4CommandException(P4Exception):
+    """ Something went wrong calling p4 which means we have to give up """
+    def __init__(self, msg):
+        self.msg = msg
+
+    def __str__(self):
+        return self.msg
+
 def isModeExecChanged(src_mode, dst_mode):
     return isModeExec(src_mode) != isModeExec(dst_mode)
 
-- 
2.20.1.390.gb5101f9297
Luke Diamand· Jan 29, 2020, 11:12 UTC · re: Luke Diamand · lore

[PATCHv1 3/6] git-p4: disable some pylint warnings, to get pylint output to something manageable

pylint is incredibly useful for finding bugs, but git-p4 has never used it, so there are a lot of warnings that while important, don't actually result in bugs.

Let's turn those off for now, so we can get some useful output.
Signed-off-by: Luke Diamand <luke@diamand.org>
---
 git-p4.py | 8 ++++++++
 1 file changed, 8 insertions(+)
diff --git a/git-p4.py b/git-p4.py
index df2a956622..d796074b87 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -7,6 +7,14 @@
 #            2007 Trolltech ASA
 # License: MIT <http://www.opensource.org/licenses/mit-license.php>
 #
+# pylint: disable=invalid-name,missing-docstring,too-many-arguments,broad-except
+# pylint: disable=no-self-use,wrong-import-position,consider-iterating-dictionary
+# pylint: disable=wrong-import-order,unused-import,too-few-public-methods
+# pylint: disable=too-many-lines,ungrouped-imports,fixme,too-many-locals
+# pylint: disable=line-too-long,bad-whitespace,superfluous-parens
+# pylint: disable=too-many-statements,too-many-instance-attributes
+# pylint: disable=too-many-branches,too-many-nested-blocks
+#
 import sys
 if sys.hexversion < 0x02040000:
     # The limiter is the subprocess module
-- 
2.20.1.390.gb5101f9297
Luke Diamand· Jan 29, 2020, 11:12 UTC · re: Luke Diamand · lore

[PATCHv1 4/6] git-p4: create helper function importRevisions()

This makes it easier to try/catch around this block of code to ensure cleanup following p4 failures is handled properly.

Signed-off-by: Luke Diamand <luke@diamand.org>
---
 git-p4.py | 132 ++++++++++++++++++++++++++++--------------------------
 1 file changed, 68 insertions(+), 64 deletions(-)
diff --git a/git-p4.py b/git-p4.py
index d796074b87..f90b43fe5e 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -3555,6 +3555,73 @@ def importHeadRevision(self, revision):
             print("IO error details: {}".format(err))
             print(self.gitError.read())
 
+
+    def importRevisions(self, args, branch_arg_given):
+        changes = []
+
+        if len(self.changesFile) > 0:
+            output = open(self.changesFile).readlines()
+            changeSet = set()
+            for line in output:
+                changeSet.add(int(line))
+
+            for change in changeSet:
+                changes.append(change)
+
+            changes.sort()
+        else:
+            # catch "git p4 sync" with no new branches, in a repo that
+            # does not have any existing p4 branches
+            if len(args) == 0:
+                if not self.p4BranchesInGit:
+                    die("No remote p4 branches.  Perhaps you never did \"git p4 clone\" in here.")
+
+                # The default branch is master, unless --branch is used to
+                # specify something else.  Make sure it exists, or complain
+                # nicely about how to use --branch.
+                if not self.detectBranches:
+                    if not branch_exists(self.branch):
+                        if branch_arg_given:
+                            die("Error: branch %s does not exist." % self.branch)
+                        else:
+                            die("Error: no branch %s; perhaps specify one with --branch." %
+                                self.branch)
+
+            if self.verbose:
+                print("Getting p4 changes for %s...%s" % (', '.join(self.depotPaths),
+                                                          self.changeRange))
+            changes = p4ChangesForPaths(self.depotPaths, self.changeRange, self.changes_block_size)
+
+            if len(self.maxChanges) > 0:
+                changes = changes[:min(int(self.maxChanges), len(changes))]
+
+        if len(changes) == 0:
+            if not self.silent:
+                print("No changes to import!")
+        else:
+            if not self.silent and not self.detectBranches:
+                print("Import destination: %s" % self.branch)
+
+            self.updatedBranches = set()
+
+            if not self.detectBranches:
+                if args:
+                    # start a new branch
+                    self.initialParent = ""
+                else:
+                    # build on a previous revision
+                    self.initialParent = parseRevision(self.branch)
+
+            self.importChanges(changes)
+
+            if not self.silent:
+                print("")
+                if len(self.updatedBranches) > 0:
+                    sys.stdout.write("Updated branches: ")
+                    for b in self.updatedBranches:
+                        sys.stdout.write("%s " % b)
+                    sys.stdout.write("\n")
+
     def openStreams(self):
         self.importProcess = subprocess.Popen(["git", "fast-import"],
                                               stdin=subprocess.PIPE,
@@ -3761,70 +3828,7 @@ def run(self, args):
         if revision:
             self.importHeadRevision(revision)
         else:
-            changes = []
-
-            if len(self.changesFile) > 0:
-                output = open(self.changesFile).readlines()
-                changeSet = set()
-                for line in output:
-                    changeSet.add(int(line))
-
-                for change in changeSet:
-                    changes.append(change)
-
-                changes.sort()
-            else:
-                # catch "git p4 sync" with no new branches, in a repo that
-                # does not have any existing p4 branches
-                if len(args) == 0:
-                    if not self.p4BranchesInGit:
-                        die("No remote p4 branches.  Perhaps you never did \"git p4 clone\" in here.")
-
-                    # The default branch is master, unless --branch is used to
-                    # specify something else.  Make sure it exists, or complain
-                    # nicely about how to use --branch.
-                    if not self.detectBranches:
-                        if not branch_exists(self.branch):
-                            if branch_arg_given:
-                                die("Error: branch %s does not exist." % self.branch)
-                            else:
-                                die("Error: no branch %s; perhaps specify one with --branch." %
-                                    self.branch)
-
-                if self.verbose:
-                    print("Getting p4 changes for %s...%s" % (', '.join(self.depotPaths),
-                                                              self.changeRange))
-                changes = p4ChangesForPaths(self.depotPaths, self.changeRange, self.changes_block_size)
-
-                if len(self.maxChanges) > 0:
-                    changes = changes[:min(int(self.maxChanges), len(changes))]
-
-            if len(changes) == 0:
-                if not self.silent:
-                    print("No changes to import!")
-            else:
-                if not self.silent and not self.detectBranches:
-                    print("Import destination: %s" % self.branch)
-
-                self.updatedBranches = set()
-
-                if not self.detectBranches:
-                    if args:
-                        # start a new branch
-                        self.initialParent = ""
-                    else:
-                        # build on a previous revision
-                        self.initialParent = parseRevision(self.branch)
-
-                self.importChanges(changes)
-
-                if not self.silent:
-                    print("")
-                    if len(self.updatedBranches) > 0:
-                        sys.stdout.write("Updated branches: ")
-                        for b in self.updatedBranches:
-                            sys.stdout.write("%s " % b)
-                        sys.stdout.write("\n")
+            self.importRevisions(args, branch_arg_given)
 
         if gitConfigBool("git-p4.importLabels"):
             self.importLabels = True
-- 
2.20.1.390.gb5101f9297
Luke Diamand· Jan 29, 2020, 11:12 UTC · re: Luke Diamand · lore

[PATCHv1 5/6] git-p4: cleanup better on error exit

After an error, git-p4 calls die(). This just exits, and leaves child processes still running.

Instead of calling die(), raise an exception and catch it where the child process(es) (git-fastimport) are created.

This was analyzed in detail here:
    https://public-inbox.org/git/20190227094926.GE19739@szeder.dev/

This change does not address the particular issue of p4CmdList() invoking a subchild and not waiting for it on error.

Signed-off-by: Luke Diamand <luke@diamand.org>
---
 git-p4.py | 43 ++++++++++++++++++++++++++++---------------
 1 file changed, 28 insertions(+), 15 deletions(-)
diff --git a/git-p4.py b/git-p4.py
index f90b43fe5e..a69a24bf4c 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -169,6 +169,9 @@ def calcDiskFree():
         return st.f_bavail * st.f_frsize
 
 def die(msg):
+    """ Terminate execution. Make sure that any running child processes have been wait()ed for before
+        calling this.
+    """
     if verbose:
         raise Exception(msg)
     else:
@@ -3574,7 +3577,7 @@ def importRevisions(self, args, branch_arg_given):
             # does not have any existing p4 branches
             if len(args) == 0:
                 if not self.p4BranchesInGit:
-                    die("No remote p4 branches.  Perhaps you never did \"git p4 clone\" in here.")
+                    raise P4CommandException("No remote p4 branches.  Perhaps you never did \"git p4 clone\" in here.")
 
                 # The default branch is master, unless --branch is used to
                 # specify something else.  Make sure it exists, or complain
@@ -3582,9 +3585,9 @@ def importRevisions(self, args, branch_arg_given):
                 if not self.detectBranches:
                     if not branch_exists(self.branch):
                         if branch_arg_given:
-                            die("Error: branch %s does not exist." % self.branch)
+                            raise P4CommandException("Error: branch %s does not exist." % self.branch)
                         else:
-                            die("Error: no branch %s; perhaps specify one with --branch." %
+                            raise P4CommandException("Error: no branch %s; perhaps specify one with --branch." %
                                 self.branch)
 
             if self.verbose:
@@ -3825,22 +3828,32 @@ def run(self, args):
 
         self.openStreams()
 
-        if revision:
-            self.importHeadRevision(revision)
-        else:
-            self.importRevisions(args, branch_arg_given)
+        err = None
 
-        if gitConfigBool("git-p4.importLabels"):
-            self.importLabels = True
+        try:
+            if revision:
+                self.importHeadRevision(revision)
+            else:
+                self.importRevisions(args, branch_arg_given)
 
-        if self.importLabels:
-            p4Labels = getP4Labels(self.depotPaths)
-            gitTags = getGitTags()
+            if gitConfigBool("git-p4.importLabels"):
+                self.importLabels = True
 
-            missingP4Labels = p4Labels - gitTags
-            self.importP4Labels(self.gitStream, missingP4Labels)
+            if self.importLabels:
+                p4Labels = getP4Labels(self.depotPaths)
+                gitTags = getGitTags()
 
-        self.closeStreams()
+                missingP4Labels = p4Labels - gitTags
+                self.importP4Labels(self.gitStream, missingP4Labels)
+
+        except P4CommandException as e:
+            err = e
+
+        finally:
+            self.closeStreams()
+
+        if err:
+            die(str(err))
 
         # Cleanup temporary branches created during import
         if self.tempBranches != []:
-- 
2.20.1.390.gb5101f9297
Luke Diamand· Jan 29, 2020, 11:12 UTC · re: Luke Diamand · lore

[PATCHv1 6/6] git-p4: check for access to remote host earlier

Check we can talk to the remote host before starting the git-fastimport subchild.

Otherwise we fail to connect, and then exit, leaving git-fastimport still running since we did not wait() for it.

Signed-off-by: Luke Diamand <luke@diamand.org>
---
 git-p4.py | 2 ++
 1 file changed, 2 insertions(+)
diff --git a/git-p4.py b/git-p4.py
index a69a24bf4c..eb5bc28cf9 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -3826,6 +3826,8 @@ def run(self, args):
                     b = b[len(self.projectName):]
                 self.createdBranches.add(b)
 
+        p4_check_access()
+
         self.openStreams()
 
         err = None
-- 
2.20.1.390.gb5101f9297
Eric Sunshine· Jan 29, 2020, 15:00 UTC · re: Luke Diamand · lore

Re: [PATCHv1 4/6] git-p4: create helper function importRevisions()

On Wed, Jan 29, 2020 at 6:13 AM Luke Diamand <luke@diamand.org> wrote:
Show 10 quoted lines
> This makes it easier to try/catch around this block of code to ensure
> cleanup following p4 failures is handled properly.
>
> Signed-off-by: Luke Diamand <luke@diamand.org>
> ---
> diff --git a/git-p4.py b/git-p4.py
> @@ -3555,6 +3555,73 @@ def importHeadRevision(self, revision):
> +    def importRevisions(self, args, branch_arg_given):
> +        if len(self.changesFile) > 0:
> +            output = open(self.changesFile).readlines()

Not a new problem (since this code is merely being relocated), but is this leaking the open file? Should there be an accompanying close() somewhere?

    f = open(self.changesFile)
    output = f.readlines()
    close(f)
or something.
Luke Diamand· Jan 29, 2020, 18:36 UTC · re: Eric Sunshine · lore

Re: [PATCHv1 4/6] git-p4: create helper function importRevisions()

On Wed, 29 Jan 2020 at 15:00, Eric Sunshine <sunshine@sunshineco.com> wrote:
Show 20 quoted lines
>
> On Wed, Jan 29, 2020 at 6:13 AM Luke Diamand <luke@diamand.org> wrote:
> > This makes it easier to try/catch around this block of code to ensure
> > cleanup following p4 failures is handled properly.
> >
> > Signed-off-by: Luke Diamand <luke@diamand.org>
> > ---
> > diff --git a/git-p4.py b/git-p4.py
> > @@ -3555,6 +3555,73 @@ def importHeadRevision(self, revision):
> > +    def importRevisions(self, args, branch_arg_given):
> > +        if len(self.changesFile) > 0:
> > +            output = open(self.changesFile).readlines()
>
> Not a new problem (since this code is merely being relocated), but is
> this leaking the open file? Should there be an accompanying close()
> somewhere?
>
>     f = open(self.changesFile)
>     output = f.readlines()
>     close(f)
with open(self.changesFile) as f:
    output = f.readlines()
I can put something like that in either as a followup, or the next re-roll.
>
> or something.
Junio C Hamano· Jan 30, 2020, 19:59 UTC · re: Eric Sunshine · lore

Re: [PATCHv1 4/6] git-p4: create helper function importRevisions()

Eric Sunshine <sunshine@sunshineco.com> writes:
Show 21 quoted lines
> On Wed, Jan 29, 2020 at 6:13 AM Luke Diamand <luke@diamand.org> wrote:
>> This makes it easier to try/catch around this block of code to ensure
>> cleanup following p4 failures is handled properly.
>>
>> Signed-off-by: Luke Diamand <luke@diamand.org>
>> ---
>> diff --git a/git-p4.py b/git-p4.py
>> @@ -3555,6 +3555,73 @@ def importHeadRevision(self, revision):
>> +    def importRevisions(self, args, branch_arg_given):
>> +        if len(self.changesFile) > 0:
>> +            output = open(self.changesFile).readlines()
>
> Not a new problem (since this code is merely being relocated), but is
> this leaking the open file? Should there be an accompanying close()
> somewhere?
>
>     f = open(self.changesFile)
>     output = f.readlines()
>     close(f)
>
> or something.

Hmph, I was naively hoping that the (never assigned to any variable) last reference going away at the end of the statement would make the file object dead, and we can let eventual GC to close it.

Nevertheless it would not hurt to explicitly control the lifetime.
Luke Diamand· Jan 31, 2020, 10:36 UTC · re: Junio C Hamano · lore

Re: [PATCHv1 4/6] git-p4: create helper function importRevisions()

On Thu, 30 Jan 2020 at 19:59, Junio C Hamano <gitster@pobox.com> wrote:
Show 30 quoted lines
>
> Eric Sunshine <sunshine@sunshineco.com> writes:
>
> > On Wed, Jan 29, 2020 at 6:13 AM Luke Diamand <luke@diamand.org> wrote:
> >> This makes it easier to try/catch around this block of code to ensure
> >> cleanup following p4 failures is handled properly.
> >>
> >> Signed-off-by: Luke Diamand <luke@diamand.org>
> >> ---
> >> diff --git a/git-p4.py b/git-p4.py
> >> @@ -3555,6 +3555,73 @@ def importHeadRevision(self, revision):
> >> +    def importRevisions(self, args, branch_arg_given):
> >> +        if len(self.changesFile) > 0:
> >> +            output = open(self.changesFile).readlines()
> >
> > Not a new problem (since this code is merely being relocated), but is
> > this leaking the open file? Should there be an accompanying close()
> > somewhere?
> >
> >     f = open(self.changesFile)
> >     output = f.readlines()
> >     close(f)
> >
> > or something.
>
> Hmph, I was naively hoping that the (never assigned to any variable)
> last reference going away at the end of the statement would make the
> file object dead, and we can let eventual GC to close it.
>
> Nevertheless it would not hurt to explicitly control the lifetime.

You are right, there is no file descriptor leak. It's easy enough to write some test code to demonstrate this.

Johannes Schindelin· Jan 30, 2020, 10:52 UTC · re: Luke Diamand · lore

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

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
>
>
Luke Diamand· Jan 30, 2020, 11:33 UTC · re: Johannes Schindelin · lore

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

On Thu, 30 Jan 2020 at 10:52, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:

Show 82 quoted lines
>
> Hi Luke,
>
> On Wed, 29 Jan 2020, Luke Diamand wrote:
>
> > 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 --

This should be fixed by my change. When that error occurs, the current version calls die(), with my change it raises an exception, and then cleans up further up the stack.

-                            die("Error: no branch %s; perhaps specify
one with --branch." %
+                            raise P4CommandException("Error: no
branch %s; perhaps specify one with --branch." %
Show 12 quoted lines
>
> 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 was actually looking at Gábor's report. He has a nifty hack which makes the errors crop up very rapidly, so I just fixed all of the failures that arose from that.

It's easy to see from inspecting the code that there are plenty of other places where this is not handled properly. Probably those should wait until the python3 fixes have been merged and/or there is some demand for them.

Show 7 quoted lines
>
> 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! Luke

Show 31 quoted lines
>
> Thanks,
> Dscho
>
> >
> > 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
> >
> >

← back to recent threads