classification
Title: RFE: faulthandler.register() should support file descriptors
Type: Stage:
Components: Versions: Python 3.5
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: haypo, kilowu, python-dev
Priority: normal Keywords: easy, patch

Created on 2015-03-03 00:00 by haypo, last changed 2015-04-06 21:34 by haypo. This issue is now closed.

Files
File name Uploaded Description Edit
issue23566.patch kilowu, 2015-03-10 06:01 review
issue23566_update.patch kilowu, 2015-03-11 04:19 review
issue23566_v3.patch kilowu, 2015-03-11 15:52 review
issue23566_fd_tests.patch kilowu, 2015-03-14 06:40 review
issue23566_fd_tests_v2.patch kilowu, 2015-03-18 17:06 review
Messages (19)
msg237091 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2015-03-03 00:00
Currently, functions of the faulthandler module require a file-like object (with a fileno() method). It would be nice to accept also file descriptors (int).

Example of code using faulthandler which creates an useless file object just to pass a file descriptor:
https://github.com/nicoddemus/pytest-faulthandler/blob/master/pytest_faulthandler.py#L13
msg237092 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2015-03-03 00:01
Only faulthandler_get_fileno() should be modified, but a new unit test should be added.
msg237730 - (view) Author: Wei Wu (kilowu) * Date: 2015-03-10 06:01
Attached a patch to this issue. Made some changes to faulthandler_get_fileno to accept integer as fd. If a fd is provided, the file member of fatal_error struct is set NULL.

An new test case is added and some common testing code is also changed in order to be reused.
msg237749 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2015-03-10 11:26
@kilowu: Thanks, I reviewed your patch.
msg237843 - (view) Author: Wei Wu (kilowu) * Date: 2015-03-11 04:19
@haypo: Thank you for your review. I attached an updated patch addressing the review comments. In addition, I also added a change note in Misc/NEWS.
msg237852 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2015-03-11 08:48
issue23566_update.patch looks good to me, but I suggested some minor changes. Usually, I do such changes myself, but I proposed this issue on the Python menthorship list, so I prefer to do the changes to learn the process of reviewing patches and taking comments in account ;-)
msg237879 - (view) Author: Wei Wu (kilowu) * Date: 2015-03-11 15:52
Updated the patch and addressed the previous review comments:

 * Fixed the hasattr problem
 * Added a default value None for filename in check_dump_traceback
 * Reverted unnecessary code change in check_dump_traceback_later
 * Added a new paragraph to the enable, dump_traceback_later and
register in doc

Let me know if there is possible further improvement in this patch.

: )
msg237939 - (view) Author: Roundup Robot (python-dev) Date: 2015-03-12 14:33
New changeset e78de5b1e3ef by Victor Stinner in branch 'default':
Issue #23566: enable(), register(), dump_traceback() and dump_traceback_later()
https://hg.python.org/cpython/rev/e78de5b1e3ef
msg237940 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2015-03-12 14:36
> Let me know if there is possible further improvement in this patch.

I was going to push your change, but I noticed that you use stderr.fileno() in tests. I modified the test to use a different file descriptor. sys.stderr is also the default value, so the test may pass because the file descriptor was ignored (if there was a bug in your change, which is not the case).

I pushed the modified patch.

Thanks for your contribution Wei Wu!
msg237941 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2015-03-12 14:55
Oops, I forgot Windows. On Windows, we should maybe use stderr.fileno() and avoid pass_fds.

(Or maybe just skip the tests on Windows?)

http://buildbot.python.org/all/builders/AMD64%20Windows7%20SP1%203.x/builds/5828/steps/test/logs/stdio

======================================================================
FAIL: test_dump_traceback_fd (test.test_faulthandler.FaultHandlerTests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "C:\buildbot.python.org\3.x.kloth-win64\build\lib\test\test_faulthandler.py", line 378, in test_dump_traceback_fd
    self.check_dump_traceback(fd=fp.fileno())
  File "C:\buildbot.python.org\3.x.kloth-win64\build\lib\test\test_faulthandler.py", line 365, in check_dump_traceback
    trace, exitcode = self.get_output(code, filename, fd)
  File "C:\buildbot.python.org\3.x.kloth-win64\build\lib\test\test_faulthandler.py", line 60, in get_output
    process = script_helper.spawn_python('-c', code, pass_fds=pass_fds)
  File "C:\buildbot.python.org\3.x.kloth-win64\build\lib\test\script_helper.py", line 136, in spawn_python
    **kw)
  File "C:\buildbot.python.org\3.x.kloth-win64\build\lib\subprocess.py", line 855, in __init__
    restore_signals, start_new_session)
  File "C:\buildbot.python.org\3.x.kloth-win64\build\lib\subprocess.py", line 1096, in _execute_child
    assert not pass_fds, "pass_fds not supported on Windows."
AssertionError: pass_fds not supported on Windows.

======================================================================
FAIL: test_enable_fd (test.test_faulthandler.FaultHandlerTests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "C:\buildbot.python.org\3.x.kloth-win64\build\lib\test\test_faulthandler.py", line 235, in test_enable_fd
    fd=fd)
  File "C:\buildbot.python.org\3.x.kloth-win64\build\lib\test\test_faulthandler.py", line 106, in check_fatal_error
    output, exitcode = self.get_output(code, filename=filename, fd=fd)
  File "C:\buildbot.python.org\3.x.kloth-win64\build\lib\test\test_faulthandler.py", line 60, in get_output
    process = script_helper.spawn_python('-c', code, pass_fds=pass_fds)
  File "C:\buildbot.python.org\3.x.kloth-win64\build\lib\test\script_helper.py", line 136, in spawn_python
    **kw)
  File "C:\buildbot.python.org\3.x.kloth-win64\build\lib\subprocess.py", line 855, in __init__
    restore_signals, start_new_session)
  File "C:\buildbot.python.org\3.x.kloth-win64\build\lib\subprocess.py", line 1096, in _execute_child
    assert not pass_fds, "pass_fds not supported on Windows."
