Merge lp:~bigkevmcd/landscape-client/517453-process-race-condition into lp:~landscape/landscape-client/trunk

[Kevin McDermott]

[Sidnei da Silva (sidnei)]

[1] tempfile and shutil don't seem to be used (at least in the patch).

Looks good, +1.

review: Approve

[lp:~bigkevmcd/landscape-client/517453-process-race-condition]

202. By Kevin McDermott on 2010-02-05

Remove extraneous imports...

[Jamu Kakar (jkakar)]

Nice work, +1!

review: Approve

[Thomas Herve (therve)]

[1] I think this patch will break on dapper which is using Python 2.4.

[2] There is still an uncovered part when the stat file is read. I suggest having a try/except which covers the whole function, with small try/finally to close file handles, instead of trying to catch errors one by one.

review: Needs Fixing

[lp:~bigkevmcd/landscape-client/517453-process-race-condition]

203. By Kevin McDermott on 2010-02-08

Add full process read try:except coverage.

204. By Kevin McDermott on 2010-02-08

Make python2.4 compatible.

205. By Kevin McDermott on 2010-02-08

Add Docstring.

[Thomas Herve (therve)]

Cool! Just one last thing:

[3] As you regrouped with only one finally, you need to close the file after each operation.

+1!

review: Approve

[lp:~bigkevmcd/landscape-client/517453-process-race-condition]

206. By Kevin McDermott on 2010-02-08

Remove the try: finally: and close files manually.

207. By Kevin McDermott on 2010-02-08

Revert the removal.

208. By Kevin McDermott on 2010-02-08

Wrap each access in try: finally: and allow the except: to handle any failure.

209. By Kevin McDermott on 2010-02-08

Add assertions to check that all files get closed after being opened.

=== modified file 'landscape/lib/process.py'
--- landscape/lib/process.py	2008-08-13 14:39:53 +0000
+++ landscape/lib/process.py	2010-02-08 15:30:35 +0000
@@ -47,88 +47,83 @@
                 yield process_info
 
     def get_process_info(self, process_id):
+        """
+        Parse the /proc/<pid>/cmdline and /proc/<pid>/status files for
+        information about the running process with process_id.
+
+        The /proc filesystem doesn't behave like ext2, open files can disappear
+        during the read process.
+        """
         cmd_line_name = ""
         process_dir = os.path.join(self._proc_dir, str(process_id))
         process_info = {"pid": process_id}
 
         try:
             file = open(os.path.join(process_dir, "cmdline"), "r")
-        except IOError:
-            # Handle the race that happens when we find a process
-            # which terminates before we open the stat file.
-            return None
-
-        try:
-            # cmdline is a \0 separated list of strings
-            # We take the first, and then strip off the path, leaving us with
-            # the basename.
-            cmd_line = file.readline()
-            cmd_line_name = os.path.basename(cmd_line.split("\0")[0])
-        finally:
-            file.close()
-
-        try:
+            try:
+                # cmdline is a \0 separated list of strings
+                # We take the first, and then strip off the path, leaving us with
+                # the basename.
+                cmd_line = file.readline()
+                cmd_line_name = os.path.basename(cmd_line.split("\0")[0])
+            finally:
+                file.close()
+
             file = open(os.path.join(process_dir, "status"), "r")
-        except IOError:
-            # Handle the race that happens when we find a process
-            # which terminates before we open the status file.
-            return None
-
-        try:
-            for line in file:
-                parts = line.split(":", 1)
-                if parts[0] == "Name":
-                    process_info["name"] = (cmd_line_name.strip() or
-                                            parts[1].strip())
-                elif parts[0] == "State":
-                    state = parts[1].strip()
-                    process_info["state"] = STATES[state]
-                elif parts[0] == "Uid":
-                    value_parts = parts[1].split()
-                    process_info["uid"] = int(value_parts[0])
-                elif parts[0] == "Gid":
-                    value_parts = parts[1].split()
-                    process_info["gid"] = int(value_parts[0])
-                elif parts[0] == "VmSize":
-                    value_parts = parts[1].split()
-                    process_info["vm-size"] = int(value_parts[0])
-                    break
-        finally:
-            file.close()
-
-        try:
+            try:
+                for line in file:
+                    parts = line.split(":", 1)
+                    if parts[0] == "Name":
+                        process_info["name"] = (cmd_line_name.strip() or
+                                                parts[1].strip())
+                    elif parts[0] == "State":
+                        state = parts[1].strip()
+                        process_info["state"] = STATES[state]
+                    elif parts[0] == "Uid":
+                        value_parts = parts[1].split()
+                        process_info["uid"] = int(value_parts[0])
+                    elif parts[0] == "Gid":
+                        value_parts = parts[1].split()
+                        process_info["gid"] = int(value_parts[0])
+                    elif parts[0] == "VmSize":
+                        value_parts = parts[1].split()
+                        process_info["vm-size"] = int(value_parts[0])
+                        break
+            finally:
+                file.close()
+
             file = open(os.path.join(process_dir, "stat"), "r")
