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

tests/fake_repos.py

 #!/usr/bin/python
 # Copyright (c) 2010 The Chromium Authors. All rights reserved.
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
 
 """Generate fake repositories for testing."""
 
 import atexit
+import datetime
 import errno
 import logging
 import os
 import pprint
 import re
+import socket
 import stat
 import subprocess
 import sys
+import tempfile
 import time
 import unittest
 
 sys.path.append(os.path.dirname(os.path.dirname(os.path.abspath(__file__))))
 
 import scm
 
 ## Utility functions
 
 
-def addKill():
-  """Add kill() method to subprocess.Popen for python <2.6"""
-  if getattr(subprocess.Popen, 'kill', None):
+def kill_pid(pid):
+  """Kills a process by its process id."""
+  try:
+    # Unable to import 'module'
+    # pylint: disable=F0401
+    import signal
+    return os.kill(pid, signal.SIGKILL)
+  except ImportError:
+    pass
+
+
+def kill_win(process):
+  """Kills a process with its windows handle.
+
+  Has no effect on other platforms.
+  """
+  try:
+    # Unable to import 'module'
+    # pylint: disable=F0401
+    import win32process
+    # Access to a protected member _handle of a client class
+    # pylint: disable=W0212
+    return win32process.TerminateProcess(process._handle, -1)
+  except ImportError:
+    pass
+
+
+def add_kill():
+  """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)
     subprocess.Popen.kill = kill_win
   else:
     def kill_nix(process):
-      import signal
-      return os.kill(process.pid, signal.SIGKILL)
+      return kill_pid(process.pid)

[Dirk Pranke, 2011/03/04 21:15:17]

I'd likely just change kill_pid to take process as an argument for consistency
w/ kill_win()

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

I can't because I need to kill 'git-daemon' child process by pid.

     subprocess.Popen.kill = kill_nix
 
 
 def rmtree(*path):
   """Recursively removes a directory, even if it's marked read-only.
 
   Remove the directory located at *path, if it exists.
 
   shutil.rmtree() doesn't work on Windows if any of the files or directories
   are read-only, which svn repositories and some .svn files are.  We need to
@@ skipping 189 matching lines @@
     self.svn_revs = [None]
     # Format is { repo: [ None, (hash, tree), (hash, tree), ... ], ... }
     # so reference looks like self.git_hashes[repo][rev][0] for hash and
     # self.git_hashes[repo][rev][1] for it's tree snapshot.
     # For consistency with self.svn_revs, it is 1-based too.
     self.git_hashes = {}
     self.svnserve = None
     self.gitdaemon = None
     self.common_init = False
     self.repos_dir = None
+    self.git_pid_file = None
     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:
       self.TRIAL_DIR = os.path.join(
           os.path.dirname(os.path.abspath(__file__)), '_trial')
     return self.TRIAL_DIR
 
-  def setUp(self):
+  def set_up(self):
     """All late initialization comes here.
 
     Note that it deletes all trial_dir() and not only repos_dir.
     """
     self.cleanup_dirt()
     if not self.common_init:
       self.common_init = True
       self.repos_dir = os.path.join(self.trial_dir(), 'repos')
       self.git_root = join(self.repos_dir, 'git')
       self.svn_checkout = join(self.repos_dir, 'svn_checkout')
       self.svn_repo = join(self.repos_dir, 'svn')
-      addKill()
+      add_kill()
       rmtree(self.trial_dir())
       os.makedirs(self.repos_dir)
-      atexit.register(self.tearDown)
+      atexit.register(self.tear_down)
 
   def cleanup_dirt(self):
     """For each dirty repository, regenerate it."""
