classification
Title: Factor out the _SuppressCoreFiles context manager
Type: enhancement Stage: resolved
Components: Tests Versions: Python 3.4
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: ezio.melotti, haypo, lambertv, ncoghlan, neologix, pitrou, python-dev
Priority: normal Keywords: easy, patch

Created on 2013-08-01 22:12 by pitrou, last changed 2013-09-06 19:18 by pitrou. This issue is now closed.

Files
File name Uploaded Description Edit
issue-18623.patch lambertv, 2013-08-22 23:57
issue-18623_v2.patch lambertv, 2013-08-23 16:23 review
issue-18623_v3.patch lambertv, 2013-08-26 23:33 review
issue-18623_v4.patch lambertv, 2013-08-31 15:50 review
Messages (16)
msg194125 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2013-08-01 22:12
_SuppressCoreFiles in test_subprocess is a useful facility for other tests, for instance test_faulthandler (which has its own logic) or test_capi (which actually happily dumps core in one of its subtests).
It could probably be factored out into test.support.
msg194128 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2013-08-01 22:34
Ah, and the recursion limit tests in test_sys dump core too...
msg195934 - (view) Author: Valerie Lambert (lambertv) * Date: 2013-08-22 23:57
I have included a patch that moves _SuppressCoreFiles into test.support and a test that checks whether resource.RLIMIT_CORE is being properly set and reset. It would be nice if there was a more explicit way to test the creation of core files, however.
The patch also corrects a comment that stated ulimit was being set to (0, 0) when in the code only the soft limit was being set to 0. I'm pretty sure that the later should be done because then we don't run into trouble when we have to restore the hard limit to its original (higher) limit.
msg195942 - (view) Author: Charles-François Natali (neologix) * (Python committer) Date: 2013-08-23 05:05
Could you post your patch without the '--git' option?
Apparently it doesn't work well with the code review tool.

> It would be nice if there was a more explicit way to test the
> creation of core files, however.

One possibility would be to fork() a child process, and make it call os.abort(). In the parent, call os.waitpid(), and call os.WCOREDUMP() on the status.
msg195953 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2013-08-23 08:58
Will also be useful for issue #18808 :)
msg195984 - (view) Author: Valerie Lambert (lambertv) * Date: 2013-08-23 16:23
Of course, here is the patch without the '--git' option.
msg196253 - (view) Author: Valerie Lambert (lambertv) * Date: 2013-08-26 23:33
I've added a new test that uses fork() and os.abort(), then asserts os.WCOREDUMP() is false. 
However, this test is currently failing. Is my test incorrect, or is this an issue with SuppressCoreFiles() itself?

If its a problem with the test I'm guessing it might have to do with how os.WCOREDUMP() decides whether a process has dumped its core or not. Is there another way we could go about this test?
msg196268 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2013-08-27 08:27
> If its a problem with the test I'm guessing it might have to do with how 
> os.WCOREDUMP() decides whether a process has dumped its core or not.

You are right, the status code doesn't seem affected by whether the core file was actually dumped or not:

$ ulimit -c
0
$ python -c "import os; os.abort()"; echo $?
Abandon
134
$ ulimit -c unlimited
$ python -c "import os; os.abort()"; echo $?
Abandon (core dumped)
134

And of course:

>>> os.WCOREDUMP(134)
True


I don't think there's any reliable way to test this: modern Linux kernels can intercept core file generation and run an executable instead (think Ubuntu's apport), so the only thing remaining to do is to just check that the context manager "works", i.e. doesn't raise anything.

(see http://linux.die.net/man/5/core "Piping core dumps to a program")
msg196272 - (view) Author: Charles-François Natali (neologix) * (Python committer) Date: 2013-08-27 10:04
The test works for me. The only problem is that faulthandler dumps a
stack: it might be better to use subprocess with stderr=devnull).

As for the status code, it looks like a bash bug:
"""
$ cat /tmp/test_core.py
import os

if os.fork() == 0:
    os.abort()
else:
    pid, status = os.waitpid(-1, 0)
    print("%d: %s" % (status, os.WCOREDUMP(status)))
$ python /tmp/test_core.py
134: True
$ ulimit -c 0
$ python /tmp/test_core.py
6: False
$ python -c "print(0x80 | 6)"
134
"""

And it's funny, because bash does detect the coredump generation, compare:
"""
$ python -c "import os; os.abort()"; echo $?
Abandon
134
"""

to

"""
$ python -c "import os; os.abort()"; echo $?
Abandon (core dumped)
134
"""

I had a quick look at the code, and indeed there's a bug: there's a
function rebuilding the exit status from the exit code:

"""
static int
process_exit_status (status)
     WAIT status;
{
  if (WIFSIGNALED (status))
    return (128 + WTERMSIG (status));
  else if (WIFSTOPPED (status) == 0)
    return (WEXITSTATUS (status));
  else
    return (EXECUTION_SUCCESS);
}
"""

Unfortunately, adding 128 sets the coredump flag every time,
regardless of the actual coredump status.
Never thought I'd encounter such a bug in bash :-)
msg196310 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2013-08-27 18:44
> As for the status code, it looks like a bash bug:

Your script gives different results here (on Ubuntu):

$ ulimit -c
0
$ ./python dumpcore.py 
6: False
$ ulimit -c unlimited
$ ./python dumpcore.py 
6: False

That's because Ubuntu overrides the core file pattern:

$ cat /proc/sys/kernel/core_pattern 
|/usr/share/apport/apport %p %s %c

If I ask for a regular core file, the script works:

$ sudo sh -c "echo core.%p > /proc/sys/kernel/core_pattern"
$ ./python dumpcore.py 
134: True

Which means the test really threatens to be unreliable (other systems may install similar mechanisms by default).
msg196642 - (view) Author: Valerie Lambert (lambertv) * Date: 2013-08-31 15:50
Running through both your scripts on my machine (using Ubuntu) WCOREDUMP is always True (though I'll only be able to find a core file when ulimit is set to unlimited, as expected).

Because there doesn't seem to be a good way to test this, I've cut the test from the patch. Is there anything else that this patch should address?
msg197086 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2013-09-06 18:42
> Is there anything else that this patch should address?

I hoped Charles-François would answer this one, but no, I don't think there's anything left :)
Next step (for another issue perhaps) is to actually re-use this context manager in the other tests which deliberately crash the interpreter.
msg197087 - (view) Author: Roundup Robot (python-dev) Date: 2013-09-06 18:50
New changeset 28a1843c0fc1 by Antoine Pitrou in branch 'default':
Issue #18623: Factor out the _SuppressCoreFiles context manager into test.support.
http://hg.python.org/cpython/rev/28a1843c0fc1
msg197089 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2013-09-06 18:51
Patch committed, thanks!
msg197093 - (view) Author: Charles-François Natali (neologix) * (Python committer) Date: 2013-09-06 19:06
>> Is there anything else that this patch should address?
>
> I hoped Charles-François would answer this one, but no, I don't think there's anything left :)

Sorry, I had completely forgotten this issue.

The only comment I have is this:

class SuppressCoreFiles(object):

The explicit 'object' superclass is old-school ;-)
msg197098 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2013-09-06 19:18
> The explicit 'object' superclass is old-school ;-)

Ha. Ok, I've fixed it.
History
Date User Action Args
2013-09-06 19:18:45pitrousetmessages: + msg197098
2013-09-06 19:06:54neologixsetmessages: + msg197093
2013-09-06 18:51:01pitrousetstatus: open -> closed
resolution: fixed
messages: + msg197089

stage: commit review -> resolved
2013-09-06 18:50:12python-devsetnosy: + python-dev
messages: + msg197087
2013-09-06 18:42:40pitrousetmessages: + msg197086
stage: needs patch -> commit review
2013-08-31 15:50:33lambertvsetfiles: + issue-18623_v4.patch

messages: + msg196642
2013-08-27 18:44:12pitrousetmessages: + msg196310
2013-08-27 10:04:40neologixsetmessages: + msg196272
2013-08-27 08:27:36pitrousetmessages: + msg196268
2013-08-26 23:33:07lambertvsetfiles: + issue-18623_v3.patch

messages: + msg196253
2013-08-23 16:23:34lambertvsetfiles: + issue-18623_v2.patch

messages: + msg195984
2013-08-23 08:58:05pitrousetmessages: + msg195953
2013-08-23 05:05:30neologixsetnosy: + neologix
messages: + msg195942
2013-08-22 23:57:26lambertvsetfiles: + issue-18623.patch

nosy: + lambertv
messages: + msg195934

keywords: + patch
2013-08-01 22:34:14pitrousetmessages: + msg194128
2013-08-01 22:15:16hayposetnosy: + haypo
2013-08-01 22:12:27pitroucreate