diff --git a/gclient.py b/gclient.py index 4480c3fe0..b2f15a772 100755 --- a/gclient.py +++ b/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 diff --git a/gclient_scm.py b/gclient_scm.py index c91d592fa..33684e661 100644 --- a/gclient_scm.py +++ b/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. diff --git a/git_cache.py b/git_cache.py index 0ca9c7ddf..8580be76a 100755 --- a/git_cache.py +++ b/git_cache.py @@ -26,6 +26,7 @@ except ImportError: # For Py3 compatibility from download_from_google_storage import Gsutil import gclient_utils +import lockfile import subcommand # Analogous to gc.autopacklimit git config. @@ -40,9 +41,6 @@ except NameError: class WinErr(Exception): pass -class LockError(Exception): - pass - class ClobberNeeded(Exception): pass @@ -82,116 +80,6 @@ def exponential_backoff_retry(fn, excs=(Exception,), name=None, count=10, sleep_time *= 2 -class Lockfile(object): - """Class to represent a cross-platform process-specific lockfile.""" - - def __init__(self, path, timeout=0): - self.path = os.path.abspath(path) - self.timeout = timeout - self.lockfile = self.path + ".lock" - self.pid = os.getpid() - - def _read_pid(self): - """Read the pid stored in the lockfile. - - Note: This method is potentially racy. By the time it returns the lockfile - may have been unlocked, removed, or stolen by some other process. - """ - try: - with open(self.lockfile, 'r') as f: - pid = int(f.readline().strip()) - except (IOError, ValueError): - pid = None - return pid - - def _make_lockfile(self): - """Safely creates a lockfile containing the current pid.""" - open_flags = (os.O_CREAT | os.O_EXCL | os.O_WRONLY) - fd = os.open(self.lockfile, open_flags, 0o644) - f = os.fdopen(fd, 'w') - print(self.pid, file=f) - f.close() - - def _remove_lockfile(self): - """Delete the lockfile. Complains (implicitly) if it doesn't exist. - - See gclient_utils.py:rmtree docstring for more explanation on the - windows case. - """ - if sys.platform == 'win32': - lockfile = os.path.normcase(self.lockfile) - - def delete(): - exitcode = subprocess.call(['cmd.exe', '/c', - 'del', '/f', '/q', lockfile]) - if exitcode != 0: - raise LockError('Failed to remove lock: %s' % (lockfile,)) - exponential_backoff_retry( - delete, - excs=(LockError,), - name='del [%s]' % (lockfile,)) - else: - os.remove(self.lockfile) - - def lock(self): - """Acquire the lock. - - This will block with a deadline of self.timeout seconds. - """ - elapsed = 0 - while True: - try: - self._make_lockfile() - return - except OSError as e: - if elapsed < self.timeout: - sleep_time = max(10, min(3, self.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 - if e.errno == errno.EEXIST: - raise LockError("%s is already locked" % self.path) - else: - raise LockError("Failed to create %s (err %s)" % (self.path, e.errno)) - - def unlock(self): - """Release the lock.""" - try: - if not self.is_locked(): - raise LockError("%s is not locked" % self.path) - if not self.i_am_locking(): - raise LockError("%s is locked, but not by me" % self.path) - self._remove_lockfile() - except WinErr: - # Windows is unreliable when it comes to file locking. YMMV. - pass - - def break_lock(self): - """Remove the lock, even if it was created by someone else.""" - try: - self._remove_lockfile() - return True - except OSError as exc: - if exc.errno == errno.ENOENT: - return False - else: - raise - - def is_locked(self): - """Test if the file is locked by anyone. - - Note: This method is potentially racy. By the time it returns the lockfile - may have been unlocked, removed, or stolen by some other process. - """ - return os.path.exists(self.lockfile) - - def i_am_locking(self): - """Test if the file is locked by this process.""" - return self.is_locked() and self.pid == self._read_pid() - - class Mirror(object): git_exe = 'git.bat' if sys.platform.startswith('win') else 'git' @@ -568,7 +456,6 @@ class Mirror(object): shallow=False, bootstrap=False, verbose=False, - ignore_lock=False, lock_timeout=0, reset_fetch_config=False): assert self.GetCachePath() @@ -576,25 +463,21 @@ class Mirror(object): depth = 10000 gclient_utils.safe_makedirs(self.GetCachePath()) - lockfile = Lockfile(self.mirror_path, lock_timeout) - if not ignore_lock: - lockfile.lock() - - try: - self._ensure_bootstrapped(depth, bootstrap, reset_fetch_config) - self._fetch(self.mirror_path, verbose, depth, no_fetch_tags, - reset_fetch_config) - except ClobberNeeded: - # This is a major failure, we need to clean and force a bootstrap. - gclient_utils.rmtree(self.mirror_path) - self.print(GIT_CACHE_CORRUPT_MESSAGE) - self._ensure_bootstrapped( - depth, bootstrap, reset_fetch_config, force=True) - self._fetch(self.mirror_path, verbose, depth, no_fetch_tags, - reset_fetch_config) - finally: - if not ignore_lock: - lockfile.unlock() + with lockfile.lock(self.mirror_path, lock_timeout): + try: + self._ensure_bootstrapped(depth, bootstrap, reset_fetch_config) + self._fetch(self.mirror_path, verbose, depth, no_fetch_tags, + reset_fetch_config) + except ClobberNeeded: + # This is a major failure, we need to clean and force a bootstrap. + gclient_utils.rmtree(self.mirror_path) + self.print(GIT_CACHE_CORRUPT_MESSAGE) + self._ensure_bootstrapped(depth, + bootstrap, + reset_fetch_config, + force=True) + self._fetch(self.mirror_path, verbose, depth, no_fetch_tags, + reset_fetch_config) def update_bootstrap(self, prune=False, gc_aggressive=False): # The folder is @@ -665,45 +548,6 @@ class Mirror(object): except OSError: logging.warn('Unable to delete temporary pack file %s' % f) - @classmethod - def BreakLocks(cls, path): - did_unlock = False - lf = Lockfile(path) - if lf.break_lock(): - did_unlock = True - # Look for lock files that might have been left behind by an interrupted - # git process. - lf = os.path.join(path, 'config.lock') - if os.path.exists(lf): - os.remove(lf) - did_unlock = True - cls.DeleteTmpPackFiles(path) - return did_unlock - - def unlock(self): - return self.BreakLocks(self.mirror_path) - - @classmethod - def UnlockAll(cls): - cachepath = cls.GetCachePath() - if not cachepath: - return - dirlist = os.listdir(cachepath) - repo_dirs = set([os.path.join(cachepath, path) for path in dirlist - if os.path.isdir(os.path.join(cachepath, path))]) - for dirent in dirlist: - if dirent.startswith('_cache_tmp') or dirent.startswith('tmp'): - gclient_utils.rm_file_or_tree(os.path.join(cachepath, dirent)) - elif (dirent.endswith('.lock') and - os.path.isfile(os.path.join(cachepath, dirent))): - repo_dirs.add(os.path.join(cachepath, dirent[:-5])) - - unlocked_repos = [] - for repo_dir in repo_dirs: - if cls.BreakLocks(repo_dir): - unlocked_repos.append(repo_dir) - - return unlocked_repos @subcommand.usage('[url of repo to check for caching]') def CMDexists(parser, args): @@ -768,9 +612,10 @@ def CMDpopulate(parser, args): parser.add_option('--no_bootstrap', '--no-bootstrap', action='store_true', help='Don\'t bootstrap from Google Storage') - parser.add_option('--ignore_locks', '--ignore-locks', + parser.add_option('--ignore_locks', + '--ignore-locks', action='store_true', - help='Don\'t try to lock repository') + help='NOOP. This flag will be removed in the future.') parser.add_option('--break-locks', action='store_true', help='Break any existing lock instead of just ignoring it') @@ -780,17 +625,16 @@ def CMDpopulate(parser, args): options, args = parser.parse_args(args) if not len(args) == 1: parser.error('git cache populate only takes exactly one repo url.') + if options.ignore_lock: + print('ignore_lock is no longer used. Please remove its usage.') url = args[0] mirror = Mirror(url, refs=options.ref) - if options.break_locks: - mirror.unlock() kwargs = { 'no_fetch_tags': options.no_fetch_tags, 'verbose': options.verbose, 'shallow': options.shallow, 'bootstrap': not options.no_bootstrap, - 'ignore_lock': options.ignore_locks, 'lock_timeout': options.timeout, 'reset_fetch_config': options.reset_fetch_config, } @@ -864,37 +708,10 @@ def CMDfetch(parser, args): return 0 -@subcommand.usage('[url of repo to unlock, or -a|--all]') +@subcommand.usage('do not use - it is a noop.') def CMDunlock(parser, args): - """Unlock one or all repos if their lock files are still around.""" - parser.add_option('--force', '-f', action='store_true', - help='Actually perform the action') - parser.add_option('--all', '-a', action='store_true', - help='Unlock all repository caches') - options, args = parser.parse_args(args) - if len(args) > 1 or (len(args) == 0 and not options.all): - parser.error('git cache unlock takes exactly one repo url, or --all') - - if not options.force: - cachepath = Mirror.GetCachePath() - lockfiles = [os.path.join(cachepath, path) - for path in os.listdir(cachepath) - if path.endswith('.lock') and os.path.isfile(path)] - parser.error('git cache unlock requires -f|--force to do anything. ' - 'Refusing to unlock the following repo caches: ' - ', '.join(lockfiles)) - - unlocked_repos = [] - if options.all: - unlocked_repos.extend(Mirror.UnlockAll()) - else: - m = Mirror(args[0]) - if m.unlock(): - unlocked_repos.append(m.mirror_path) - - if unlocked_repos: - logging.info('Broke locks on these caches:\n %s' % '\n '.join( - unlocked_repos)) + """This command does nothing.""" + print('This command does nothing and will be removed in the future.') class OptionParser(optparse.OptionParser): diff --git a/lockfile.py b/lockfile.py new file mode 100644 index 000000000..9e4163586 --- /dev/null +++ b/lockfile.py @@ -0,0 +1,116 @@ +# 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() diff --git a/tests/git_cache_test.py b/tests/git_cache_test.py index 42df6a6c2..8cd95aedd 100755 --- a/tests/git_cache_test.py +++ b/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)) diff --git a/tests/lockfile_test.py b/tests/lockfile_test.py new file mode 100755 index 000000000..a29540ece --- /dev/null +++ b/tests/lockfile_test.py @@ -0,0 +1,115 @@ +#!/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)) diff --git a/win32imports.py b/win32imports.py new file mode 100644 index 000000000..6de547112 --- /dev/null +++ b/win32imports.py @@ -0,0 +1,61 @@ +# 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