Patchwork D9053: locking: remove support for inheriting locks in subprocess

login
register
mail settings
Submitter phabricator
Date Sept. 18, 2020, 4:16 p.m.
Message ID <differential-rev-PHID-DREV-nil7i7mbz2eozqf26dwd-req@mercurial-scm.org>
Download mbox | patch
Permalink /patch/47228/
State Superseded
Headers show

Comments

phabricator - Sept. 18, 2020, 4:16 p.m.
martinvonz created this revision.
Herald added a reviewer: hg-reviewers.
Herald added a subscriber: mercurial-patches.

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

REPOSITORY
  rHG Mercurial

BRANCH
  default

REVISION DETAIL
  https://phab.mercurial-scm.org/D9053

AFFECTED FILES
  mercurial/localrepo.py
  mercurial/lock.py
  mercurial/scmutil.py
  tests/test-lock.py

CHANGE DETAILS




To: martinvonz, #hg-reviewers
Cc: mercurial-patches, mercurial-devel

Patch

diff --git a/tests/test-lock.py b/tests/test-lock.py
--- a/tests/test-lock.py
+++ b/tests/test-lock.py
@@ -169,121 +169,6 @@ 
         state.assertpostreleasecalled(True)
         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):
         """This tests whether lock acquisition fails as expected, even if
         (1) lock can't be acquired (makelock fails by EEXIST), and
diff --git a/mercurial/scmutil.py b/mercurial/scmutil.py
--- a/mercurial/scmutil.py
+++ b/mercurial/scmutil.py
@@ -1735,29 +1735,6 @@ 
     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):
     def __init__(self, ui, updatebar, topic, unit=b"", total=None):
         self.ui = ui
diff --git a/mercurial/lock.py b/mercurial/lock.py
--- a/mercurial/lock.py
+++ b/mercurial/lock.py
@@ -202,8 +202,6 @@ 
         releasefn=None,
         acquirefn=None,
         desc=None,
-        inheritchecker=None,
-        parentlock=None,
         signalsafe=True,
         dolock=True,
     ):
@@ -214,10 +212,6 @@ 
         self.releasefn = releasefn
         self.acquirefn = acquirefn
         self.desc = desc
-        self._inheritchecker = inheritchecker
-        self.parentlock = parentlock
-        self._parentheld = False
-        self._inherited = False
         if signalsafe:
             self._maybedelayedinterrupt = _delayedinterrupt
         else:
@@ -290,14 +284,6 @@ 
                     if locker is None:
                         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)
                     if locker is not None:
                         raise error.LockHeld(
@@ -377,38 +363,6 @@ 
         locker = self._readlock()
         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):
         """release the lock and execute callback function if any
 
@@ -425,18 +379,16 @@ 
                 if self.releasefn:
                     self.releasefn()
             finally:
-                if not self._parentheld:
-                    try:
-                        self.vfs.unlink(self.f)
-                    except OSError:
-                        pass
+                try:
+                    self.vfs.unlink(self.f)
+                except OSError:
+                    pass
             # The postrelease functions typically assume the lock is not held
             # at all.
-            if not self._parentheld:
-                for callback in self.postrelease:
-                    callback(success)
-                # Prevent double usage and help clear cycles.
-                self.postrelease = None
+            for callback in self.postrelease:
+                callback(success)
+            # Prevent double usage and help clear cycles.
+            self.postrelease = None
 
 
 def release(*locks):
diff --git a/mercurial/localrepo.py b/mercurial/localrepo.py
--- a/mercurial/localrepo.py
+++ b/mercurial/localrepo.py
@@ -2678,22 +2678,8 @@ 
             ce.refresh()
 
     def _lock(
-        self,
-        vfs,
-        lockname,
-        wait,
-        releasefn,
-        acquirefn,
-        desc,
-        inheritchecker=None,
-        parentenvvar=None,
+        self, vfs, lockname, wait, releasefn, acquirefn, desc,
     ):
-        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
         warntimeout = 0
         if wait:
@@ -2711,8 +2697,6 @@ 
             releasefn=releasefn,
             acquirefn=acquirefn,
             desc=desc,
-            inheritchecker=inheritchecker,
-            parentlock=parentlock,
             signalsafe=signalsafe,
         )
         return l
@@ -2753,12 +2737,6 @@ 
         self._lockref = weakref.ref(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):
         '''Lock the non-store parts of the repository (everything under
         .hg except .hg/store) and return a weak reference to the lock.
@@ -2796,8 +2774,6 @@ 
             unlock,
             self.invalidatedirstate,
             _(b'working directory of %s') % self.origroot,
-            inheritchecker=self._wlockchecktransaction,
-            parentenvvar=b'HG_WLOCK_LOCKER',
         )
         self._wlockref = weakref.ref(l)
         return l