From 66a66dd9012b981dc2b6233d1c38fad36a3d4a57 Mon Sep 17 00:00:00 2001 From: Xinan Lin Date: Tue, 7 Oct 2025 13:56:25 -0700 Subject: [PATCH] contains_revision should respect lock_timeout attached by users BUG=449182250,407051026 Change-Id: Icee1725cb0c0f0f4b6a757c4b381d6589486e40d Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/7000923 Reviewed-by: Scott Lee Commit-Queue: Xinan Lin --- gclient_scm.py | 14 ++++++++++---- git_cache.py | 12 ++---------- 2 files changed, 12 insertions(+), 14 deletions(-) diff --git a/gclient_scm.py b/gclient_scm.py index f1ceb1d33d..e692bf2343 100644 --- a/gclient_scm.py +++ b/gclient_scm.py @@ -1337,8 +1337,14 @@ class GitWrapper(SCMWrapper): """ # 'hash' is overloaded and can refer to a SHA-1 hash or refs/changes/*. is_sha = gclient_utils.IsFullGitSha(revision) - - if rev_type == 'hash' and is_sha and mirror.contains_revision(revision): + # If git_cache has read-write lock, the lock_timeout only has to hold + # when another process is sync()-ing. But before that, it blocks both + # sync() and contains_revision() from other processes. + # Override lock_timeout to 20s if users set it to 0, because 20s should + # cover most practical cases. + lock_timeout = getattr(options, 'lock_timeout', 0) + if rev_type == 'hash' and is_sha and mirror.contains_revision( + revision, lock_timeout or 20): if options.verbose: self.Print('skipping mirror update, it has rev=%s already' % revision, @@ -1352,12 +1358,12 @@ class GitWrapper(SCMWrapper): mirror.populate(verbose=False, bootstrap=not getattr(options, 'no_bootstrap', False), depth=depth, - lock_timeout=getattr(options, 'lock_timeout', 0)) + lock_timeout=lock_timeout) # Make sure we've actually fetched the revision we want, but only if it # was specified as an explicit commit hash. if rev_type == 'hash' and is_sha and not mirror.contains_revision( - revision): + revision, lock_timeout or 20): raise gclient_utils.Error(f'Failed to fetch {revision}.') def _Clone(self, revision, url, options): diff --git a/git_cache.py b/git_cache.py index 6224ebcb8a..4c8cc754d6 100755 --- a/git_cache.py +++ b/git_cache.py @@ -313,22 +313,14 @@ class Mirror(object): self.Rename(tempdir, directory) return True - def contains_revision(self, revision): + def contains_revision(self, revision, timeout): if not self.exists(): return False - # This will raise LockError(), if another process is - # 1) sync()-ing or - # 2) calling contains_revision(). - # - # In case (1), the caller is responsible for handling the LockError(). - # As per (2), the below gives 20 secs timeout just to cover most - # practical cases. - # # Ideally, read-write locks should be used, Then, the below would # - acquire the read lock immediately or # - raise LockError() if there is an ongoing sync()-ing. - with lockfile.lock(self.mirror_path, timeout=20): + with lockfile.lock(self.mirror_path, timeout=timeout): # If the sentinent file exists at this point, it indicates that # the bootstrapping process was interrupted, leaving the cache # entries in a bad state.