reimplement the bigmem test memory watchdog as a subprocess

[neologix]

BPO	14154
Nosy	@pitrou, @vstinner
Files	mem_watchdog_subprocess.diff mem_watchdog_1.diff mem_watchdog_2.diff

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = None
closed_at = <Date 2012-03-24.19:38:27.194>
created_at = <Date 2012-02-28.20:13:09.379>
labels = ['type-feature', 'tests']
title = 'reimplement the bigmem test memory watchdog as a subprocess'
updated_at = <Date 2012-03-24.19:38:27.193>
user = 'https://bugs.python.org/neologix'

bugs.python.org fields:

activity = <Date 2012-03-24.19:38:27.193>
actor = 'neologix'
assignee = 'none'
closed = True
closed_date = <Date 2012-03-24.19:38:27.194>
closer = 'neologix'
components = ['Tests']
creation = <Date 2012-02-28.20:13:09.379>
creator = 'neologix'
dependencies = []
files = ['24674', '24950', '24974']
hgrepos = []
issue_num = 14154
keywords = ['patch', 'needs review']
message_count = 10.0
messages = ['154570', '154572', '154577', '154601', '154605', '156363', '156365', '156444', '156468', '156686']
nosy_count = 5.0
nosy_names = ['pitrou', 'vstinner', 'nadeem.vawda', 'neologix', 'python-dev']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue14154'
versions = ['Python 3.3']

[neologix]

As suggested in http://bugs.python.org/msg154330, here's a rewrite of the test memory watchdog using subprocess instead of thread + faulthandler.

[pitrou]

It's so much simpler that I feel a bit ridiculous with my earlier solution :)

[vstinner]

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.

[neologix]

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

[vstinner]

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

[neologix]

Here's a new version, with a dedicated script for the watchdog process.

[vstinner]

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.

[neologix]

Here's a patch flushing stdout explicitely (should not be necessay
unless the watchdog crashes, but...).
Also, redirect stderr to /dev/null.

[vstinner]

mem_watchdog_2.diff looks good to me.

[python-dev]

New changeset 53a2488605e3 by Charles-François Natali in branch 'default':
Issue bpo-14154: Reimplement the bigmem test memory watchdog as a subprocess.
http://hg.python.org/cpython/rev/53a2488605e3