This is an archive of the discontinued Mercurial Phabricator instance.

Differential D7459

lock: pass "success" boolean to _afterlock callbacks
ClosedPublic

Authored by spectral on Nov 19 2019, 10:15 PM.

Details

Reviewers
pulkit
Group Reviewers
hg-reviewers
Commits
rHG888bd39ed555: lock: pass "success" boolean to _afterlock callbacks
rHG4b065b019b4e: lock: pass "success" boolean to _afterlock callbacks
Summary

This lets the callback decide if it should actually run or not. I suspect that
most callbacks (and hooks) *should not* run in this scenario, but I'm trying
to not break any existing behavior. persistmanifestcache, however, seems
actively dangerous to run: we just encountered an exception and the repo is in
an unknown state (hopefully a consistent one due to transactions, but this is
not 100% guaranteed), and the data we cache may be based on this unknown
state.

This was observed by our users since we wrap some of the functions that
persistmanifestcache calls and it expects that the repo object is in a certain
state that we'd set up earlier. If the user hits ctrl-c before we establish
that state, we end up crashing there. I'm going to make that extension
resilient to this issue, but figured it might be a common issue and should be
handled here as well instead of just working around the issue.

Diff Detail

Repository
rHG Mercurial
Branch
default
Lint
No Linters Available
Unit
No Unit Test Coverage

Revision Contents
Changeset List

Diff 18248

hgext/remotefilelog/__init__.py

if not (shallow and bgprefetchrevs and isready): if not (shallow and bgprefetchrevs and isready):
return return
bgrepack = repo.ui.configbool(b'remotefilelog', b'backgroundrepack') bgrepack = repo.ui.configbool(b'remotefilelog', b'backgroundrepack')
# update a revset with a date limit # update a revset with a date limit
bgprefetchrevs = revdatelimit(ui, bgprefetchrevs) bgprefetchrevs = revdatelimit(ui, bgprefetchrevs)
def anon(): def anon(unused_success):
if util.safehasattr(repo, b'ranprefetch') and repo.ranprefetch: if util.safehasattr(repo, b'ranprefetch') and repo.ranprefetch:
return return
repo.ranprefetch = True repo.ranprefetch = True
repo.backgroundprefetch(bgprefetchrevs, repack=bgrepack) repo.backgroundprefetch(bgprefetchrevs, repack=bgrepack)
repo._afterlock(anon) repo._afterlock(anon)

mercurial/bundle2.py

op.repo.hook( op.repo.hook(
b'prepushkey', throw=True, **pycompat.strkwargs(hookargs) b'prepushkey', throw=True, **pycompat.strkwargs(hookargs)
) )
bookstore.applychanges(op.repo, op.gettransaction(), changes) bookstore.applychanges(op.repo, op.gettransaction(), changes)
if pushkeycompat: if pushkeycompat:
def runhook(): def runhook(unused_success):
for hookargs in allhooks: for hookargs in allhooks:
op.repo.hook(b'pushkey', **pycompat.strkwargs(hookargs)) op.repo.hook(b'pushkey', **pycompat.strkwargs(hookargs))
op.repo._afterlock(runhook) op.repo._afterlock(runhook)
elif bookmarksmode == b'records': elif bookmarksmode == b'records':
for book, node in changes: for book, node in changes:
record = {b'bookmark': book, b'node': node} record = {b'bookmark': book, b'node': node}

mercurial/changegroup.py

targetphase = phaseall = phases.draft targetphase = phaseall = phases.draft
if added: if added:
phases.registernew(repo, tr, targetphase, added) phases.registernew(repo, tr, targetphase, added)
if phaseall is not None: if phaseall is not None:
phases.advanceboundary(repo, tr, phaseall, cgnodes) phases.advanceboundary(repo, tr, phaseall, cgnodes)
if changesets > 0: if changesets > 0:
def runhooks(): def runhooks(unused_success):
# These hooks run when the lock releases, not when the # These hooks run when the lock releases, not when the
# transaction closes. So it's possible for the changelog # transaction closes. So it's possible for the changelog
# to have changed since we last saw it. # to have changed since we last saw it.
if clstart >= len(repo): if clstart >= len(repo):
return return
repo.hook(b"changegroup", **pycompat.strkwargs(hookargs)) repo.hook(b"changegroup", **pycompat.strkwargs(hookargs))

