Patchwork [2,of,4] lock: move acquirefn call to inside the lock

login
register
mail settings
Submitter Siddharth Agarwal
Date Sept. 23, 2015, 12:15 a.m.
Message ID <c10a294cdec22c49568d.1442967321@dev6666.prn1.facebook.com>
Download mbox | patch
Permalink /patch/10587/
State Accepted
Commit db4c192cb9b3744c42b85af45ea6c8015d271bcc
Headers show

Comments

Siddharth Agarwal - Sept. 23, 2015, 12:15 a.m.
# HG changeset patch
# User Siddharth Agarwal <sid0@fb.com>
# Date 1442956182 25200
#      Tue Sep 22 14:09:42 2015 -0700
# Node ID c10a294cdec22c49568da7863745085b19d846f7
# Parent  37e382191204560e2cdd8e616c0022907c4f6aa7
lock: move acquirefn call to inside the lock

We're going to need to call it again as part of reinitialization after a
subprocess inherits the lock.
Pierre-Yves David - Sept. 23, 2015, 9:05 a.m.
On 09/22/2015 05:15 PM, Siddharth Agarwal wrote:
> # HG changeset patch
> # User Siddharth Agarwal <sid0@fb.com>
> # Date 1442956182 25200
> #      Tue Sep 22 14:09:42 2015 -0700
> # Node ID c10a294cdec22c49568da7863745085b19d846f7
> # Parent  37e382191204560e2cdd8e616c0022907c4f6aa7
> lock: move acquirefn call to inside the lock
>
> We're going to need to call it again as part of reinitialization after a
> subprocess inherits the lock.

My pinky is telling me that we are about to introduce easy way to create 
reference cycle. What's your plan in this regard?
Siddharth Agarwal - Sept. 23, 2015, 6:47 p.m.
On 9/23/15 2:05 AM, Pierre-Yves David wrote:
>
>
> On 09/22/2015 05:15 PM, Siddharth Agarwal wrote:
>> # HG changeset patch
>> # User Siddharth Agarwal <sid0@fb.com>
>> # Date 1442956182 25200
>> #      Tue Sep 22 14:09:42 2015 -0700
>> # Node ID c10a294cdec22c49568da7863745085b19d846f7
>> # Parent  37e382191204560e2cdd8e616c0022907c4f6aa7
>> lock: move acquirefn call to inside the lock
>>
>> We're going to need to call it again as part of reinitialization after a
>> subprocess inherits the lock.
>
> My pinky is telling me that we are about to introduce easy way to 
> create reference cycle. What's your plan in this regard?

As mentioned on IRC:
- this patch itself introduces no new cycles
- acquirefn could theoretically be used to introduce a cycle, but so 
could the releasefn parameter, which already exists. So we're not making 
things worse.
Pierre-Yves David - Sept. 24, 2015, 1:05 p.m.
On 09/23/2015 11:47 AM, Siddharth Agarwal wrote:
> On 9/23/15 2:05 AM, Pierre-Yves David wrote:
>>
>>
>> On 09/22/2015 05:15 PM, Siddharth Agarwal wrote:
>>> # HG changeset patch
>>> # User Siddharth Agarwal <sid0@fb.com>
>>> # Date 1442956182 25200
>>> #      Tue Sep 22 14:09:42 2015 -0700
>>> # Node ID c10a294cdec22c49568da7863745085b19d846f7
>>> # Parent  37e382191204560e2cdd8e616c0022907c4f6aa7
>>> lock: move acquirefn call to inside the lock
>>>
>>> We're going to need to call it again as part of reinitialization after a
>>> subprocess inherits the lock.
>>
>> My pinky is telling me that we are about to introduce easy way to
>> create reference cycle. What's your plan in this regard?
>
> As mentioned on IRC:
> - this patch itself introduces no new cycles
> - acquirefn could theoretically be used to introduce a cycle, but so
> could the releasefn parameter, which already exists. So we're not making
> things worse.

Okay, then patches 1-2 have been pushed to the clowncopter.

>
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> https://selenic.com/mailman/listinfo/mercurial-devel

Patch

diff --git a/mercurial/localrepo.py b/mercurial/localrepo.py
--- a/mercurial/localrepo.py
+++ b/mercurial/localrepo.py
@@ -1210,7 +1210,8 @@  class localrepository(object):
 
     def _lock(self, vfs, lockname, wait, releasefn, acquirefn, desc):
         try:
