jenkins-bot merged this change.

View Change

[IMPR] Rewrite log_page/unlock_page implementation

- Use threading.Condition context manager instead of try/finally
- move test_lock_page from TestSiteGenerators to TestLockingPage
  class because this test is not related to a generator

Change-Id: I1ff2c39d43b78d7e71d27720e8ec83e3a48a6925
---
M pywikibot/site.py
M tests/site_tests.py
2 files changed, 24 insertions(+), 43 deletions(-)

diff --git a/pywikibot/site.py b/pywikibot/site.py
index fac1d66..f8950f6 100644
--- a/pywikibot/site.py
+++ b/pywikibot/site.py
@@ -6,7 +6,7 @@
 groups of wikis on the same topic in different languages.
 """
 #
-# (C) Pywikibot team, 2008-2019
+# (C) Pywikibot team, 2008-2020
 #
 # Distributed under the terms of the MIT license.
 #
@@ -780,7 +780,7 @@
             self.code in self.family.use_hard_category_redirects)
 
         # following are for use with lock_page and unlock_page methods
-        self._pagemutex = threading.Lock()
+        self._pagemutex = threading.Condition()
         self._locked_pages = set()
 
     @deprecated(since='20141225')
@@ -880,7 +880,7 @@
     def __setstate__(self, attrs):
         """Restore things removed in __getstate__."""
         self.__dict__.update(attrs)
-        self._pagemutex = threading.Lock()
+        self._pagemutex = threading.Condition()
 
     def user(self):
         """Return the currently-logged in bot username, or None."""
@@ -1065,29 +1065,12 @@
 
         """
         title = page.title(with_section=False)
-
-        self._pagemutex.acquire()
-        try:
+        with self._pagemutex:
             while title in self._locked_pages:
                 if not block:
                     raise PageInUse(title)
-
-                # The mutex must be released so that page can be unlocked
-                self._pagemutex.release()
-                time.sleep(.25)
-                self._pagemutex.acquire()
-
+                self._pagemutex.wait()
             self._locked_pages.add(title)
-        finally:
-            # time.sleep may raise an exception from signal handler (eg:
-            # KeyboardInterrupt) while the lock is released, and there is no
-            # reason to acquire the lock again given that our caller will
-            # receive the exception. The state of the lock is therefore
-            # undefined at the point of this finally block.
-            try:
-                self._pagemutex.release()
-            except RuntimeError:
-                pass
 
     def unlock_page(self, page):
         """
@@ -1097,11 +1080,9 @@
         @type page: pywikibot.Page
 
         """
-        self._pagemutex.acquire()
-        try:
+        with self._pagemutex:
             self._locked_pages.discard(page.title(with_section=False))
-        finally:
-            self._pagemutex.release()
+            self._pagemutex.notify_all()
 
     def disambcategory(self):
         """Return Category in which disambig pages are listed."""
diff --git a/tests/site_tests.py b/tests/site_tests.py
index d700c51..afa9a02 100644
--- a/tests/site_tests.py
+++ b/tests/site_tests.py
@@ -1,7 +1,7 @@
 # -*- coding: utf-8 -*-
 """Tests for the site module."""
 #
-# (C) Pywikibot team, 2008-2019
+# (C) Pywikibot team, 2008-2020
 #
 # Distributed under the terms of the MIT license.
 #
@@ -974,19 +974,6 @@
             self.assertIsInstance(link, pywikibot.Page)
             self.assertIn(link.namespace(), (2, 3))
 
-    def test_lock_page(self):
-        """Test the site.lock_page() and site.unlock_page() method."""
-        site = self.get_site()
-        p1 = pywikibot.Page(site, 'Foo')
-
-        site.lock_page(page=p1, block=True)
-        self.assertRaises(pywikibot.site.PageInUse, site.lock_page, page=p1,
-                          block=False)
-        site.unlock_page(page=p1)
-        # verify it's unlocked
-        site.lock_page(page=p1, block=False)
-        site.unlock_page(page=p1)
-
     def test_protectedpages_create(self):
         """Test that protectedpages returns protected page titles."""
         if self.site.mw_version < '1.15':
@@ -1089,7 +1076,7 @@
         self.assertRaises(AssertionError, func, 'm', 2, 1, True, True)
 
 
-class TestThreadsLockingPage(DefaultSiteTestCase):
+class TestLockingPage(DefaultSiteTestCase):
     """Test cases for lock/unlock a page within threads."""
 
     cached = True
@@ -1102,8 +1089,8 @@
         time.sleep(wait)
         page.site.unlock_page(page=page)
 
-    def test_lock_page(self):
-        """Test the site.lock_page() and site.unlock_page() method."""
+    def test_threads_locking_page(self):
+        """Test lock_page and unlock_page methods for multiple threads."""
         # Start few threads
         threads = []
         for i in range(5):
@@ -1121,6 +1108,19 @@
                 self.assertFalse(thread.is_alive(),
                                  'test page is still locked')
 
+    def test_lock_page(self):
+        """Test the site.lock_page() and site.unlock_page() method."""
+        site = self.get_site()
+        p1 = pywikibot.Page(site, 'Foo')
+
+        site.lock_page(page=p1, block=True)
+        self.assertRaises(pywikibot.site.PageInUse, site.lock_page, page=p1,
+                          block=False)
+        site.unlock_page(page=p1)
+        # verify it's unlocked
+        site.lock_page(page=p1, block=False)
+        site.unlock_page(page=p1)
+
 
 class TestSiteGeneratorsUsers(DefaultSiteTestCase):
     """Test cases for Site methods with users."""

To view, visit change 541089. To unsubscribe, or for help writing mail filters, visit settings.