AssertionError: pass_fds not supported on Windows.
msg238006 - (view) Author: Wei Wu (kilowu) * Date: 2015-03-13 03:39
Or we could reuse the file created by filename in subprocess?

if filename:
    file = open(filename, "wb")
    if use_fd:
        file = file.fileno()
else:
    file = None

In this case, we need to pass two arguments(both filename and a bool use_fd) to check_xxx functions.
msg238018 - (view) Author: Roundup Robot (python-dev) Date: 2015-03-13 10:14
New changeset 211e29335e72 by Victor Stinner in branch 'default':
Issue #23566: Skip "fd" tests of test_faulthandler on Windows
https://hg.python.org/cpython/rev/211e29335e72
msg238019 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2015-03-13 10:17
I commited a fix to repair Windows buildbots.

> Or we could reuse the file created by filename in subprocess?

I tried to pass a file descriptor from the parent to the child
process, but this option is complex. It's possible to pass a handle
with close_fds=False, but then I have to recreate a file descriptor
with a function from msvcrt.open_fshandle(). This solution looks
complex for a simple unit test.

Your solution looks simple. Would you like to work on a patch?
msg238044 - (view) Author: Wei Wu (kilowu) * Date: 2015-03-13 18:06
The last approach I proposed requires some change in "template code" of check_xxx methods. To make it better, we can add a bool parameter to the check_xxx functions, True value indicating a fd test. If a filename is given at the same time, then a fd can get from that file. Otherwise the fd should be sys.stderr.fileno().

e.g.
file = None
fp = None
if filename:
    fp = open(filename, "wb")
    # Must use a different name to prevent the file from closing...
    file = fp
if fd:
    if fp is not None:
        file = fp.fileno()
    else:
        file = sys.stderr.fileno()

# file can be file-object, fd or None
(use_the_file_to_function...)


The fd-passing approach can co-exist with this one. However it will make "template code" more complex. So I suggest just use one approach to write these fd tests.

I will work on a patch(based on tip) at this weekend.
msg238073 - (view) Author: Wei Wu (kilowu) * Date: 2015-03-14 06:40
I attached a patch that implements the solution described above.
msg238189 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2015-03-16 10:30
I reviewed  issue23566_fd_tests.patch .
msg238460 - (view) Author: Wei Wu (kilowu) * Date: 2015-03-18 17:06
The updated patch refactored test code a little bit according to the latest review comments by Victor.
msg239870 - (view) Author: Wei Wu (kilowu) * Date: 2015-04-02 02:10
@haypo, would you review issue23566_fd_tests_v2.patch? It's been a time since the last update of it.

However I think the fd tests on windows is just fine to be skipped. So what's the next plan? Shall we continue to work on it or just resolve this issue?
msg240187 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2015-04-06 21:34
> However I think the fd tests on windows is just fine to be skipped. So what's the next plan? Shall we continue to work on it or just resolve this issue?

issue23566_fd_tests_v2.patch makes test_faulthandler.py a little more complex. It's maybe better to just skip tests on Windows, the code is already well tested on other platforms, and faulthandler.c doesn't contain code specific to Windows when handling file descriptors.

I close the issue, thanks for your patches Wei Wu!
History
Date User Action Args
2015-04-06 21:34:37hayposetstatus: open -> closed
resolution: fixed
messages: + msg240187
2015-04-02 02:10:05kilowusetmessages: + msg239870
2015-03-18 17:06:42kilowusetfiles: + issue23566_fd_tests_v2.patch

messages: + msg238460
2015-03-16 10:30:16hayposetmessages: + msg238189
2015-03-14 06:40:20kilowusetfiles: + issue23566_fd_tests.patch

messages: + msg238073
2015-03-13 18:06:05kilowusetmessages: + msg238044
2015-03-13 10:26:15sigisetnosy: - sigi
2015-03-13 10:17:45hayposetmessages: + msg238019
2015-03-13 10:14:32python-devsetmessages: + msg238018
2015-03-13 03:39:53kilowusetmessages: + msg238006
2015-03-12 14:55:47hayposetstatus: closed -> open
resolution: fixed -> (no value)
messages: + msg237941
2015-03-12 14:36:01hayposetstatus: open -> closed
resolution: fixed
messages: + msg237940
2015-03-12 14:33:21python-devsetnosy: + python-dev
messages: + msg237939
2015-03-11 15:52:32kilowusetfiles: + issue23566_v3.patch

messages: + msg237879
2015-03-11 08:48:10hayposetmessages: + msg237852
2015-03-11 04:19:48kilowusetfiles: + issue23566_update.patch

messages: + msg237843
2015-03-10 11:26:40hayposetmessages: + msg237749
2015-03-10 06:01:13kilowusetfiles: + issue23566.patch

nosy: + kilowu
messages: + msg237730

keywords: + patch
2015-03-04 08:01:37sigisetnosy: + sigi
2015-03-03 00:01:14hayposetmessages: + msg237092
2015-03-03 00:00:15haypocreate