[issue23566] RFE: faulthandler.register() should support file descriptors

[STINNER Victor]

STINNER Victor added the comment:

> 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!

--
resolution:  -> fixed
status: open -> closed

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue23566] RFE: faulthandler.register() should support file descriptors

[Wei Wu]

Wei Wu added the comment:

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue23566] RFE: faulthandler.register() should support file descriptors

[Wei Wu]

Wei Wu added the comment:

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

--
Added file: http://bugs.python.org/file38546/issue23566_fd_tests_v2.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue23566] RFE: faulthandler.register() should support file descriptors

[STINNER Victor]

STINNER Victor added the comment:

I reviewed  issue23566_fd_tests.patch .

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue23566] RFE: faulthandler.register() should support file descriptors

[Wei Wu]

Wei Wu added the comment:

I attached a patch that implements the solution described above.

--
Added file: http://bugs.python.org/file38480/issue23566_fd_tests.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue23566] RFE: faulthandler.register() should support file descriptors

[Wei Wu]

Wei Wu added the comment:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue23566] RFE: faulthandler.register() should support file descriptors

[Christoph Sieghart]

Changes by Christoph Sieghart :


--
nosy:  -sigi

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue23566] RFE: faulthandler.register() should support file descriptors

[STINNER Victor]

STINNER Victor added the comment:

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?

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue23566] RFE: faulthandler.register() should support file descriptors

[Roundup Robot]

Roundup Robot added the comment:

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue23566] RFE: faulthandler.register() should support file descriptors

[Wei Wu]

Wei Wu added the comment:

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 tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue23566] RFE: faulthandler.register() should support file descriptors

[STINNER Victor]

STINNER Victor added the comment:

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.

--
resolution: fixed -> 
status: closed -> open

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue23566] RFE: faulthandler.register() should support file descriptors

[STINNER Victor]

STINNER Victor added the comment:

> 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!

--
resolution:  -> fixed
status: open -> closed

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue23566] RFE: faulthandler.register() should support file descriptors

[Roundup Robot]

Roundup Robot added the comment:

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

--
nosy: +python-dev

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue23566] RFE: faulthandler.register() should support file descriptors

[Wei Wu]

Wei Wu added the comment:

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.

: )

--
Added file: http://bugs.python.org/file38445/issue23566_v3.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue23566] RFE: faulthandler.register() should support file descriptors

[STINNER Victor]

STINNER Victor added the comment:

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue23566] RFE: faulthandler.register() should support file descriptors

[Wei Wu]

Wei Wu added the comment:

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

--
Added file: http://bugs.python.org/file38436/issue23566_update.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue23566] RFE: faulthandler.register() should support file descriptors

[STINNER Victor]

STINNER Victor added the comment:

@kilowu: Thanks, I reviewed your patch.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue23566] RFE: faulthandler.register() should support file descriptors

[Wei Wu]

Wei Wu added the comment:

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.

--
keywords: +patch
nosy: +kilowu
Added file: http://bugs.python.org/file38417/issue23566.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue23566] RFE: faulthandler.register() should support file descriptors

[Christoph Sieghart]

Changes by Christoph Sieghart :


--
nosy: +sigi

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue23566] RFE: faulthandler.register() should support file descriptors

[STINNER Victor]

STINNER Victor added the comment:

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue23566] RFE: faulthandler.register() should support file descriptors

[STINNER Victor]

New submission from STINNER Victor:

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

--
keywords: easy
messages: 237091
nosy: haypo
priority: normal
severity: normal
status: open
title: RFE: faulthandler.register() should support file descriptors
versions: Python 3.5

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com