launchpad-reviewers team mailing list archive

[Maximiliano Bertacchini <maximiliano.bertacchini@xxxxxxxxxxxxx>]

Maximiliano Bertacchini has proposed merging lp:~maxiberta/launchpad/deduplicate-process_exists into lp:launchpad.

Commit message:
Deduplicate code into lp.services.osutils.process_exists().

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~maxiberta/launchpad/deduplicate-process_exists/+merge/342150

Deduplicate code into lp.services.osutils.process_exists().
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~maxiberta/launchpad/deduplicate-process_exists into lp:launchpad.

=== modified file 'lib/lp/scripts/utilities/killservice.py'
--- lib/lp/scripts/utilities/killservice.py	2015-10-26 12:34:50 +0000
+++ lib/lp/scripts/utilities/killservice.py	2018-03-26 21:44:39 +0000
@@ -1,6 +1,6 @@
 #!../bin/py
 
-# Copyright 2009 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2018 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 # This module uses relative imports.
@@ -17,6 +17,7 @@
 
 from lp.services.config import config
 from lp.services.mailman.runmailman import stop_mailman
+from lp.services.osutils import process_exists
 from lp.services.pidfile import (
     get_pid,
     pidfile_path,
@@ -101,17 +102,6 @@
                 pass
 
 
-def process_exists(pid):
-    """True if the given process exists."""
-    try:
-        os.getpgid(pid)
-    except OSError as x:
-        if x.errno == 3:
-            return False
-        logging.error("Unknown exception from getpgid - %s", str(x))
-    return True
-
-
 def wait_for_pids(pids, wait, log):
     """
     Wait until all signalled processes are dead, or until we hit the

=== modified file 'lib/lp/services/osutils.py'
--- lib/lp/services/osutils.py	2017-01-11 18:12:28 +0000
+++ lib/lp/services/osutils.py	2018-03-26 21:44:39 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2017 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2018 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Utilities for doing the sort of thing the os module does."""
@@ -12,6 +12,7 @@
     'get_pid_from_file',
     'open_for_writing',
     'override_environ',
+    'process_exists',
     'remove_if_exists',
     'remove_tree',
     'two_stage_kill',
@@ -194,3 +195,17 @@
         if os.path.isfile(filename) and os.access(filename, os.X_OK):
             return True
     return False
+
+
+def process_exists(pid):
+    """Return True if the specified process already exists."""
+    try:
+        os.kill(pid, 0)
+    except os.error as err:
+        if err.errno == errno.ESRCH:
+            # All is well - the process doesn't exist.
+            return False
+        else:
+            # We got a strange OSError, which we'll pass upwards.
+            raise
+    return True

=== modified file 'lib/lp/services/pidfile.py'
--- lib/lp/services/pidfile.py	2014-01-10 04:30:31 +0000
+++ lib/lp/services/pidfile.py	2018-03-26 21:44:39 +0000
@@ -1,10 +1,9 @@
-# Copyright 2009 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2018 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 __metaclass__ = type
 
 import atexit
-import errno
 import os
 from signal import (
     signal,
@@ -14,6 +13,7 @@
 import tempfile
 
 from lp.services.config import config
+from lp.services.osutils import process_exists
 
 
 def pidfile_path(service_name, use_config=None):
@@ -104,13 +104,9 @@
     # right here at the same time. But that's sufficiently unlikely, and
     # stale PIDs are sufficiently annoying, that it's a reasonable
     # tradeoff.
-    try:
-        os.kill(pid, 0)
-    except OSError as e:
-        if e.errno == errno.ESRCH:
-            remove_pidfile(service_name, use_config)
-            return False
-        raise
+    if not process_exists(pid):
+        remove_pidfile(service_name, use_config)
+        return False
 
     # There's a PID file and we couldn't definitively say that the
     # process no longer exists. Assume that we're locked.

=== modified file 'lib/lp/services/sitesearch/tests/test_googleservice.py'
--- lib/lp/services/sitesearch/tests/test_googleservice.py	2018-03-16 14:50:01 +0000
+++ lib/lp/services/sitesearch/tests/test_googleservice.py	2018-03-26 21:44:39 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2018 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """
@@ -8,10 +8,10 @@
 __metaclass__ = type
 
 
-import errno
 import os
 import unittest
 
+from lp.services.osutils import process_exists
 from lp.services.pidfile import pidfile_path
 from lp.services.sitesearch import googletestservice
 
@@ -37,17 +37,3 @@
         self.assertFalse(
             os.path.exists(filepath),
             "The PID file '%s' should have been deleted." % filepath)
-
-
-def process_exists(pid):
-    """Return True if the specified process already exists."""
-    try:
-        os.kill(pid, 0)
-    except os.error as err:
-        if err.errno == errno.ESRCH:
-            # All is well - the process doesn't exist.
-            return False
-        else:
-            # We got a strange OSError, which we'll pass upwards.
-            raise
-    return True

=== modified file 'lib/lp/services/tests/test_osutils.py'
--- lib/lp/services/tests/test_osutils.py	2017-01-14 00:28:10 +0000
+++ lib/lp/services/tests/test_osutils.py	2018-03-26 21:44:39 +0000
@@ -5,16 +5,21 @@
 
 __metaclass__ = type
 
+import errno
 import os
 import tempfile
 
-from fixtures import EnvironmentVariable
+from fixtures import (
+    EnvironmentVariable,
+    MockPatch,
+    )
 from testtools.matchers import FileContains
 
 from lp.services.osutils import (
     ensure_directory_exists,
     find_on_path,
     open_for_writing,
+    process_exists,
     remove_tree,
     write_file,
     )
@@ -128,3 +133,22 @@
         write_file(os.path.join(bin_dir, "program"), "")
         self.useFixture(EnvironmentVariable("PATH", bin_dir))
         self.assertFalse(find_on_path("program"))
+
+
+class TestProcessExists(TestCase):
+
+    def test_with_process_running(self):
+        pid = os.getpid()
+        self.assertTrue(process_exists(pid))
+
+    def test_with_process_not_running(self):
+        exception = OSError()
+        exception.errno = errno.ESRCH
+        self.useFixture(MockPatch('os.kill', side_effect=exception))
+        self.assertFalse(process_exists(123))
+
+    def test_with_unknown_error(self):
+        exception = OSError()
+        exception.errno = errno.ENOMEM
+        self.useFixture(MockPatch('os.kill', side_effect=exception))
+        self.assertRaises(OSError, process_exists, 123)