Use OS level locking in git_cache.py

Without OS level locking it's possible to leave "lock" files on disk
which will prevent next run to acquire those locks. This can easily
happen if SIGKIL is issued.

R=apolito@google.com, ehmaldonado@chromium.org

Bug: 1049610
Change-Id: Id87aa1376b9ea5ff0c2d14f3603636493ed1dd5b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/2189333Reviewed-by: Edward Lesmes <ehmaldonado@chromium.org>
Reviewed-by: Robbie Iannucci <iannucci@chromium.org>
Commit-Queue: Josip Sokcevic <sokcevic@google.com>

gclient.py

@@ -1407,12 +1407,14 @@ The local checkout in %(checkout_path)s reports:
 
 You should ensure that the URL listed in .gclient is correct and either change
 it or fix the checkout.
-'''  % {'checkout_path': os.path.join(self.root_dir, dep.name),
-        'expected_url': dep.url,
-        'expected_scm': dep.GetScmName(),
-        'mirror_string': mirror_string,
-        'actual_url': actual_url,
-        'actual_scm': dep.GetScmName()})
+''' % {
+                  'checkout_path': os.path.join(self.root_dir, dep.name),
+                  'expected_url': dep.url,
+                  'expected_scm': dep.GetScmName(),
+                  'mirror_string': mirror_string,
+                  'actual_url': actual_url,
+                  'actual_scm': dep.GetScmName()
+              })
 
   def SetConfig(self, content):
     assert not self.dependencies
@@ -2687,13 +2689,12 @@ def CMDsync(parser, args):
   parser.add_option('--no_bootstrap', '--no-bootstrap',
                     action='store_true',
                     help='Don\'t bootstrap from Google Storage.')
-  parser.add_option('--ignore_locks', action='store_true',
-                    help='GIT ONLY - Ignore cache locks.')
-  parser.add_option('--break_repo_locks', action='store_true',
-                    help='GIT ONLY - Forcibly remove repo locks (e.g. '
-                      'index.lock). This should only be used if you know for '
-                      'certain that this invocation of gclient is the only '
-                      'thing operating on the git repos (e.g. on a bot).')
+  parser.add_option('--ignore_locks',
+                    action='store_true',
+                    help='No longer used.')
+  parser.add_option('--break_repo_locks',
+                    action='store_true',
+                    help='No longer used.')
   parser.add_option('--lock_timeout', type='int', default=5000,
                     help='GIT ONLY - Deadline (in seconds) to wait for git '
                          'cache lock to become available. Default is %default.')
@@ -2714,6 +2715,13 @@ def CMDsync(parser, args):
   if not client:
     raise gclient_utils.Error('client not configured; see \'gclient config\'')
 
+  if options.ignore_locks:
+    print('Warning: ignore_locks is no longer used. Please remove its usage.')
+
+  if options.break_repo_locks:
+    print('Warning: break_repo_locks is no longer used. Please remove its '
+          'usage.')
+
   if options.revisions and options.head:
     # TODO(maruel): Make it a parser.error if it doesn't break any builder.
     print('Warning: you cannot use both --head and --revision')
@@ -2784,12 +2792,14 @@ def CMDrevert(parser, args):
                     help='don\'t run pre-DEPS hooks', default=False)
   parser.add_option('--upstream', action='store_true',
                     help='Make repo state match upstream branch.')
-  parser.add_option('--break_repo_locks', action='store_true',
-                    help='GIT ONLY - Forcibly remove repo locks (e.g. '
-                      'index.lock). This should only be used if you know for '
-                      'certain that this invocation of gclient is the only '
-                      'thing operating on the git repos (e.g. on a bot).')
+  parser.add_option('--break_repo_locks',
+                    action='store_true',
+                    help='No longer used.')
   (options, args) = parser.parse_args(args)