mercurial/localrepo.py

"""To be run if transaction is successful, will schedule a hook run """To be run if transaction is successful, will schedule a hook run
""" """
# Don't reference tr2 in hook() so we don't hold a reference. # Don't reference tr2 in hook() so we don't hold a reference.
# This reduces memory consumption when there are multiple # This reduces memory consumption when there are multiple
# transactions per lock. This can likely go away if issue5045 # transactions per lock. This can likely go away if issue5045
# fixes the function accumulation. # fixes the function accumulation.
hookargs = tr2.hookargs hookargs = tr2.hookargs
def hookfunc(): def hookfunc(unused_success):
repo = reporef() repo = reporef()
if hook.hashook(repo.ui, b'txnclose-bookmark'): if hook.hashook(repo.ui, b'txnclose-bookmark'):
bmchanges = sorted(tr.changes[b'bookmarks'].items()) bmchanges = sorted(tr.changes[b'bookmarks'].items())
for name, (old, new) in bmchanges: for name, (old, new) in bmchanges:
args = tr.hookargs.copy() args = tr.hookargs.copy()
args.update(bookmarks.preparehookargs(name, old, new)) args.update(bookmarks.preparehookargs(name, old, new))
repo.hook( repo.hook(
b'txnclose-bookmark', b'txnclose-bookmark',
The callback will be executed when the outermost lock is released The callback will be executed when the outermost lock is released
(with wlock being higher level than 'lock').""" (with wlock being higher level than 'lock')."""
for ref in (self._wlockref, self._lockref): for ref in (self._wlockref, self._lockref):
l = ref and ref() l = ref and ref()
if l and l.held: if l and l.held:
l.postrelease.append(callback) l.postrelease.append(callback)
break break
else: # no lock have been found. else: # no lock have been found.
callback() callback(True)
def lock(self, wait=True): def lock(self, wait=True):
'''Lock the repository store (.hg/store) and return a weak reference '''Lock the repository store (.hg/store) and return a weak reference
to the lock. Use this before modifying the store (e.g. committing or to the lock. Use this before modifying the store (e.g. committing or
stripping). If you are opening a transaction, get a lock as well.) stripping). If you are opening a transaction, get a lock as well.)
If both 'lock' and 'wlock' must be acquired, ensure you always acquires If both 'lock' and 'wlock' must be acquired, ensure you always acquires
'wlock' first to avoid a dead-lock hazard.''' 'wlock' first to avoid a dead-lock hazard.'''
ms.reset() ms.reset()
except: # re-raises except: # re-raises
if edited: if edited:
self.ui.write( self.ui.write(
_(b'note: commit message saved in %s\n') % msgfn _(b'note: commit message saved in %s\n') % msgfn
) )
raise raise
def commithook(): def commithook(unused_success):
# hack for command that use a temporary commit (eg: histedit) # hack for command that use a temporary commit (eg: histedit)
# temporary commit got stripped before hook release # temporary commit got stripped before hook release
if self.changelog.hasnode(ret): if self.changelog.hasnode(ret):
self.hook( self.hook(
b"commit", node=hex(ret), parent1=hookp1, parent2=hookp2 b"commit", node=hex(ret), parent1=hookp1, parent2=hookp2
) )
self._afterlock(commithook) self._afterlock(commithook)
except error.HookAbort as exc: except error.HookAbort as exc:
self.ui.write_err(_(b"pushkey-abort: %s\n") % exc) self.ui.write_err(_(b"pushkey-abort: %s\n") % exc)
if exc.hint: if exc.hint:
self.ui.write_err(_(b"(%s)\n") % exc.hint) self.ui.write_err(_(b"(%s)\n") % exc.hint)
return False return False
self.ui.debug(b'pushing key for "%s:%s"\n' % (namespace, key)) self.ui.debug(b'pushing key for "%s:%s"\n' % (namespace, key))
ret = pushkey.push(self, namespace, key, old, new) ret = pushkey.push(self, namespace, key, old, new)
def runhook(): def runhook(unused_success):
self.hook( self.hook(
b'pushkey', b'pushkey',
namespace=namespace, namespace=namespace,
key=key, key=key,
old=old, old=old,
new=new, new=new,
ret=ret, ret=ret,
) )

