Correctly kill 'git daemon' child process, fixing a lot of testing issues.

tests/fake_repos.py

Index: tests/fake_repos.py
diff --git a/tests/fake_repos.py b/tests/fake_repos.py
index 8674aca56acf1f5a6ea2cbe23bec013a1623ccd8..2fde1e9a51ac07fb61a3d9ff0594f69a86783c75 100755
--- a/tests/fake_repos.py
+++ b/tests/fake_repos.py
@@ -11,9 +11,11 @@ import logging
 import os
 import pprint
 import re
+import socket
 import stat
 import subprocess
 import sys
+import tempfile
 import time
 import unittest
 
@@ -24,23 +26,35 @@ import scm
 ## Utility functions
 
 
+def kill_pid(pid):
+  """Kills a process by its process id."""
+  try:
+    import signal
+    return os.kill(pid, signal.SIGKILL)
+  except ImportError:
+    pass
+
+
 def addKill():

[Evan Martin, 2011/03/04 18:13:13]

The capitalization style of this doesn't match the function above, nor do either
match the one below.  Please pick one style for the file.  :)

[M-A Ruel, 2011/03/04 18:53:30]

I want to convert to PEP8 but I do that incrementally to keep the diff readable.
The reason tearDownXX() style is used is because of unittest member functions.

I've changed many more function names.

-  """Add kill() method to subprocess.Popen for python <2.6"""
-  if getattr(subprocess.Popen, 'kill', None):
+  """Adds kill() method to subprocess.Popen for python <2.6"""
+  if hasattr(subprocess.Popen, 'kill'):
     return
+
   # Unable to import 'module'
   # pylint: disable=F0401
   if sys.platform == 'win32':
     def kill_win(process):
-      import win32process
-      # Access to a protected member _handle of a client class
-      # pylint: disable=W0212
-      return win32process.TerminateProcess(process._handle, -1)
+      try:
+        import win32process
+        # Access to a protected member _handle of a client class
+        # pylint: disable=W0212
+        return win32process.TerminateProcess(process._handle, -1)
+      except ImportError:
+        pass

[Evan Martin, 2011/03/04 18:13:13]

Maybe split this out into a function like kill_pid?

[M-A Ruel, 2011/03/04 18:53:30]

done

     subprocess.Popen.kill = kill_win
   else:
     def kill_nix(process):
-      import signal
-      return os.kill(process.pid, signal.SIGKILL)
+      return kill_pid(process.pid)
     subprocess.Popen.kill = kill_nix
 
 
@@ -250,11 +264,14 @@ class FakeReposBase(object):
     self.gitdaemon = None
     self.common_init = False
     self.repos_dir = None
+    self.gitpidfile = None

[Evan Martin, 2011/03/04 18:13:13]

git_pid_file

     self.git_root = None
     self.svn_checkout = None
     self.svn_repo = None
     self.git_dirty = False
     self.svn_dirty = False
+    self.svn_port = 3690
+    self.git_port = 9418
 
   def trial_dir(self):
     if not self.TRIAL_DIR:
@@ -281,39 +298,52 @@ class FakeReposBase(object):
 
   def cleanup_dirt(self):
     """For each dirty repository, regenerate it."""
-    if self.svnserve and self.svn_dirty:
+    if self.svn_dirty:
+      if not self.tearDownSVN():
+        logging.warning('Using both leaking checkout and svn dirty checkout')
+    if self.git_dirty:
+      if not self.tearDownGIT():
+        logging.warning('Using both leaking checkout and git dirty checkout')
+
+  def tearDown(self):
+    self.tearDownSVN()
+    self.tearDownGIT()
+    if not self.SHOULD_LEAK:
+      logging.debug('Removing %s' % self.trial_dir())
+      rmtree(self.trial_dir())
+
+  def tearDownSVN(self):

[Evan Martin, 2011/03/04 18:13:13]

Also weird capitalization here.

+    if self.svnserve:
       logging.debug('Killing svnserve pid %s' % self.svnserve.pid)
       self.svnserve.kill()
+      self.wait_for_port_to_close(self.svn_port)
       self.svnserve = None
       if not self.SHOULD_LEAK:
-        logging.debug('Removing dirty %s' % self.svn_repo)
+        logging.debug('Removing %s' % self.svn_repo)
         rmtree(self.svn_repo)
-        logging.debug('Removing dirty %s' % self.svn_checkout)
+        logging.debug('Removing %s' % self.svn_checkout)
         rmtree(self.svn_checkout)
       else:
-        logging.warning('Using both leaking checkout and dirty checkout')
-    if self.gitdaemon and self.git_dirty:
+        return False
+    return True
+
+  def tearDownGIT(self):

[Evan Martin, 2011/03/04 18:13:13]

"Git" or "git", but never "GIT"

