Fixed race condition in 'repo sync -jN' that would open multiple masters.

This fixes the SSH Control Masters to be managed in a thread-safe
fashion.  This is important because "repo sync -jN" uses threads to
sync more than one repository at the same time.  The problem didn't
show up earlier because it was masked if all of the threads tried to
connect to the same host that was used on the "repo init" line.

docs/manifest-format.txt

@@ -20,12 +20,15 @@ A manifest XML file (e.g. 'default.xml') roughly conforms to the
 following DTD:
 
   <!DOCTYPE manifest [
-    <!ELEMENT manifest (remote*,
+    <!ELEMENT manifest (notice?,
+                        remote*,
                         default?,
                         manifest-server?,
                         remove-project*,
                         project*)>
   
+    <!ELEMENT notice (#PCDATA)>
+  
     <!ELEMENT remote (EMPTY)>
     <!ATTLIST remote name         ID    #REQUIRED>
     <!ATTLIST remote fetch        CDATA #REQUIRED>

git_config.py

@@ -18,6 +18,10 @@ import os
 import re
 import subprocess
 import sys
+try:
+  import threading as _threading
+except ImportError:
+  import dummy_threading as _threading
 import time
 from signal import SIGTERM
 from urllib2 import urlopen, HTTPError
@@ -361,76 +365,97 @@ class RefSpec(object):
 _master_processes = []
 _master_keys = set()
 _ssh_master = True
+_master_keys_lock = None
+
+def init_ssh():
+  """Should be called once at the start of repo to init ssh master handling.
+
+  At the moment, all we do is to create our lock.
+  """
+  global _master_keys_lock
+  assert _master_keys_lock is None, "Should only call init_ssh once"
+  _master_keys_lock = _threading.Lock()
 
 def _open_ssh(host, port=None):
   global _ssh_master
 
-  # Check to see whether we already think that the master is running; if we
+  # Acquire the lock.  This is needed to prevent opening multiple masters for
-  # think it's already running, return right away.
+  # the same host when we're running "repo sync -jN" (for N > 1) _and_ the
-  if port is not None:
+  # manifest <remote fetch="ssh://xyz"> specifies a different host from the
-    key = '%s:%s' % (host, port)
+  # one that was passed to repo init.
-  else:
+  _master_keys_lock.acquire()
-    key = host
-
-  if key in _master_keys:
-    return True
-
-  if not _ssh_master \
-  or 'GIT_SSH' in os.environ \
-  or sys.platform in ('win32', 'cygwin'):
-    # failed earlier, or cygwin ssh can't do this
-    #
-    return False
-
-  # We will make two calls to ssh; this is the common part of both calls.
-  command_base = ['ssh',
-                   '-o','ControlPath %s' % ssh_sock(),
-                   host]
-  if port is not None:
-    command_base[1:1] = ['-p',str(port)]
-
-  # Since the key wasn't in _master_keys, we think that master isn't running.
-  # ...but before actually starting a master, we'll double-check.  This can
-  # be important because we can't tell that that 'git@myhost.com' is the same
-  # as 'myhost.com' where "User git" is setup in the user's ~/.ssh/config file.
-  check_command = command_base + ['-O','check']
   try:
-    Trace(': %s', ' '.join(check_command))
-    check_process = subprocess.Popen(check_command,
-                                     stdout=subprocess.PIPE,
-                                     stderr=subprocess.PIPE)
-    check_process.communicate() # read output, but ignore it...
-    isnt_running = check_process.wait()
 
-    if not isnt_running:
+    # Check to see whether we already think that the master is running; if we
-      # Our double-check found that the master _was_ infact running.  Add to
+    # think it's already running, return right away.
-      # the list of keys.
+    if port is not None:
-      _master_keys.add(key)
+      key = '%s:%s' % (host, port)
+    else:
+      key = host
+
+    if key in _master_keys:
       return True
-  except Exception:
-    # Ignore excpetions.  We we will fall back to the normal command and print
-    # to the log there.
-    pass
 
-  command = command_base[:1] + \
+    if not _ssh_master \
-            ['-M', '-N'] + \
+    or 'GIT_SSH' in os.environ \
-            command_base[1:]
+    or sys.platform in ('win32', 'cygwin'):
-  try:
+      # failed earlier, or cygwin ssh can't do this
-    Trace(': %s', ' '.join(command))
+      #
-    p = subprocess.Popen(command)
+      return False
-  except Exception, e:
-    _ssh_master = False
-    print >>sys.stderr, \
-      '\nwarn: cannot enable ssh control master for %s:%s\n%s' \
-      % (host,port, str(e))
-    return False
 
-  _master_processes.append(p)
+    # We will make two calls to ssh; this is the common part of both calls.
-  _master_keys.add(key)
+    command_base = ['ssh',
-  time.sleep(1)
+                     '-o','ControlPath %s' % ssh_sock(),
-  return True
+                     host]
+    if port is not None:
+      command_base[1:1] = ['-p',str(port)]
+
+    # Since the key wasn't in _master_keys, we think that master isn't running.
+    # ...but before actually starting a master, we'll double-check.  This can
+    # be important because we can't tell that that 'git@myhost.com' is the same
+    # as 'myhost.com' where "User git" is setup in the user's ~/.ssh/config file.
+    check_command = command_base + ['-O','check']
+    try:
+      Trace(': %s', ' '.join(check_command))
+      check_process = subprocess.Popen(check_command,
+                                       stdout=subprocess.PIPE,
+                                       stderr=subprocess.PIPE)
+      check_process.communicate() # read output, but ignore it...
+      isnt_running = check_process.wait()
+
+      if not isnt_running:
+        # Our double-check found that the master _was_ infact running.  Add to
+        # the list of keys.
+        _master_keys.add(key)
+        return True
+    except Exception:
+      # Ignore excpetions.  We we will fall back to the normal command and print
+      # to the log there.
+      pass
+
+    command = command_base[:1] + \
+              ['-M', '-N'] + \
+              command_base[1:]
+    try:
+      Trace(': %s', ' '.join(command))
+      p = subprocess.Popen(command)
+    except Exception, e:
+      _ssh_master = False
+      print >>sys.stderr, \
+        '\nwarn: cannot enable ssh control master for %s:%s\n%s' \
+        % (host,port, str(e))
+      return False
+
+    _master_processes.append(p)
+    _master_keys.add(key)
+    time.sleep(1)
+    return True
+  finally:
+    _master_keys_lock.release()
 
 def close_ssh():
+  global _master_keys_lock
+
   terminate_ssh_clients()
 
   for p in _master_processes:
@@ -449,6 +474,9 @@ def close_ssh():
     except OSError:
       pass
 
+  # We're done with the lock, so we can delete it.
+  _master_keys_lock = None
+
 URI_SCP = re.compile(r'^([^@:]*@?[^:/]{1,}):')
 URI_ALL = re.compile(r'^([a-z][a-z+]*)://([^@/]*@?[^/]*)/')
 

main.py

@@ -28,7 +28,7 @@ import re
 import sys
 
 from trace import SetTrace
-from git_config import close_ssh
+from git_config import init_ssh, close_ssh
 from command import InteractiveCommand
 from command import MirrorSafeCommand
 from command import PagedCommand
@@ -214,6 +214,7 @@ def _Main(argv):
   repo = _Repo(opt.repodir)
   try:
     try:
+      init_ssh()
       repo._Run(argv)
     finally:
       close_ssh()

manifest_xml.py

@@ -107,6 +107,15 @@ class XmlManifest(object):
     root = doc.createElement('manifest')
     doc.appendChild(root)
 
+    # Save out the notice.  There's a little bit of work here to give it the
+    # right whitespace, which assumes that the notice is automatically indented
+    # by 4 by minidom.
+    if self.notice:
+      notice_element = root.appendChild(doc.createElement('notice'))
+      notice_lines = self.notice.splitlines()
+      indented_notice = ('\n'.join(" "*4 + line for line in notice_lines))[4:]
+      notice_element.appendChild(doc.createTextNode(indented_notice))
+
     d = self.default
     sort_remotes = list(self.remotes.keys())
     sort_remotes.sort()
@@ -179,6 +188,11 @@ class XmlManifest(object):
     self._Load()
     return self._default
 
+  @property
+  def notice(self):
+    self._Load()
+    return self._notice
+
   @property
   def manifest_server(self):
     self._Load()
@@ -193,6 +207,7 @@ class XmlManifest(object):
     self._projects = {}
     self._remotes = {}
     self._default = None
+    self._notice = None
     self.branch = None
     self._manifest_server = None
 
@@ -263,6 +278,14 @@ class XmlManifest(object):
     if self._default is None:
       self._default = _Default()
 
+    for node in config.childNodes:
+      if node.nodeName == 'notice':
+        if self._notice is not None:
+          raise ManifestParseError, \
+                'duplicate notice in %s' % \
+                (self.manifestFile)
+        self._notice = self._ParseNotice(node)
+
     for node in config.childNodes:
       if node.nodeName == 'manifest-server':
         url = self._reqatt(node, 'url')
@@ -338,6 +361,45 @@ class XmlManifest(object):
       d.revisionExpr = None
     return d
 
+  def _ParseNotice(self, node):
+    """
+    reads a <notice> element from the manifest file
+
+    The <notice> element is distinct from other tags in the XML in that the
+    data is conveyed between the start and end tag (it's not an empty-element
+    tag).
+
+    The white space (carriage returns, indentation) for the notice element is
+    relevant and is parsed in a way that is based on how python docstrings work.
+    In fact, the code is remarkably similar to here:
+      http://www.python.org/dev/peps/pep-0257/
+    """
+    # Get the data out of the node...
+    notice = node.childNodes[0].data
+
+    # Figure out minimum indentation, skipping the first line (the same line
+    # as the <notice> tag)...
+    minIndent = sys.maxint
+    lines = notice.splitlines()
+    for line in lines[1:]:
+      lstrippedLine = line.lstrip()
+      if lstrippedLine:
+        indent = len(line) - len(lstrippedLine)
+        minIndent = min(indent, minIndent)
+
+    # Strip leading / trailing blank lines and also indentation.
+    cleanLines = [lines[0].strip()]
+    for line in lines[1:]:
+      cleanLines.append(line[minIndent:].rstrip())
+
+    # Clear completely blank lines from front and back...
+    while cleanLines and not cleanLines[0]:
+      del cleanLines[0]
+    while cleanLines and not cleanLines[-1]:
+      del cleanLines[-1]
+
+    return '\n'.join(cleanLines)
+
   def _ParseProject(self, node):
     """
     reads a <project> element from the manifest file

subcmds/sync.py

@@ -361,6 +361,11 @@ uncommitted changes are present' % project.relpath
     if not syncbuf.Finish():
       sys.exit(1)
 
+    # If there's a notice that's supposed to print at the end of the sync, print
+    # it now...
+    if self.manifest.notice:
+      print self.manifest.notice
+
 def _PostRepoUpgrade(manifest):
   for project in manifest.projects.values():
     if project.Exists: