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

Proposed by Kevin McDermott on 2010-02-05
Status: Merged
Approved by: Jamu Kakar on 2010-02-05
Approved revision: not available
Merged at revision: not available
Proposed branch: lp:~bigkevmcd/landscape-client/517453-process-race-condition
Merge into: lp:~landscape/landscape-client/trunk
Diff against target: 230 lines (+114/-71)
3 files modified
landscape/lib/process.py (+64/-69)
landscape/lib/tests/test_process.py (+49/-1)
landscape/monitor/activeprocessinfo.py (+1/-1)
To merge this branch: bzr merge lp:~bigkevmcd/landscape-client/517453-process-race-condition
Reviewer Review Type Date Requested Status
Thomas Herve (community) Approve on 2010-02-08
Jamu Kakar (community) 2010-02-05 Approve on 2010-02-05
Sidnei da Silva (community) 2010-02-05 Approve on 2010-02-05
Review via email: mp+18697@code.launchpad.net
To post a comment you must log in.
Sidnei da Silva (sidnei) wrote :

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

Looks good, +1.

review: Approve
202. By Kevin McDermott on 2010-02-05

Remove extraneous imports...

Jamu Kakar (jkakar) wrote :

Nice work, +1!

review: Approve
Thomas Herve (therve) wrote :