-    if self.svnserve and self.svn_dirty:
-      logging.debug('Killing svnserve pid %s' % self.svnserve.pid)
-      self.svnserve.kill()
-      self.svnserve = None
-      if not self.SHOULD_LEAK:
-        logging.debug('Removing dirty %s' % self.svn_repo)
-        rmtree(self.svn_repo)
-        logging.debug('Removing dirty %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:
-      logging.debug('Killing git-daemon pid %s' % self.gitdaemon.pid)
-      self.gitdaemon.kill()
-      self.gitdaemon = None
-      if not self.SHOULD_LEAK:
-        logging.debug('Removing dirty %s' % self.git_root)
-        rmtree(self.git_root)
-      else:
-        logging.warning('Using both leaking checkout and dirty checkout')
+    if self.svn_dirty:
+      if not self.tear_down_svn():
+        logging.warning('Using both leaking checkout and svn dirty checkout')
+    if self.git_dirty:
+      if not self.tear_down_git():
+        logging.warning('Using both leaking checkout and git dirty checkout')
 
-  def tearDown(self):
+  def tear_down(self):
+    self.tear_down_svn()
+    self.tear_down_git()
+    if not self.SHOULD_LEAK:
+      logging.debug('Removing %s' % self.trial_dir())
+      rmtree(self.trial_dir())
+
+  def tear_down_svn(self):
     if self.svnserve:
       logging.debug('Killing svnserve pid %s' % self.svnserve.pid)
       self.svnserve.kill()
+      self.wait_for_port_to_free(self.svn_port)
       self.svnserve = None
+      if not self.SHOULD_LEAK:
+        logging.debug('Removing %s' % self.svn_repo)
+        rmtree(self.svn_repo)
+        logging.debug('Removing %s' % self.svn_checkout)
+        rmtree(self.svn_checkout)
+      else:
+        return False
+    return True
+
+  def tear_down_git(self):
     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())
+      if self.git_pid_file:
+        pid = int(self.git_pid_file.read())
+        self.git_pid_file.close()
+        kill_pid(pid)
+        self.git_pid_file = None
+      self.wait_for_port_to_free(self.git_port)
+      if not self.SHOULD_LEAK:
+        logging.debug('Removing %s' % self.git_root)
+        rmtree(self.git_root)
+      else:
+        return False
+    return True
 
   @staticmethod
   def _genTree(root, tree_dict):
     """For a dictionary of file contents, generate a filesystem."""
     if not os.path.isdir(root):
       os.makedirs(root)
     for (k, v) in tree_dict.iteritems():
       k_os = k.replace('/', os.sep)
       k_arr = k_os.split(os.sep)
       if len(k_arr) > 1:
         p = os.sep.join([root] + k_arr[:-1])
         if not os.path.isdir(p):
           os.makedirs(p)
       if v is None:
         os.remove(join(root, k))
       else:
         write(join(root, k), v)
 
-  def setUpSVN(self):
+  def set_up_svn(self):
     """Creates subversion repositories and start the servers."""
-    self.setUp()
+    self.set_up()
     if self.svnserve:
       return True
     try:
       check_call(['svnadmin', 'create', self.svn_repo])
     except OSError:
       return False
     write(join(self.svn_repo, 'conf', 'svnserve.conf'),
         '[general]\n'
         'anon-access = read\n'
         'auth-access = write\n'
         'password-db = passwd\n')
     text = '[users]\n'
     text += ''.join('%s = %s\n' % (usr, pwd) for usr, pwd in self.USERS)
     write(join(self.svn_repo, 'conf', 'passwd'), text)
 
     # Start the daemon.
     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_bind(self.svn_port, self.svnserve)
     self.populateSvn()
     self.svn_dirty = False
     return True
 
-  def setUpGIT(self):
+  def set_up_git(self):
     """Creates git repositories and start the servers."""
-    self.setUp()
+    self.set_up()
     if self.gitdaemon:
       return True
     if sys.platform == 'win32':
       return False