-            l = lockmod.lock(vfs, lockname, 0, releasefn=releasefn, desc=desc)
+            l = lockmod.lock(vfs, lockname, 0, releasefn=releasefn,
+                             acquirefn=acquirefn, desc=desc)
         except error.LockHeld as inst:
             if not wait:
                 raise
@@ -1219,10 +1220,9 @@  class localrepository(object):
             # default to 600 seconds timeout
             l = lockmod.lock(vfs, lockname,
                              int(self.ui.config("ui", "timeout", "600")),
-                             releasefn=releasefn, desc=desc)
+                             releasefn=releasefn, acquirefn=acquirefn,
+                             desc=desc)
             self.ui.warn(_("got lock after %s seconds\n") % l.delay)
-        if acquirefn:
-            acquirefn()
         return l
 
     def _afterlock(self, callback):
diff --git a/mercurial/lock.py b/mercurial/lock.py
--- a/mercurial/lock.py
+++ b/mercurial/lock.py
@@ -38,16 +38,20 @@  class lock(object):
 
     _host = None
 
-    def __init__(self, vfs, file, timeout=-1, releasefn=None, desc=None):
+    def __init__(self, vfs, file, timeout=-1, releasefn=None, acquirefn=None,
+                 desc=None):
         self.vfs = vfs
         self.f = file
         self.held = 0
         self.timeout = timeout
         self.releasefn = releasefn
+        self.acquirefn = acquirefn
         self.desc = desc
         self.postrelease  = []
         self.pid = os.getpid()
         self.delay = self.lock()
+        if self.acquirefn:
+            self.acquirefn()
 
     def __del__(self):
         if self.held:
diff --git a/tests/test-lock.py b/tests/test-lock.py
--- a/tests/test-lock.py
+++ b/tests/test-lock.py
@@ -15,23 +15,38 @@  testlockname = 'testlock'
 class teststate(object):
     def __init__(self, testcase):
         self._testcase = testcase
+        self._acquirecalled = False
         self._releasecalled = False
         self._postreleasecalled = False
         d = tempfile.mkdtemp(dir=os.getcwd())
         self.vfs = scmutil.vfs(d, audit=False)
 
     def makelock(self, *args, **kwargs):
-        l = lock.lock(self.vfs, testlockname, releasefn=self.releasefn, *args,
-                      **kwargs)
+        l = lock.lock(self.vfs, testlockname, releasefn=self.releasefn,
+                      acquirefn=self.acquirefn, *args, **kwargs)
         l.postrelease.append(self.postreleasefn)
         return l
 
+    def acquirefn(self):
+        self._acquirecalled = True
+
     def releasefn(self):
         self._releasecalled = True
 
     def postreleasefn(self):
         self._postreleasecalled = True
 
+    def assertacquirecalled(self, called):
+        self._testcase.assertEqual(
+            self._acquirecalled, called,
+            'expected acquire to be %s but was actually %s' % (
+                self._tocalled(called),
+                self._tocalled(self._acquirecalled),
+            ))
+
+    def resetacquirefn(self):
+        self._acquirecalled = False
+
     def assertreleasecalled(self, called):
         self._testcase.assertEqual(
             self._releasecalled, called,
@@ -73,6 +88,7 @@  class testlock(unittest.TestCase):
     def testlock(self):
         state = teststate(self)
         lock = state.makelock()
+        state.assertacquirecalled(True)
         lock.release()
         state.assertreleasecalled(True)
         state.assertpostreleasecalled(True)
@@ -81,7 +97,13 @@  class testlock(unittest.TestCase):
     def testrecursivelock(self):
         state = teststate(self)
         lock = state.makelock()
+        state.assertacquirecalled(True)
+
+        state.resetacquirefn()
         lock.lock()
+        # recursive lock should not call acquirefn again
+        state.assertacquirecalled(False)
+
         lock.release() # brings lock refcount down from 2 to 1
         state.assertreleasecalled(False)
         state.assertpostreleasecalled(False)
@@ -95,6 +117,7 @@  class testlock(unittest.TestCase):
     def testlockfork(self):
         state = teststate(self)
         lock = state.makelock()
+        state.assertacquirecalled(True)
         lock.lock()
         # fake a fork
         lock.pid += 1