+  if options.break_repo_locks:
+    print('Warning: break_repo_locks is no longer used. Please remove its ' +
+          'usage.')
+
   # --force is implied.
   options.force = True
   options.reset = False

gclient_scm.py

@@ -982,9 +982,7 @@ class GitWrapper(SCMWrapper):
     mirror.populate(verbose=options.verbose,
                     bootstrap=not getattr(options, 'no_bootstrap', False),
                     depth=depth,
-                    ignore_lock=getattr(options, 'ignore_locks', False),
                     lock_timeout=getattr(options, 'lock_timeout', 0))
-    mirror.unlock()
 
   def _Clone(self, revision, url, options):
     """Clone a git repository from the given URL.

lockfile.py

+# Copyright 2020 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.
+"""Exclusive filelocking for all supported platforms."""
+
+from __future__ import print_function
+
+import contextlib
+import logging
+import os
+import sys
+import time
+
+
+class LockError(Exception):
+  pass
+
+
+if sys.platform.startswith('win'):
+  # Windows implementation
+  import win32imports
+
+  BYTES_TO_LOCK = 1
+
+  def _open_file(lockfile):
+    return win32imports.Handle(
+        win32imports.CreateFileW(
+            lockfile,  # lpFileName
+            win32imports.GENERIC_WRITE,  # dwDesiredAccess
+            0,  # dwShareMode=prevent others from opening file
+            None,  # lpSecurityAttributes
+            win32imports.CREATE_ALWAYS,  # dwCreationDisposition
+            win32imports.FILE_ATTRIBUTE_NORMAL,  # dwFlagsAndAttributes
+            None  # hTemplateFile
+        ))
+
+  def _close_file(handle):
+    # CloseHandle releases lock too.
+    win32imports.CloseHandle(handle)
+
+  def _lock_file(handle):
+    ret = win32imports.LockFileEx(
+        handle,  # hFile
+        win32imports.LOCKFILE_FAIL_IMMEDIATELY
+        | win32imports.LOCKFILE_EXCLUSIVE_LOCK,  # dwFlags
+        0,  #dwReserved
+        BYTES_TO_LOCK,  # nNumberOfBytesToLockLow
+        0,  # nNumberOfBytesToLockHigh
+        win32imports.Overlapped()  # lpOverlapped
+    )
+    # LockFileEx returns result as bool, which is converted into an integer
+    # (1 == successful; 0 == not successful)
+    if ret == 0:
+      error_code = win32imports.GetLastError()
+      raise OSError('Failed to lock handle (error code: %d).' % error_code)
+else:
+  # Unix implementation
+  import fcntl
+
+  def _open_file(lockfile):
+    open_flags = (os.O_CREAT | os.O_WRONLY)
+    return os.open(lockfile, open_flags, 0o644)
+
+  def _close_file(fd):
+    os.close(fd)
+
+  def _lock_file(fd):
+    fcntl.flock(fd, fcntl.LOCK_EX | fcntl.LOCK_NB)
+
+
+def _try_lock(lockfile):
+  f = _open_file(lockfile)
+  try:
+    _lock_file(f)
+  except Exception:
+    _close_file(f)
+    raise
+  return lambda: _close_file(f)
+
+
+def _lock(path, timeout=0):
+  """_lock returns function to release the lock if locking was successful.
+
+  _lock also implements simple retry logic."""
+  elapsed = 0
+  while True:
+    try:
+      return _try_lock(path + '.locked')
+    except (OSError, IOError) as e:
+      if elapsed < timeout:
+        sleep_time = min(10, timeout - elapsed)
+        logging.info(
+            'Could not create git cache lockfile; '
+            'will retry after sleep(%d).', sleep_time)
+        elapsed += sleep_time
+        time.sleep(sleep_time)
+        continue
+      raise LockError("Error locking %s (err: %s)" % (path, str(e)))
+
+
+@contextlib.contextmanager
+def lock(path, timeout=0):
+  """Get exclusive lock to path.
+
+  Usage:
+    import lockfile
+    with lockfile.lock(path, timeout):
+      # Do something
+      pass
+
+   """
+  release_fn = _lock(path, timeout)
+  try:
+    yield
+  finally:
+    release_fn()

tests/git_cache_test.py

@@ -5,6 +5,7 @@
 
 """Unit tests for git_cache.py"""
 
+import logging
 import os
 import shutil
 import subprocess
@@ -245,6 +246,8 @@ class MirrorTest(unittest.TestCase):
 
 
 if __name__ == '__main__':
+  logging.basicConfig(
+      level=logging.DEBUG if '-v' in sys.argv else logging.ERROR)
   sys.exit(coverage_utils.covered_main((
     os.path.join(DEPOT_TOOLS_ROOT, 'git_cache.py')
   ), required_percentage=0))

tests/lockfile_test.py

+#!/usr/bin/env vpython3
+# Copyright 2020 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.
+"""Unit tests for lockfile.py"""
+
+import logging
+import os
+import shutil
+import sys
+import tempfile
+import threading
+import unittest
+
+if sys.version_info.major == 2:
+  import mock
+  import Queue
+else:
+  from unittest import mock
+  import queue as Queue
+
+DEPOT_TOOLS_ROOT = os.path.dirname(os.path.dirname(os.path.abspath(__file__)))
+sys.path.insert(0, DEPOT_TOOLS_ROOT)
+
+from testing_support import coverage_utils
+
+import lockfile
+
+
+class LockTest(unittest.TestCase):
+  def setUp(self):
+    self.cache_dir = tempfile.mkdtemp(prefix='lockfile')
+    self.addCleanup(shutil.rmtree, self.cache_dir, ignore_errors=True)
+
+  def testLock(self):
+    with lockfile.lock(self.cache_dir):
+      # cached dir locked, attempt to lock it again
+      with self.assertRaises(lockfile.LockError):
+        with lockfile.lock(self.cache_dir):
+          pass
+
+    with lockfile.lock(self.cache_dir):
+      pass
+
+  @mock.patch('time.sleep')
+  def testLockConcurrent(self, sleep_mock):
+    '''testLockConcurrent simulates what happens when two separate processes try
+    to acquire the same file lock with timeout.'''
+    # Queues q_f1 and q_sleep are used to controll execution of individual
+    # threads.
+    q_f1 = Queue.Queue()
+    q_sleep = Queue.Queue()
+    results = Queue.Queue()
+
+    def side_effect(arg):
+      '''side_effect is called when with l.lock is blocked. In this unit test
+      case, it comes from f2.'''
+      logging.debug('sleep: started')
+      q_sleep.put(True)
+      logging.debug('sleep: waiting for q_sleep to be consumed')
+      q_sleep.join()
+      logging.debug('sleep: waiting for result before exiting')
+      results.get(timeout=1)
+      logging.debug('sleep: exiting')
+
+    sleep_mock.side_effect = side_effect
+
+    def f1():
+      '''f1 enters first in l.lock (controlled via q_f1). It then waits for
+      side_effect to put a message in queue q_sleep.'''
+      logging.debug('f1 started, locking')
+
+      with lockfile.lock(self.cache_dir, timeout=1):
+        logging.debug('f1: locked')
+        q_f1.put(True)
+        logging.debug('f1: waiting on q_f1 to be consumed')
+        q_f1.join()
+        logging.debug('f1: done waiting on q_f1, getting q_sleep')
+        q_sleep.get(timeout=1)
+        results.put(True)
+
+      logging.debug('f1: lock released')
+      q_sleep.task_done()
+      logging.debug('f1: exiting')
+
+    def f2():
+      '''f2 enters second in l.lock (controlled by q_f1).'''
+      logging.debug('f2: started, consuming q_f1')
+      q_f1.get(timeout=1)  # wait for f1 to execute lock
+      q_f1.task_done()
+      logging.debug('f2: done waiting for q_f1, locking')
+
+      with lockfile.lock(self.cache_dir, timeout=1):
+        logging.debug('f2: locked')
+        results.put(True)
+
+    t1 = threading.Thread(target=f1)
+    t1.start()
+    t2 = threading.Thread(target=f2)
+    t2.start()
+    t1.join()
+    t2.join()
+
+    # One result was consumed by side_effect, we expect only one in the queue.
+    self.assertEqual(1, results.qsize())
+    sleep_mock.assert_called_once_with(1)
+
+
+if __name__ == '__main__':
+  logging.basicConfig(
+      level=logging.DEBUG if '-v' in sys.argv else logging.ERROR)
+  sys.exit(
+      coverage_utils.covered_main(
+          (os.path.join(DEPOT_TOOLS_ROOT, 'git_cache.py')),
+          required_percentage=0))

win32imports.py

+# Copyright 2020 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.
+"""Win32 functions and constants."""
+
+import ctypes
+import ctypes.wintypes
+
+GENERIC_WRITE = 0x40000000
+CREATE_ALWAYS = 0x00000002
+FILE_ATTRIBUTE_NORMAL = 0x00000080
+LOCKFILE_EXCLUSIVE_LOCK = 0x00000002
+LOCKFILE_FAIL_IMMEDIATELY = 0x00000001
+
+
+class Overlapped(ctypes.Structure):
+  """Overlapped is required and used in LockFileEx and UnlockFileEx."""
+  _fields_ = [('Internal', ctypes.wintypes.LPVOID),
+              ('InternalHigh', ctypes.wintypes.LPVOID),
+              ('Offset', ctypes.wintypes.DWORD),
+              ('OffsetHigh', ctypes.wintypes.DWORD),
+              ('Pointer', ctypes.wintypes.LPVOID),
+              ('hEvent', ctypes.wintypes.HANDLE)]
+
+
+# https://docs.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-createfilew
+CreateFileW = ctypes.windll.kernel32.CreateFileW
+CreateFileW.argtypes = [
+    ctypes.wintypes.LPCWSTR,  # lpFileName
+    ctypes.wintypes.DWORD,  # dwDesiredAccess
+    ctypes.wintypes.DWORD,  # dwShareMode
+    ctypes.wintypes.LPVOID,  # lpSecurityAttributes
+    ctypes.wintypes.DWORD,  # dwCreationDisposition
+    ctypes.wintypes.DWORD,  # dwFlagsAndAttributes
+    ctypes.wintypes.LPVOID,  # hTemplateFile
+]
+CreateFileW.restype = ctypes.wintypes.HANDLE
+
+# https://docs.microsoft.com/en-us/windows/win32/api/handleapi/nf-handleapi-closehandle
+CloseHandle = ctypes.windll.kernel32.CloseHandle
+CloseHandle.argtypes = [
+    ctypes.wintypes.HANDLE,  # hFile
+]
+CloseHandle.restype = ctypes.wintypes.BOOL
+
+# https://docs.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-lockfileex
+LockFileEx = ctypes.windll.kernel32.LockFileEx
+LockFileEx.argtypes = [
+    ctypes.wintypes.HANDLE,  # hFile
+    ctypes.wintypes.DWORD,  # dwFlags
+    ctypes.wintypes.DWORD,  # dwReserved
+    ctypes.wintypes.DWORD,  # nNumberOfBytesToLockLow
+    ctypes.wintypes.DWORD,  # nNumberOfBytesToLockHigh
+    ctypes.POINTER(Overlapped),  # lpOverlapped
+]
+LockFileEx.restype = ctypes.wintypes.BOOL
+
+# Commonly used functions are listed here so callers don't need to import
+# ctypes.
+GetLastError = ctypes.GetLastError
+Handle = ctypes.wintypes.HANDLE