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 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 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
 
 
 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.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:
       self.TRIAL_DIR = os.path.join(
           os.path.dirname(os.path.abspath(__file__)), '_trial')
     return self.TRIAL_DIR
 
   def setUp(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()
       rmtree(self.trial_dir())
       os.makedirs(self.repos_dir)
       atexit.register(self.tearDown)
 
   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.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 %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 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 not self.SHOULD_LEAK:
-      logging.debug('Removing %s' % self.trial_dir())
-      rmtree(self.trial_dir())
+      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 %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:
@@ skipping 20 matching lines @@
         '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_open(self.svn_port)
     self.populateSvn()
     self.svn_dirty = False
     return True
 
   def setUpGIT(self):
     """Creates git repositories and start the servers."""
     self.setUp()
     if self.gitdaemon:
       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
 
   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.
+      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()
 
   def populateGit(self):
     raise NotImplementedError()
 
 
 class FakeRepos(FakeReposBase):
   """Implements populateSvn() and populateGit()."""
   NB_GIT_REPOS = 4
@@ skipping 317 matching lines @@
 
 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))