+    assert self.git_pid_file == 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.git_pid_file = tempfile.NamedTemporaryFile()
+    cmd = ['git', 'daemon',
+        '--export-all',
+        '--reuseaddr',
+        '--base-path=' + self.repos_dir,
+        '--pid-file=' + self.git_pid_file.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_bind(self.git_port, self.gitdaemon)
     self.git_dirty = False
     return True
 
   def _commit_svn(self, tree):
     self._genTree(self.svn_checkout, tree)
     commit_svn(self.svn_checkout, self.USERS[0][0], self.USERS[0][1])
     if self.svn_revs and self.svn_revs[-1]:
       new_tree = self.svn_revs[-1].copy()
       new_tree.update(tree)
     else:
       new_tree = tree.copy()
     self.svn_revs.append(new_tree)
 
   def _commit_git(self, repo, tree):
     repo_root = join(self.git_root, repo)
     self._genTree(repo_root, tree)
     commit_hash = commit_git(repo_root)
     if self.git_hashes[repo][-1]:
       new_tree = self.git_hashes[repo][-1][1].copy()
       new_tree.update(tree)
     else:
       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.
+      assert False, '%d shouldn\'t be bound' % port
+    except EnvironmentError:
+      pass
+    finally:
+      sock.close()
+
+  def wait_for_port_to_bind(self, port, process):
+    sock = socket.socket()
+    try:
+      start = datetime.datetime.utcnow()

[Dirk Pranke, 2011/03/04 21:15:17]

Curious why you're using datetime instead of just time?

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

Habit, no real reason.

+      maxdelay = datetime.timedelta(seconds=30)
+      while (datetime.datetime.utcnow() - start) < maxdelay:
+        try:
+          sock.connect((self.HOST, port))
+          logging.debug('%d is now bound' % port)
+          return
+        except EnvironmentError:
+          pass
+        logging.debug('%d is still not bound' % port)
+    finally:
+      sock.close()
+    # The process failed to bind. Kill it and dump its ouput.
+    process.kill()
+    logging.error('%s' % process.communicate()[0])
+    assert False, '%d is still not bound' % port
+
+  def wait_for_port_to_free(self, port):
+    start = datetime.datetime.utcnow()
+    maxdelay = datetime.timedelta(seconds=30)
+    while (datetime.datetime.utcnow() - start) < maxdelay:
+      try:
+        sock = socket.socket()
+        sock.connect((self.HOST, port))
+        logging.debug('%d was bound, waiting to free' % port)
+      except EnvironmentError:
+        logging.debug('%d now free' % port)
+        return
+      finally:
+        sock.close()
+    assert False, '%d is still bound' % port
+
   def populateSvn(self):
     raise NotImplementedError()
 
   def populateGit(self):
     raise NotImplementedError()
 
 
 class FakeRepos(FakeReposBase):
   """Implements populateSvn() and populateGit()."""
   NB_GIT_REPOS = 4
@@ skipping 215 matching lines @@
   # Override if necessary.
   FAKE_REPOS_CLASS = FakeRepos
 
   def __init__(self, *args, **kwargs):
     unittest.TestCase.__init__(self, *args, **kwargs)
     if not FakeReposTestBase.FAKE_REPOS:
       FakeReposTestBase.FAKE_REPOS = self.FAKE_REPOS_CLASS()
 
   def setUp(self):
     unittest.TestCase.setUp(self)
-    self.FAKE_REPOS.setUp()
+    self.FAKE_REPOS.set_up()
 
     # Remove left overs and start fresh.
     if not self.CLASS_ROOT_DIR:
       self.CLASS_ROOT_DIR = join(self.FAKE_REPOS.trial_dir(), 'smoke')
     self.root_dir = join(self.CLASS_ROOT_DIR, self.id())
     rmtree(self.root_dir)
     os.makedirs(self.root_dir)
     self.svn_base = 'svn://%s/svn/' % self.FAKE_REPOS.HOST
     self.git_base = 'git://%s/git/' % self.FAKE_REPOS.HOST
 
@@ skipping 64 matching lines @@
 
   def gittree(self, repo, rev):
     """Sort-hand: returns the directory tree for a git 'revision'."""
     return self.FAKE_REPOS.git_hashes[repo][int(rev)][1]
 
 
 def main(argv):
   fake = FakeRepos()
   print 'Using %s' % fake.trial_dir()
   try:
-    fake.setUp()
+    fake.set_up_svn()
+    fake.set_up_git()
     print('Fake setup, press enter to quit or Ctrl-C to keep the checkouts.')
     sys.stdin.readline()
   except KeyboardInterrupt:
     fake.SHOULD_LEAK = True
   return 0
 
 
 if '-l' in sys.argv:
   # See SHOULD_LEAK definition in FakeReposBase for its purpose.
   FakeReposBase.SHOULD_LEAK = True
   print 'Leaking!'
   sys.argv.remove('-l')
 
 
 if __name__ == '__main__':
   sys.exit(main(sys.argv))