+            try:
+                # These variable names are lifted directly from proc(5)
+                # utime: The number of jiffies that this process has been scheduled in
+                # user mode.
+                # stime: The number of jiffies that this process has been scheduled in
+                # kernel mode.
+                # cutime: The number of jiffies that this process's waited-for children
+                # have been scheduled in user mode.
+                # cstime: The number of jiffies that this process's waited-for children
+                # have been scheduled in kernel mode.
+                parts = file.read().split()
+                start_time = int(parts[21])
+                utime = int(parts[13])
+                stime = int(parts[14])
+                uptime = self._uptime or get_uptime()
+                pcpu = calculate_pcpu(utime, stime, uptime,
+                                      start_time, self._jiffies_per_sec)
+                process_info["percent-cpu"] = pcpu
+                delta = timedelta(0, start_time // self._jiffies_per_sec)
+                if self._boot_time is None:
+                   logging.warning("Skipping process (PID %s) without boot time.")
+                   return None
+                process_info["start-time"] = to_timestamp(self._boot_time  + delta)
+            finally:
+                file.close()
+
         except IOError:
             # Handle the race that happens when we find a process
             # which terminates before we open the stat file.
             return None
 
-        # These variable names are lifted directly from proc(5)
-        # utime: The number of jiffies that this process has been scheduled in
-        # user mode.
-        # stime: The number of jiffies that this process has been scheduled in
-        # kernel mode.
-        # cutime: The number of jiffies that this process's waited-for children
-        # have been scheduled in user mode.
-        # cstime: The number of jiffies that this process's waited-for children
-        # have been scheduled in kernel mode.
-        try:
-            parts = file.read().split()
-            start_time = int(parts[21])
-            utime = int(parts[13])
-            stime = int(parts[14])
-            uptime = self._uptime or get_uptime()
-            pcpu = calculate_pcpu(utime, stime, uptime,
-                                  start_time, self._jiffies_per_sec)
-            process_info["percent-cpu"] = pcpu
-            delta = timedelta(0, start_time // self._jiffies_per_sec)
-            if self._boot_time is None:
-                logging.warning("Skipping process (PID %s) without boot time.")
-                return None
-            process_info["start-time"] = to_timestamp(self._boot_time  + delta)
-        finally:
-            file.close()
-
         assert("pid" in process_info and "state" in process_info
                and "name" in process_info and "uid" in process_info
                and "gid" in process_info and "start-time" in process_info)

=== modified file 'landscape/lib/tests/test_process.py'
--- landscape/lib/tests/test_process.py	2008-06-25 08:01:39 +0000
+++ landscape/lib/tests/test_process.py	2010-02-08 15:30:35 +0000
@@ -2,7 +2,55 @@
 
 from landscape.tests.helpers import LandscapeTest
 
-from landscape.lib.process import calculate_pcpu
+from landscape.lib.process import calculate_pcpu, ProcessInformation
+
+
+class ProcessInfoTest(LandscapeTest):
+
+    def test_missing_process_race(self):
+        """
+        We use os.listdir("/proc") to get the list of active processes, if a
+        process ends before we attempt to read the process' information, then
+        this should not trigger an error. 
+        """
+        listdir_mock = self.mocker.replace("os.listdir")
+        listdir_mock("/proc")
+        self.mocker.result(["12345"])
+
+        class FakeFile(object):
+
+            def __init__(self, response=""):
+                self._response = response
+                self.closed = False
+
+            def readline(self):
+                return self._response
+
+            def __iter__(self):
+                if self._response is None:
+                    raise IOError("Fake file error")
+                else:
+                    yield self._response
+
+            def close(self):
+                self.closed = True
+
+        open_mock = self.mocker.replace("__builtin__.open")
+        open_mock("/proc/12345/cmdline", "r")
+        fakefile1 = FakeFile("test-binary")
+        self.mocker.result(fakefile1)
+
+        open_mock("/proc/12345/status", "r")
+        fakefile2 = FakeFile(None)
+        self.mocker.result(fakefile2)
+
+        self.mocker.replay()
+
+        process_info = ProcessInformation("/proc")
+        processes = list(process_info.get_all_process_info())
+        self.assertEquals(processes, [])
+        self.assertTrue(fakefile1.closed)
+        self.assertTrue(fakefile2.closed)
 
 
 class CalculatePCPUTest(unittest.TestCase):

=== modified file 'landscape/monitor/activeprocessinfo.py'
--- landscape/monitor/activeprocessinfo.py	2009-10-06 16:27:23 +0000
+++ landscape/monitor/activeprocessinfo.py	2010-02-08 15:30:35 +0000
@@ -41,7 +41,7 @@
         if self._first_run:
             message["kill-all-processes"] = True
         message.update(self._detect_process_changes())
-        
+
         if message:
             message["type"] = "active-process-info"
             return message