classification
Title: reimplement the bigmem test memory watchdog as a subprocess
Type: enhancement Stage: resolved
Components: Tests Versions: Python 3.3
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: nadeem.vawda, neologix, pitrou, python-dev, vstinner
Priority: normal Keywords: needs review, patch

Created on 2012-02-28 20:13 by neologix, last changed 2012-03-24 19:38 by neologix. This issue is now closed.

Files
File name Uploaded Description Edit
mem_watchdog_subprocess.diff neologix, 2012-02-28 20:13 review
mem_watchdog_1.diff neologix, 2012-03-19 21:56 review
mem_watchdog_2.diff neologix, 2012-03-20 18:26 review
Messages (10)
msg154570 - (view) Author: Charles-François Natali (neologix) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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/<PID>/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/<PID>/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) * (Python committer) 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/<PID>/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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) Date: 2012-03-20 22:08
mem_watchdog_2.diff looks good to me.
msg156686 - (view) Author: Roundup Robot (python-dev) (Python triager) 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
History
Date User Action Args
2012-03-24 19:38:27neologixsetstatus: open -> closed
resolution: fixed
stage: patch review -> resolved
2012-03-24 09:06:49python-devsetnosy: + python-dev
messages: + msg156686
2012-03-20 22:08:30vstinnersetmessages: + msg156468
2012-03-20 18:26:41neologixsetfiles: + mem_watchdog_2.diff

messages: + msg156444
2012-03-19 22:12:07vstinnersetmessages: + msg156365
2012-03-19 21:56:11neologixsetfiles: + mem_watchdog_1.diff

messages: + msg156363
2012-02-29 09:08:35vstinnersetmessages: + msg154605
2012-02-29 08:10:45neologixsetmessages: + msg154601
2012-02-28 22:48:47vstinnersetmessages: + msg154577
2012-02-28 21:03:07pitrousetnosy: + vstinner
messages: + msg154572
2012-02-28 20:22:45nadeem.vawdasetnosy: + nadeem.vawda
2012-02-28 20:13:09neologixcreate