Reland "Use OS level locking in git_cache.py"
This is a reland of d3affaa62488e0d67e04f810f1c2f2f40793e06b
Original change's description:
> 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/+/2189333
> Reviewed-by: Edward Lesmes <ehmaldonado@chromium.org>
> Reviewed-by: Robbie Iannucci <iannucci@chromium.org>
> Commit-Queue: Josip Sokcevic <sokcevic@google.com>
Bug: 1049610
Change-Id: I58e65a10f7c779e0de1121ba7167c694996e390c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/2211189
Reviewed-by: Edward Lesmes <ehmaldonado@chromium.org>
Reviewed-by: Robbie Iannucci <iannucci@chromium.org>
Commit-Queue: Josip Sokcevic <sokcevic@google.com>
diff --git a/git_cache.py b/git_cache.py
index 0ca9c7d..41bc8e7 100755
--- a/git_cache.py
+++ b/git_cache.py
@@ -26,6 +26,7 @@
from download_from_google_storage import Gsutil
import gclient_utils
+import lockfile
import subcommand
# Analogous to gc.autopacklimit git config.
@@ -40,9 +41,6 @@
class WinErr(Exception):
pass
-class LockError(Exception):
- pass
-
class ClobberNeeded(Exception):
pass
@@ -82,116 +80,6 @@
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 @@
shallow=False,
bootstrap=False,
verbose=False,
- ignore_lock=False,
lock_timeout=0,
reset_fetch_config=False):
assert self.GetCachePath()
@@ -576,25 +463,21 @@
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 <git number>
@@ -665,45 +548,6 @@
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 @@
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,18 @@
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_locks:
+ print('ignore_locks is no longer used. Please remove its usage.')
+ if options.break_locks:
+ print('break_locks 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 +710,10 @@
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):