| msg154570 - (view) |
Author: Charles-François Natali (neologix) *  |
Date: 2012-02-28 20:13 |
| As suggested in http://bugs.python.org/msg154330, here's a rewrite of the test memory watchdog using subprocess instead of thread + faulthandler. |
|
|
| msg154572 - (view) |
Author: Antoine Pitrou (pitrou) * |
Date: 2012-02-28 21:03 |
| It's so much simpler that I feel a bit ridiculous with my earlier solution :) |
|
|
| msg154577 - (view) |
Author: STINNER Victor (vstinner) * |
Date: 2012-02-28 22:48 |
| A subprocess looks simpler (and safer?) than a C thread with a pipe. + f = open(self.procfile, 'r') 'rb' mode is enough here, no need of Unicode ;-) + self.mem_watchdog = subprocess.Popen(..., stdin=f) Can't you open the /proc/pid/stat file in the child process? It might be an issue with SELinux or Grsecurity, but I don't expect that our buildbot use such security patch. + statm = sys.stdin.read() file_watchdog() did read only 1024 bytes. I don't know if it would be better or not to use an arbitrary limit (to avoid filling the memory?). You should catch OSError here. You may only display the memory usage if it changed and start by a sleep. -- You may also write the watchdog script in .py file instead of passing it on the command line, so it will be easier to maintain it later. |
|
|
| msg154601 - (view) |
Author: Charles-François Natali (neologix) * |
Date: 2012-02-29 08:10 |
| > + f = open(self.procfile, 'r') > > 'rb' mode is enough here, no need of Unicode ;-) Why? At least to me, 'r' stands for text I/O, whereas 'rb' stands for binary I/O: here, I want to read /proc//statm, which is a textual representation of the memory usage (entries are separated by space characters). > + self.mem_watchdog = subprocess.Popen(..., stdin=f) > > Can't you open the /proc/pid/stat file in the child process? It might be an issue with SELinux or Grsecurity, but I don't expect that our buildbot use such security patch. Yes, SELinux and grsecurity are the first reason I open it in the parent. The second reason is that, if the parent crashes after the fork(), but before the child starts, I don't want it to read another process' /proc//statm (I know this would require an immediate recycling of the PID which is thus really unlikely, but hey). > + statm = sys.stdin.read() > > file_watchdog() did read only 1024 bytes. I don't know if it would be better or not to use an arbitrary limit (to avoid filling the memory?). """ $ cat /proc/self/statm 954 106 84 5 0 67 0 """ I think that should fit in memory ;-) > You should catch OSError here. Why? If we get an OSError, we can't do much except exiting anyway. > You may also write the watchdog script in .py file instead of passing it on the command line, so it will be easier to maintain it later. Yes, that would be better. |
|
|
| msg154605 - (view) |
Author: STINNER Victor (vstinner) * |
Date: 2012-02-29 09:08 |
| >> + f = open(self.procfile, 'r') >> >> 'rb' mode is enough here, no need of Unicode ;-) > > Why? The parent process doesn't read the file content, only the child. The parent only needs a file descriptor. >> + self.mem_watchdog = subprocess.Popen(..., stdin=f) >> >> Can't you open the /proc/pid/stat file in the child process? It might be an issue with SELinux or Grsecurity, but I don't expect that our buildbot use such security patch. > > (...) I don't want it to read another > process' /proc//statm (I know this would require an immediate > recycling of the PID which is thus really unlikely, but hey). Oh ok, this is a good reason. You may document such tricky justifications. >> You should catch OSError here. > > Why? If we get an OSError, we can't do much except exiting anyway. To not print a traceback if the parent dies. |
|
|
| msg156363 - (view) |
Author: Charles-François Natali (neologix) * |
Date: 2012-03-19 21:56 |
| Here's a new version, with a dedicated script for the watchdog process. |
|
|
| msg156365 - (view) |
Author: STINNER Victor (vstinner) * |
Date: 2012-03-19 22:12 |
| memory_watchdog.py should probably use sys.stdout.flush(), and you should replace print("...") by sys.stdout.write("...\n") to only call sys.stdout.write once (print calls write a second time just to write the newline). +1 for the subprocess instead of the thread. |
|
|
| msg156444 - (view) |
Author: Charles-François Natali (neologix) * |
Date: 2012-03-20 18:26 |
| Here's a patch flushing stdout explicitely (should not be necessay unless the watchdog crashes, but...). Also, redirect stderr to /dev/null. |
|
|
| msg156468 - (view) |
Author: STINNER Victor (vstinner) * |
Date: 2012-03-20 22:08 |
| mem_watchdog_2.diff looks good to me. |
|
|
| msg156686 - (view) |
Author: Roundup Robot (python-dev)  |
Date: 2012-03-24 09:06 |
| New changeset 53a2488605e3 by Charles-François Natali in branch 'default': Issue #14154: Reimplement the bigmem test memory watchdog as a subprocess. http://hg.python.org/cpython/rev/53a2488605e3 |
|
|