[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
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) wrote :

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
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.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'landscape/lib/process.py'
2--- landscape/lib/process.py 2008-08-13 14:39:53 +0000
3+++ landscape/lib/process.py 2010-02-08 15:30:35 +0000
4@@ -47,88 +47,83 @@
5 yield process_info
6
7 def get_process_info(self, process_id):
8+ """
9+ Parse the /proc/<pid>/cmdline and /proc/<pid>/status files for
10+ information about the running process with process_id.
11+
12+ The /proc filesystem doesn't behave like ext2, open files can disappear
13+ during the read process.
14+ """
15 cmd_line_name = ""
16 process_dir = os.path.join(self._proc_dir, str(process_id))
17 process_info = {"pid": process_id}
18
19 try:
20 file = open(os.path.join(process_dir, "cmdline"), "r")
21- except IOError:
22- # Handle the race that happens when we find a process
23- # which terminates before we open the stat file.
24- return None
25-
26- try:
27- # cmdline is a \0 separated list of strings
28- # We take the first, and then strip off the path, leaving us with
29- # the basename.
30- cmd_line = file.readline()
31- cmd_line_name = os.path.basename(cmd_line.split("\0")[0])
32- finally:
33- file.close()
34-
35- try:
36+ try:
37+ # cmdline is a \0 separated list of strings
38+ # We take the first, and then strip off the path, leaving us with
39+ # the basename.
40+ cmd_line = file.readline()
41+ cmd_line_name = os.path.basename(cmd_line.split("\0")[0])
42+ finally:
43+ file.close()
44+
45 file = open(os.path.join(process_dir, "status"), "r")
46- except IOError:
47- # Handle the race that happens when we find a process
48- # which terminates before we open the status file.
49- return None
50-
51- try:
52- for line in file:
53- parts = line.split(":", 1)
54- if parts[0] == "Name":
55- process_info["name"] = (cmd_line_name.strip() or
56- parts[1].strip())
57- elif parts[0] == "State":
58- state = parts[1].strip()
59- process_info["state"] = STATES[state]
60- elif parts[0] == "Uid":
61- value_parts = parts[1].split()
62- process_info["uid"] = int(value_parts[0])
63- elif parts[0] == "Gid":
64- value_parts = parts[1].split()
65- process_info["gid"] = int(value_parts[0])
66- elif parts[0] == "VmSize":
67- value_parts = parts[1].split()
68- process_info["vm-size"] = int(value_parts[0])
69- break
70- finally:
71- file.close()
72-
73- try:
74+ try:
75+ for line in file:
76+ parts = line.split(":", 1)
77+ if parts[0] == "Name":
78+ process_info["name"] = (cmd_line_name.strip() or
79+ parts[1].strip())
80+ elif parts[0] == "State":
81+ state = parts[1].strip()
82+ process_info["state"] = STATES[state]
83+ elif parts[0] == "Uid":
84+ value_parts = parts[1].split()
85+ process_info["uid"] = int(value_parts[0])
86+ elif parts[0] == "Gid":
87+ value_parts = parts[1].split()
88+ process_info["gid"] = int(value_parts[0])
89+ elif parts[0] == "VmSize":
90+ value_parts = parts[1].split()
91+ process_info["vm-size"] = int(value_parts[0])
92+ break
93+ finally:
94+ file.close()
95+
96 file = open(os.path.join(process_dir, "stat"), "r")
97+ try:
98+ # These variable names are lifted directly from proc(5)
99+ # utime: The number of jiffies that this process has been scheduled in
100+ # user mode.
101+ # stime: The number of jiffies that this process has been scheduled in
102+ # kernel mode.
103+ # cutime: The number of jiffies that this process's waited-for children
104+ # have been scheduled in user mode.
105+ # cstime: The number of jiffies that this process's waited-for children
106+ # have been scheduled in kernel mode.
107+ parts = file.read().split()
108+ start_time = int(parts[21])
109+ utime = int(parts[13])
110+ stime = int(parts[14])
111+ uptime = self._uptime or get_uptime()
112+ pcpu = calculate_pcpu(utime, stime, uptime,
113+ start_time, self._jiffies_per_sec)
114+ process_info["percent-cpu"] = pcpu
115+ delta = timedelta(0, start_time // self._jiffies_per_sec)
116+ if self._boot_time is None:
117+ logging.warning("Skipping process (PID %s) without boot time.")
118+ return None
119+ process_info["start-time"] = to_timestamp(self._boot_time + delta)
120+ finally:
121+ file.close()
122+
123 except IOError:
124 # Handle the race that happens when we find a process
125 # which terminates before we open the stat file.
126 return None
127
128- # These variable names are lifted directly from proc(5)
129- # utime: The number of jiffies that this process has been scheduled in
130- # user mode.
131- # stime: The number of jiffies that this process has been scheduled in
132- # kernel mode.
133- # cutime: The number of jiffies that this process's waited-for children
134- # have been scheduled in user mode.
135- # cstime: The number of jiffies that this process's waited-for children
136- # have been scheduled in kernel mode.
137- try:
138- parts = file.read().split()
139- start_time = int(parts[21])
140- utime = int(parts[13])
141- stime = int(parts[14])
142- uptime = self._uptime or get_uptime()
143- pcpu = calculate_pcpu(utime, stime, uptime,
144- start_time, self._jiffies_per_sec)
145- process_info["percent-cpu"] = pcpu
146- delta = timedelta(0, start_time // self._jiffies_per_sec)
147- if self._boot_time is None:
148- logging.warning("Skipping process (PID %s) without boot time.")
149- return None
150- process_info["start-time"] = to_timestamp(self._boot_time + delta)
151- finally:
152- file.close()
153-
154 assert("pid" in process_info and "state" in process_info
155 and "name" in process_info and "uid" in process_info
156 and "gid" in process_info and "start-time" in process_info)
157
158=== modified file 'landscape/lib/tests/test_process.py'
159--- landscape/lib/tests/test_process.py 2008-06-25 08:01:39 +0000
160+++ landscape/lib/tests/test_process.py 2010-02-08 15:30:35 +0000
161@@ -2,7 +2,55 @@
162
163 from landscape.tests.helpers import LandscapeTest
164
165-from landscape.lib.process import calculate_pcpu
166+from landscape.lib.process import calculate_pcpu, ProcessInformation
167+
168+
169+class ProcessInfoTest(LandscapeTest):
170+
171+ def test_missing_process_race(self):
172+ """
173+ We use os.listdir("/proc") to get the list of active processes, if a
174+ process ends before we attempt to read the process' information, then
175+ this should not trigger an error.
176+ """
177+ listdir_mock = self.mocker.replace("os.listdir")
178+ listdir_mock("/proc")
179+ self.mocker.result(["12345"])
180+
181+ class FakeFile(object):
182+
183+ def __init__(self, response=""):
184+ self._response = response
185+ self.closed = False
186+
187+ def readline(self):
188+ return self._response
189+
190+ def __iter__(self):
191+ if self._response is None:
192+ raise IOError("Fake file error")
193+ else:
194+ yield self._response
195+
196+ def close(self):
197+ self.closed = True
198+
199+ open_mock = self.mocker.replace("__builtin__.open")
200+ open_mock("/proc/12345/cmdline", "r")
201+ fakefile1 = FakeFile("test-binary")
202+ self.mocker.result(fakefile1)
203+
204+ open_mock("/proc/12345/status", "r")
205+ fakefile2 = FakeFile(None)
206+ self.mocker.result(fakefile2)
207+
208+ self.mocker.replay()
209+
210+ process_info = ProcessInformation("/proc")
211+ processes = list(process_info.get_all_process_info())
212+ self.assertEquals(processes, [])
213+ self.assertTrue(fakefile1.closed)
214+ self.assertTrue(fakefile2.closed)
215
216
217 class CalculatePCPUTest(unittest.TestCase):
218
219=== modified file 'landscape/monitor/activeprocessinfo.py'
220--- landscape/monitor/activeprocessinfo.py 2009-10-06 16:27:23 +0000
221+++ landscape/monitor/activeprocessinfo.py 2010-02-08 15:30:35 +0000
222@@ -41,7 +41,7 @@
223 if self._first_run:
224 message["kill-all-processes"] = True
225 message.update(self._detect_process_changes())
226-
227+
228 if message:
229 message["type"] = "active-process-info"
230 return message

Subscribers

People subscribed via source and target branches

to all changes: