This is an archive of the discontinued Mercurial Phabricator instance.

Differential D9053

locking: remove support for inheriting locks in subprocess
ClosedPublic

Authored by martinvonz on Sep 18 2020, 12:15 PM.

Details

Reviewers
indygreg
Group Reviewers
hg-reviewers
Commits
rHG9b16bb3b2349: locking: remove support for inheriting locks in subprocess
Summary

This seems to have been added for merge driver, and since merge driver
is now gone...

Diff Detail

Repository
rHG Mercurial
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

martinvonz created this revision.Sep 18 2020, 12:15 PM
indygreg accepted this revision.Sep 22 2020, 10:07 PM
indygreg added a subscriber: indygreg.

Wow - I had no clue this functionality existed! This functionality feels brittle and I'll gladly approve changes to simplify the locking code.

This revision is now accepted and ready to land.Sep 22 2020, 10:07 PM

Revision Contents
Changeset List

PathPackages
Mmercurial/localrepo.py (26 lines)
Mmercurial/lock.py (64 lines)
Mmercurial/scmutil.py (23 lines)
Mtests/test-lock.py (115 lines)

Diff 22761

mercurial/localrepo.py

"""Reload stats of cached files so that they are flagged as valid""" """Reload stats of cached files so that they are flagged as valid"""
for k, ce in self._filecache.items(): for k, ce in self._filecache.items():
k = pycompat.sysstr(k) k = pycompat.sysstr(k)
if k == 'dirstate' or k not in self.__dict__: if k == 'dirstate' or k not in self.__dict__:
continue continue
ce.refresh() ce.refresh()
def _lock( def _lock(
self, self, vfs, lockname, wait, releasefn, acquirefn, desc,
vfs,
lockname,
wait,
releasefn,
acquirefn,
desc,
inheritchecker=None,
parentenvvar=None,
): ):
parentlock = None
# the contents of parentenvvar are used by the underlying lock to
# determine whether it can be inherited
if parentenvvar is not None:
parentlock = encoding.environ.get(parentenvvar)
timeout = 0 timeout = 0
warntimeout = 0 warntimeout = 0
if wait: if wait:
timeout = self.ui.configint(b"ui", b"timeout") timeout = self.ui.configint(b"ui", b"timeout")
warntimeout = self.ui.configint(b"ui", b"timeout.warn") warntimeout = self.ui.configint(b"ui", b"timeout.warn")
# internal config: ui.signal-safe-lock # internal config: ui.signal-safe-lock
signalsafe = self.ui.configbool(b'ui', b'signal-safe-lock') signalsafe = self.ui.configbool(b'ui', b'signal-safe-lock')
l = lockmod.trylock( l = lockmod.trylock(
self.ui, self.ui,
vfs, vfs,
lockname, lockname,
timeout, timeout,
warntimeout, warntimeout,
releasefn=releasefn, releasefn=releasefn,
acquirefn=acquirefn, acquirefn=acquirefn,
desc=desc, desc=desc,
inheritchecker=inheritchecker,
parentlock=parentlock,
signalsafe=signalsafe, signalsafe=signalsafe,
) )
return l return l
def _afterlock(self, callback): def _afterlock(self, callback):
"""add a callback to be run when the repository is fully unlocked """add a callback to be run when the repository is fully unlocked
The callback will be executed when the outermost lock is released The callback will be executed when the outermost lock is released
wait=wait, wait=wait,
releasefn=None, releasefn=None,
acquirefn=self.invalidate, acquirefn=self.invalidate,
desc=_(b'repository %s') % self.origroot, desc=_(b'repository %s') % self.origroot,
) )
self._lockref = weakref.ref(l) self._lockref = weakref.ref(l)
return l return l
def _wlockchecktransaction(self):
if self.currenttransaction() is not None:
raise error.LockInheritanceContractViolation(
b'wlock cannot be inherited in the middle of a transaction'
)
def wlock(self, wait=True): def wlock(self, wait=True):
'''Lock the non-store parts of the repository (everything under '''Lock the non-store parts of the repository (everything under
.hg except .hg/store) and return a weak reference to the lock. .hg except .hg/store) and return a weak reference to the lock.
Use this before modifying files in .hg. Use this before modifying files in .hg.
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.'''
l = self._lock( l = self._lock(
self.vfs, self.vfs,
b"wlock", b"wlock",
wait, wait,
unlock, unlock,
self.invalidatedirstate, self.invalidatedirstate,
_(b'working directory of %s') % self.origroot, _(b'working directory of %s') % self.origroot,
inheritchecker=self._wlockchecktransaction,
parentenvvar=b'HG_WLOCK_LOCKER',
) )
self._wlockref = weakref.ref(l) self._wlockref = weakref.ref(l)
return l return l
def _currentlock(self, lockref): def _currentlock(self, lockref):
"""Returns the lock if it's held, or None if it's not.""" """Returns the lock if it's held, or None if it's not."""
if lockref is None: if lockref is None:
return None return None

mercurial/lock.py

def __init__( def __init__(
self, self,
vfs, vfs,
fname, fname,
timeout=-1, timeout=-1,
releasefn=None, releasefn=None,
acquirefn=None, acquirefn=None,
desc=None, desc=None,
inheritchecker=None,
parentlock=None,
signalsafe=True, signalsafe=True,
dolock=True, dolock=True,
): ):
self.vfs = vfs self.vfs = vfs
self.f = fname self.f = fname
self.held = 0 self.held = 0
self.timeout = timeout self.timeout = timeout
self.releasefn = releasefn self.releasefn = releasefn
self.acquirefn = acquirefn self.acquirefn = acquirefn
self.desc = desc self.desc = desc
self._inheritchecker = inheritchecker
self.parentlock = parentlock
self._parentheld = False
self._inherited = False
if signalsafe: if signalsafe:
self._maybedelayedinterrupt = _delayedinterrupt self._maybedelayedinterrupt = _delayedinterrupt
else: else:
self._maybedelayedinterrupt = util.nullcontextmanager self._maybedelayedinterrupt = util.nullcontextmanager
self.postrelease = [] self.postrelease = []
self.pid = self._getpid() self.pid = self._getpid()
if dolock: if dolock:
self.delay = self.lock() self.delay = self.lock()
self.vfs.makelock(lockname, self.f) self.vfs.makelock(lockname, self.f)
self.held = 1 self.held = 1
except (OSError, IOError) as why: except (OSError, IOError) as why:
if why.errno == errno.EEXIST: if why.errno == errno.EEXIST:
locker = self._readlock() locker = self._readlock()
if locker is None: if locker is None:
continue continue
# special case where a parent process holds the lock -- this
# is different from the pid being different because we do
# want the unlock and postrelease functions to be called,
# but the lockfile to not be removed.
if locker == self.parentlock:
self._parentheld = True
self.held = 1
return
locker = self._testlock(locker) locker = self._testlock(locker)
if locker is not None: if locker is not None:
raise error.LockHeld( raise error.LockHeld(
errno.EAGAIN, errno.EAGAIN,
self.vfs.join(self.f), self.vfs.join(self.f),
self.desc, self.desc,
locker, locker,
) )
not alive, we can safely break lock. not alive, we can safely break lock.
The lock file is only deleted when None is returned. The lock file is only deleted when None is returned.
""" """
locker = self._readlock() locker = self._readlock()
return self._testlock(locker) return self._testlock(locker)
@contextlib.contextmanager
def inherit(self):
"""context for the lock to be inherited by a Mercurial subprocess.
Yields a string that will be recognized by the lock in the subprocess.
Communicating this string to the subprocess needs to be done separately
-- typically by an environment variable.
"""
if not self.held:
raise error.LockInheritanceContractViolation(
b'inherit can only be called while lock is held'
)
if self._inherited:
raise error.LockInheritanceContractViolation(
b'inherit cannot be called while lock is already inherited'
)
if self._inheritchecker is not None:
self._inheritchecker()
if self.releasefn:
self.releasefn()
if self._parentheld:
lockname = self.parentlock
else:
lockname = b'%s:%d' % (lock._host, self.pid)
self._inherited = True
try:
yield lockname
finally:
if self.acquirefn:
self.acquirefn()
self._inherited = False
def release(self, success=True): 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:
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:
for callback in self.postrelease: for callback in self.postrelease:
callback(success) 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/scmutil.py

raise error.Abort( raise error.Abort(
_(b"extdata command '%s' failed: %s") _(b"extdata command '%s' failed: %s")
% (cmd, procutil.explainexit(proc.returncode)) % (cmd, procutil.explainexit(proc.returncode))
) )
return data return data
def _locksub(repo, lock, envvar, cmd, environ=None, *args, **kwargs):
if lock is None:
raise error.LockInheritanceContractViolation(
b'lock can only be inherited while held'
)
if environ is None:
environ = {}
with lock.inherit() as locker:
environ[envvar] = locker
return repo.ui.system(cmd, environ=environ, *args, **kwargs)
def wlocksub(repo, cmd, *args, **kwargs):
"""run cmd as a subprocess that allows inheriting repo's wlock
This can only be called while the wlock is held. This takes all the
arguments that ui.system does, and returns the exit code of the
subprocess."""
return _locksub(
repo, repo.currentwlock(), b'HG_WLOCK_LOCKER', cmd, *args, **kwargs
)
class progress(object): class progress(object):
def __init__(self, ui, updatebar, topic, unit=b"", total=None): def __init__(self, ui, updatebar, topic, unit=b"", total=None):
self.ui = ui self.ui = ui
self.pos = 0 self.pos = 0
self.topic = topic self.topic = topic
self.unit = unit self.unit = unit
self.total = total self.total = total
self.debug = ui.configbool(b'progress', b'debug') self.debug = ui.configbool(b'progress', b'debug')

tests/test-lock.py

state.assertlockexists(True) state.assertlockexists(True)
# release the actual lock # release the actual lock
lock.release() lock.release()
state.assertreleasecalled(True) state.assertreleasecalled(True)
state.assertpostreleasecalled(True) state.assertpostreleasecalled(True)
state.assertlockexists(False) state.assertlockexists(False)
def testinheritlock(self):
d = tempfile.mkdtemp(dir=encoding.getcwd())
parentstate = teststate(self, d)
parentlock = parentstate.makelock()
parentstate.assertacquirecalled(True)
# set up lock inheritance
with parentlock.inherit() as lockname:
parentstate.assertreleasecalled(True)
parentstate.assertpostreleasecalled(False)
parentstate.assertlockexists(True)
childstate = teststate(self, d, pidoffset=1)
childlock = childstate.makelock(parentlock=lockname)
childstate.assertacquirecalled(True)
childlock.release()
childstate.assertreleasecalled(True)
childstate.assertpostreleasecalled(False)
childstate.assertlockexists(True)
parentstate.resetacquirefn()
parentstate.assertacquirecalled(True)
parentlock.release()
parentstate.assertreleasecalled(True)
parentstate.assertpostreleasecalled(True)
parentstate.assertlockexists(False)
def testmultilock(self):
d = tempfile.mkdtemp(dir=encoding.getcwd())
state0 = teststate(self, d)
lock0 = state0.makelock()
state0.assertacquirecalled(True)
with lock0.inherit() as lock0name:
state0.assertreleasecalled(True)
state0.assertpostreleasecalled(False)
state0.assertlockexists(True)
state1 = teststate(self, d, pidoffset=1)
lock1 = state1.makelock(parentlock=lock0name)
state1.assertacquirecalled(True)
# from within lock1, acquire another lock
with lock1.inherit() as lock1name:
# since the file on disk is lock0's this should have the same
# name
self.assertEqual(lock0name, lock1name)
state2 = teststate(self, d, pidoffset=2)
lock2 = state2.makelock(parentlock=lock1name)
state2.assertacquirecalled(True)
lock2.release()
state2.assertreleasecalled(True)
state2.assertpostreleasecalled(False)
state2.assertlockexists(True)
state1.resetacquirefn()
state1.assertacquirecalled(True)
lock1.release()
state1.assertreleasecalled(True)
state1.assertpostreleasecalled(False)
state1.assertlockexists(True)
lock0.release()
def testinheritlockfork(self):
d = tempfile.mkdtemp(dir=encoding.getcwd())
parentstate = teststate(self, d)
parentlock = parentstate.makelock()
parentstate.assertacquirecalled(True)
# set up lock inheritance
with parentlock.inherit() as lockname:
childstate = teststate(self, d, pidoffset=1)
childlock = childstate.makelock(parentlock=lockname)
childstate.assertacquirecalled(True)
# fork the child lock
forkchildlock = copy.copy(childlock)
forkchildlock._pidoffset += 1
forkchildlock.release()
childstate.assertreleasecalled(False)
childstate.assertpostreleasecalled(False)
childstate.assertlockexists(True)
# release the child lock
childlock.release()
childstate.assertreleasecalled(True)
childstate.assertpostreleasecalled(False)
childstate.assertlockexists(True)
parentlock.release()
def testinheritcheck(self):
d = tempfile.mkdtemp(dir=encoding.getcwd())
state = teststate(self, d)
def check():
raise error.LockInheritanceContractViolation('check failed')
lock = state.makelock(inheritchecker=check)
state.assertacquirecalled(True)
with self.assertRaises(error.LockInheritanceContractViolation):
with lock.inherit():
pass
lock.release()
def testfrequentlockunlock(self): def testfrequentlockunlock(self):
"""This tests whether lock acquisition fails as expected, even if """This tests whether lock acquisition fails as expected, even if
(1) lock can't be acquired (makelock fails by EEXIST), and (1) lock can't be acquired (makelock fails by EEXIST), and
(2) locker info can't be read in (readlock fails by ENOENT) while (2) locker info can't be read in (readlock fails by ENOENT) while
retrying 5 times. retrying 5 times.
""" """
d = tempfile.mkdtemp(dir=encoding.getcwd()) d = tempfile.mkdtemp(dir=encoding.getcwd())