+    if self.gitdaemon:
       logging.debug('Killing git-daemon pid %s' % self.gitdaemon.pid)
       self.gitdaemon.kill()
       self.gitdaemon = None
+      if self.gitpidfile:
+        pid = int(self.gitpidfile.read())
+        self.gitpidfile.close()
+        kill_pid(pid)
+        self.gitpidfile = None
+      self.wait_for_port_to_close(self.git_port)
       if not self.SHOULD_LEAK:
-        logging.debug('Removing dirty %s' % self.git_root)
+        logging.debug('Removing %s' % self.git_root)
         rmtree(self.git_root)
       else:
-        logging.warning('Using both leaking checkout and dirty checkout')
-
-  def tearDown(self):
-    if self.svnserve:
-      logging.debug('Killing svnserve pid %s' % self.svnserve.pid)
-      self.svnserve.kill()
-      self.svnserve = None
-    if self.gitdaemon:
-      logging.debug('Killing git-daemon pid %s' % self.gitdaemon.pid)
-      self.gitdaemon.kill()
-      self.gitdaemon = None
-    if not self.SHOULD_LEAK:
-      logging.debug('Removing %s' % self.trial_dir())
-      rmtree(self.trial_dir())
+        return False
+    return True
 
   @staticmethod
   def _genTree(root, tree_dict):
@@ -354,7 +384,9 @@ class FakeReposBase(object):
     cmd = ['svnserve', '-d', '--foreground', '-r', self.repos_dir]
     if self.HOST == '127.0.0.1':
       cmd.append('--listen-host=127.0.0.1')
+    self.check_port_is_free(self.svn_port)
     self.svnserve = Popen(cmd, cwd=self.svn_repo)
+    self.wait_for_port_to_open(self.svn_port)
     self.populateSvn()
     self.svn_dirty = False
     return True
@@ -366,16 +398,23 @@ class FakeReposBase(object):
       return True
     if sys.platform == 'win32':
       return False
+    assert self.gitpidfile == None
     for repo in ['repo_%d' % r for r in range(1, self.NB_GIT_REPOS + 1)]:
       check_call(['git', 'init', '-q', join(self.git_root, repo)])
       self.git_hashes[repo] = [None]
+    # Unlike svn, populate git before starting the server.
     self.populateGit()
     # Start the daemon.
-    cmd = ['git', 'daemon', '--export-all', '--base-path=' + self.repos_dir]
+    self.gitpidfile = tempfile.NamedTemporaryFile()
+    cmd = ['git', 'daemon',
+        '--export-all',
+        '--base-path=' + self.repos_dir,
+        '--pid-file=' + self.gitpidfile.name]
     if self.HOST == '127.0.0.1':
       cmd.append('--listen=127.0.0.1')
-    logging.debug(cmd)
+    self.check_port_is_free(self.git_port)
     self.gitdaemon = Popen(cmd, cwd=self.repos_dir)
+    self.wait_for_port_to_open(self.git_port)
     self.git_dirty = False
     return True
 
@@ -400,6 +439,44 @@ class FakeReposBase(object):
       new_tree = tree.copy()
     self.git_hashes[repo].append((commit_hash, new_tree))
 
+  def check_port_is_free(self, port):
+    sock = socket.socket()
+    try:
+      sock.connect((self.HOST, port))
+      # It worked, throw.
+      logging.fatal('%d shouldn\'t be open' % port)

[Evan Martin, 2011/03/04 18:13:13]

"open" is a confusing word here -- it means not-free, yeah?

[M-A Ruel, 2011/03/04 18:53:30]

Renamed.

+      assert False
+    except EnvironmentError:
+      pass
+    finally:
+      sock.close()
+
+  def wait_for_port_to_open(self, port):
+    sock = socket.socket()
+    try:
+      while True:
+        try:
+          sock.connect((self.HOST, port))
+          logging.debug('%d is now open' % port)
+          break
+        except EnvironmentError:
+          pass
+        logging.debug('%d not open yet' % port)
+    finally:
+      sock.close()

[Evan Martin, 2011/03/04 18:13:13]

Should this have a timeout int it?

[M-A Ruel, 2011/03/04 18:53:30]

Added a clamp at 30 seconds.

+
+  def wait_for_port_to_close(self, port):
+    while True:
+      try:
+        sock = socket.socket()
+        sock.connect((self.HOST, port))
+        logging.debug('%d was open, waiting to close' % port)

[Evan Martin, 2011/03/04 18:13:13]

Is the wait done by the sock.close()?

[M-A Ruel, 2011/03/04 18:53:30]

There's no wait done, it loops as fast as it can, which is not that fast. Even
for svnserve, which is rather slow to start, it does 20 loops max.

+      except EnvironmentError:
+        logging.debug('%d now closed' % port)
+        break
+      finally:
+        sock.close()
+
   def populateSvn(self):
     raise NotImplementedError()