mercurial/lock.py

self.delay = self.lock() self.delay = self.lock()
if self.acquirefn: if self.acquirefn:
self.acquirefn() self.acquirefn()
def __enter__(self): def __enter__(self):
return self return self
def __exit__(self, exc_type, exc_value, exc_tb): def __exit__(self, exc_type, exc_value, exc_tb):
self.release() success = all(a is None for a in (exc_type, exc_value, exc_tb))
self.release(success=success)
def __del__(self): def __del__(self):
if self.held: if self.held:
warnings.warn( warnings.warn(
"use lock.release instead of del lock", "use lock.release instead of del lock",
category=DeprecationWarning, category=DeprecationWarning,
stacklevel=2, stacklevel=2,
) )
self._inherited = True self._inherited = True
try: try:
yield lockname yield lockname
finally: finally:
if self.acquirefn: if self.acquirefn:
self.acquirefn() self.acquirefn()
self._inherited = False self._inherited = False
def release(self): def release(self, success=True):
"""release the lock and execute callback function if any """release the lock and execute callback function if any
If the lock has been acquired multiple times, the actual release is If the lock has been acquired multiple times, the actual release is
delayed to the last release call.""" delayed to the last release call."""
if self.held > 1: if self.held > 1:
self.held -= 1 self.held -= 1
elif self.held == 1: elif self.held == 1:
self.held = 0 self.held = 0
if self._getpid() != self.pid: if self._getpid() != self.pid:
# we forked, and are not the parent # we forked, and are not the parent
return return
try: try:
if self.releasefn: if self.releasefn:
self.releasefn() self.releasefn()
finally: finally:
if not self._parentheld: if not self._parentheld:
try: try:
self.vfs.unlink(self.f) self.vfs.unlink(self.f)
except OSError: except OSError:
pass pass
# The postrelease functions typically assume the lock is not held # The postrelease functions typically assume the lock is not held
# at all. # at all.
if not self._parentheld: if not self._parentheld:
for callback in self.postrelease: for callback in self.postrelease:
callback() callback(success)
# Prevent double usage and help clear cycles. # Prevent double usage and help clear cycles.
self.postrelease = None self.postrelease = None
def release(*locks): def release(*locks):
for lock in locks: for lock in locks:
if lock is not None: if lock is not None:
lock.release() lock.release()

mercurial/manifest.py

self._fulltextcache._opener = repo.wcachevfs self._fulltextcache._opener = repo.wcachevfs
if repo._currentlock(repo._wlockref) is None: if repo._currentlock(repo._wlockref) is None:
return return
reporef = weakref.ref(repo) reporef = weakref.ref(repo)
manifestrevlogref = weakref.ref(self) manifestrevlogref = weakref.ref(self)
def persistmanifestcache(): def persistmanifestcache(success):
# Repo is in an unknown state, do not persist.
if not success:
return
repo = reporef() repo = reporef()
self = manifestrevlogref() self = manifestrevlogref()
if repo is None or self is None: if repo is None or self is None:
return return
if repo.manifestlog.getstorage(b'') is not self: if repo.manifestlog.getstorage(b'') is not self:
# there's a different manifest in play now, abort # there's a different manifest in play now, abort
return return
self._fulltextcache.write() self._fulltextcache.write()

tests/test-lock.py

return l return l
def acquirefn(self): def acquirefn(self):
self._acquirecalled = True self._acquirecalled = True
def releasefn(self): def releasefn(self):
self._releasecalled = True self._releasecalled = True
def postreleasefn(self): def postreleasefn(self, success):
self._postreleasecalled = True self._postreleasecalled = True
def assertacquirecalled(self, called): def assertacquirecalled(self, called):
self._testcase.assertEqual( self._testcase.assertEqual(
self._acquirecalled, self._acquirecalled,
called, called,
'expected acquire to be %s but was actually %s' 'expected acquire to be %s but was actually %s'
% (self._tocalled(called), self._tocalled(self._acquirecalled),), % (self._tocalled(called), self._tocalled(self._acquirecalled),),