Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RFE: faulthandler.register() should support file descriptors #67754

Closed
vstinner opened this issue Mar 3, 2015 · 19 comments
Closed

RFE: faulthandler.register() should support file descriptors #67754

vstinner opened this issue Mar 3, 2015 · 19 comments
Labels

Comments

@vstinner
Copy link
Member

vstinner commented Mar 3, 2015

BPO 23566
Nosy @vstinner, @kilowu
Files
  • issue23566.patch
  • issue23566_update.patch
  • issue23566_v3.patch
  • issue23566_fd_tests.patch
  • issue23566_fd_tests_v2.patch
  • 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 2015-04-06.21:34:37.683>
    created_at = <Date 2015-03-03.00:00:15.069>
    labels = ['easy']
    title = 'RFE: faulthandler.register() should support file descriptors'
    updated_at = <Date 2015-04-06.21:34:37.682>
    user = 'https://github.com/vstinner'

    bugs.python.org fields:

    activity = <Date 2015-04-06.21:34:37.682>
    actor = 'vstinner'
    assignee = 'none'
    closed = True
    closed_date = <Date 2015-04-06.21:34:37.683>
    closer = 'vstinner'
    components = []
    creation = <Date 2015-03-03.00:00:15.069>
    creator = 'vstinner'
    dependencies = []
    files = ['38417', '38436', '38445', '38480', '38546']
    hgrepos = []
    issue_num = 23566
    keywords = ['patch', 'easy']
    message_count = 19.0
    messages = ['237091', '237092', '237730', '237749', '237843', '237852', '237879', '237939', '237940', '237941', '238006', '238018', '238019', '238044', '238073', '238189', '238460', '239870', '240187']
    nosy_count = 3.0
    nosy_names = ['vstinner', 'python-dev', 'kilowu']
    pr_nums = []
    priority = 'normal'
    resolution = 'fixed'
    stage = None
    status = 'closed'
    superseder = None
    type = None
    url = 'https://bugs.python.org/issue23566'
    versions = ['Python 3.5']

    @vstinner
    Copy link
    Member Author

    vstinner commented Mar 3, 2015

    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

    @vstinner vstinner added the easy label Mar 3, 2015
    @vstinner
    Copy link
    Member Author

    vstinner commented Mar 3, 2015

    Only faulthandler_get_fileno() should be modified, but a new unit test should be added.

    @kilowu
    Copy link
    Mannequin

    kilowu mannequin commented Mar 10, 2015

    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.

    @vstinner
    Copy link
    Member Author

    vstinner commented Mar 10, 2015

    @kilowu: Thanks, I reviewed your patch.

    @kilowu
    Copy link
    Mannequin

    kilowu mannequin commented Mar 11, 2015

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

    @vstinner
    Copy link
    Member Author

    vstinner commented Mar 11, 2015

    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 ;-)

    @kilowu
    Copy link
    Mannequin

    kilowu mannequin commented Mar 11, 2015

    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.

    : )

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Mar 12, 2015

    New changeset e78de5b1e3ef by Victor Stinner in branch 'default':
    Issue bpo-23566: enable(), register(), dump_traceback() and dump_traceback_later()
    https://hg.python.org/cpython/rev/e78de5b1e3ef

    @vstinner
    Copy link
    Member Author

    vstinner commented Mar 12, 2015

    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!

    @vstinner
    Copy link
    Member Author

    vstinner commented Mar 12, 2015

    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.

    @vstinner vstinner reopened this Mar 12, 2015
    @kilowu
    Copy link
    Mannequin

    kilowu mannequin commented Mar 13, 2015

    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.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Mar 13, 2015

    New changeset 211e29335e72 by Victor Stinner in branch 'default':
    Issue bpo-23566: Skip "fd" tests of test_faulthandler on Windows
    https://hg.python.org/cpython/rev/211e29335e72

    @vstinner
    Copy link
    Member Author

    vstinner commented Mar 13, 2015

    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?

    @kilowu
    Copy link
    Mannequin

    kilowu mannequin commented Mar 13, 2015

    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.

    @kilowu
    Copy link
    Mannequin

    kilowu mannequin commented Mar 14, 2015

    I attached a patch that implements the solution described above.

    @vstinner
    Copy link
    Member Author

    vstinner commented Mar 16, 2015

    I reviewed issue23566_fd_tests.patch .

    @kilowu
    Copy link
    Mannequin

    kilowu mannequin commented Mar 18, 2015

    The updated patch refactored test code a little bit according to the latest review comments by Victor.

    @kilowu
    Copy link
    Mannequin

    kilowu mannequin commented Apr 2, 2015

    @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?

    @vstinner
    Copy link
    Member Author

    vstinner commented Apr 6, 2015

    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!

    @vstinner vstinner closed this as completed Apr 6, 2015
    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    Projects
    None yet
    Development

    No branches or pull requests

    1 participant