Issue #23566: enable(), register(), dump_traceback() and dump_traceback_later()

functions of faulthandler now accept file descriptors. Patch by Wei Wu.

5 files changed, 172 insertions, 78 deletions

diff --git a/Doc/library/faulthandler.rst b/Doc/library/faulthandler.rst
index eb2016a..3a5badd 100644
--- a/Doc/library/faulthandler.rst
+++ b/Doc/library/faulthandler.rst
@@ -47,6 +47,9 @@ Dumping the traceback
    Dump the tracebacks of all threads into *file*. If *all_threads* is
    ``False``, dump only the current thread.
 
+   .. versionchanged:: 3.5
+      Added support for passing file descriptor to this function.
+
 
 Fault handler state
 -------------------
@@ -59,6 +62,12 @@ Fault handler state
    produce tracebacks for every running thread. Otherwise, dump only the current
    thread.
 
+   The *file* must be kept open until the fault handler is disabled: see
+   :ref:`issue with file descriptors <faulthandler-fd>`.
+
+   .. versionchanged:: 3.5
+      Added support for passing file descriptor to this function.
+
 .. function:: disable()
 
    Disable the fault handler: uninstall the signal handlers installed by
@@ -82,9 +91,16 @@ Dumping the tracebacks after a timeout
    call replaces previous parameters and resets the timeout. The timer has a
    sub-second resolution.
 
+   The *file* must be kept open until the traceback is dumped or
+   :func:`cancel_dump_traceback_later` is called: see :ref:`issue with file
+   descriptors <faulthandler-fd>`.
+
    This function is implemented using a watchdog thread and therefore is not
    available if Python is compiled with threads disabled.
 
+   .. versionchanged:: 3.5
+      Added support for passing file descriptor to this function.
+
 .. function:: cancel_dump_traceback_later()
 
    Cancel the last call to :func:`dump_traceback_later`.
@@ -99,8 +115,14 @@ Dumping the traceback on a user signal
    the traceback of all threads, or of the current thread if *all_threads* is
    ``False``, into *file*. Call the previous handler if chain is ``True``.
 
+   The *file* must be kept open until the signal is unregistered by
+   :func:`unregister`: see :ref:`issue with file descriptors <faulthandler-fd>`.
+
    Not available on Windows.
 
+   .. versionchanged:: 3.5
+      Added support for passing file descriptor to this function.
+
 .. function:: unregister(signum)
 
    Unregister a user signal: uninstall the handler of the *signum* signal
@@ -110,6 +132,8 @@ Dumping the traceback on a user signal
    Not available on Windows.
 
 
+.. _faulthandler-fd:
+
 Issue with file descriptors
 ---------------------------
 
diff --git a/Doc/whatsnew/3.5.rst b/Doc/whatsnew/3.5.rst
index 33e6011..17be2a2 100644
--- a/Doc/whatsnew/3.5.rst
+++ b/Doc/whatsnew/3.5.rst
@@ -417,6 +417,14 @@ xmlrpc
 * :class:`xmlrpc.client.ServerProxy` is now a :term:`context manager`.
   (Contributed by Claudiu Popa in :issue:`20627`.)
 
+faulthandler
+------------
+
+* :func:`~faulthandler.enable`, :func:`~faulthandler.register`,
+  :func:`~faulthandler.dump_traceback` and
+  :func:`~faulthandler.dump_traceback_later` functions now accept file
+  descriptors.  (Contributed by Wei Wu in :issue:`23566`.)
+
 
 Optimizations
 =============
diff --git a/Lib/test/test_faulthandler.py b/Lib/test/test_faulthandler.py
index 35129b6..bbefae7 100644
--- a/Lib/test/test_faulthandler.py
+++ b/Lib/test/test_faulthandler.py
@@ -42,7 +42,7 @@ def temporary_filename():
         support.unlink(filename)
 
 class FaultHandlerTests(unittest.TestCase):
-    def get_output(self, code, filename=None):
+    def get_output(self, code, filename=None, fd=None):
         """
         Run the specified code in Python (in a new child process) and read the
         output from the standard error or from a file (if filename is set).
@@ -53,8 +53,11 @@ class FaultHandlerTests(unittest.TestCase):
         thread XXX".
         """
         code = dedent(code).strip()
+        pass_fds = []
+        if fd is not None:
+            pass_fds.append(fd)
         with support.SuppressCrashReport():
-            process = script_helper.spawn_python('-c', code)
+            process = script_helper.spawn_python('-c', code, pass_fds=pass_fds)
         stdout, stderr = process.communicate()
         exitcode = process.wait()
         output = support.strip_python_stderr(stdout)
@@ -64,13 +67,20 @@ class FaultHandlerTests(unittest.TestCase):
             with open(filename, "rb") as fp:
                 output = fp.read()
             output = output.decode('ascii', 'backslashreplace')
+        elif fd is not None:
+            self.assertEqual(output, '')
+            os.lseek(fd, os.SEEK_SET, 0)
+            with open(fd, "rb", closefd=False) as fp:
+                output = fp.read()
+            output = output.decode('ascii', 'backslashreplace')
         output = re.sub('Current thread 0x[0-9a-f]+',
                         'Current thread XXX',
                         output)
         return output.splitlines(), exitcode
 
     def check_fatal_error(self, code, line_number, name_regex,
-                          filename=None, all_threads=True, other_regex=None):
+                          filename=None, all_threads=True, other_regex=None,
+                          fd=None):
         """
         Check that the fault handler for fatal errors is enabled and check the
         traceback from the child process output.
@@ -93,7 +103,7 @@ class FaultHandlerTests(unittest.TestCase):
             header=re.escape(header))).strip()
         if other_regex:
             regex += '|' + other_regex
-        output, exitcode = self.get_output(code, filename)
+        output, exitcode = self.get_output(code, filename=filename, fd=fd)
         output = '\n'.join(output)
         self.assertRegex(output, regex)
         self.assertNotEqual(exitcode, 0)
@@ -211,6 +221,19 @@ class FaultHandlerTests(unittest.TestCase):
                 'Segmentation fault',
                 filename=filename)
 
+    def test_enable_fd(self):
+        with tempfile.TemporaryFile('wb+') as fp:
+            fd = fp.fileno()
+            self.check_fatal_error("""
+                import faulthandler
+                import sys
+                faulthandler.enable(%s)
+                faulthandler._sigsegv()
+                """ % fd,
+                4,
+                'Segmentation fault',
+                fd=fd)
+
     def test_enable_single_thread(self):
         self.check_fatal_error("""
             import faulthandler
@@ -297,7 +320,7 @@ class FaultHandlerTests(unittest.TestCase):
         output = subprocess.check_output(args, env=env)
         self.assertEqual(output.rstrip(), b"True")
 
-    def check_dump_traceback(self, filename):
+    def check_dump_traceback(self, *, filename=None, fd=None):
         """
         Explicitly call dump_traceback() function and check its output.
         Raise an error if the output doesn't match the expected format.
@@ -305,10 +328,16 @@ class FaultHandlerTests(unittest.TestCase):
         code = """
             import faulthandler
 
+            filename = {filename!r}
+            fd = {fd}
+
             def funcB():
-                if {has_filename}:
-                    with open({filename}, "wb") as fp:
+                if filename:
+                    with open(filename, "wb") as fp:
                         faulthandler.dump_traceback(fp, all_threads=False)
+                elif fd is not None:
+                    faulthandler.dump_traceback(fd,
+                                                all_threads=False)
                 else:
                     faulthandler.dump_traceback(all_threads=False)
 
@@ -318,29 +347,35 @@ class FaultHandlerTests(unittest.TestCase):
             funcA()
             """
         code = code.format(
-            filename=repr(filename),
-            has_filename=bool(filename),
+            filename=filename,
+            fd=fd,
         )
         if filename:
-            lineno = 6
+            lineno = 9
+        elif fd is not None:
+            lineno = 12
         else:
-            lineno = 8
+            lineno = 14
         expected = [
             'Stack (most recent call first):',
             '  File "<string>", line %s in funcB' % lineno,
-            '  File "<string>", line 11 in funcA',
-            '  File "<string>", line 13 in <module>'
+            '  File "<string>", line 17 in funcA',
+            '  File "<string>", line 19 in <module>'
         ]
-        trace, exitcode = self.get_output(code, filename)
+        trace, exitcode = self.get_output(code, filename, fd)
         self.assertEqual(trace, expected)
         self.assertEqual(exitcode, 0)
 
     def test_dump_traceback(self):
-        self.check_dump_traceback(None)
+        self.check_dump_traceback()
 
     def test_dump_traceback_file(self):
         with temporary_filename() as filename:
-            self.check_dump_traceback(filename)
+            self.check_dump_traceback(filename=filename)
+
+    def test_dump_traceback_fd(self):
+        with tempfile.TemporaryFile('wb+') as fp:
+            self.check_dump_traceback(fd=fp.fileno())
 
     def test_truncate(self):
         maxlen = 500
@@ -433,7 +468,10 @@ class FaultHandlerTests(unittest.TestCase):
         with temporary_filename() as filename:
             self.check_dump_traceback_threads(filename)
 
-    def _check_dump_traceback_later(self, repeat, cancel, filename, loops):
+    @unittest.skipIf(not hasattr(faulthandler, 'dump_traceback_later'),
+                     'need faulthandler.dump_traceback_later()')
+    def check_dump_traceback_later(self, repeat=False, cancel=False, loops=1,
+                                   *, filename=None, fd=None):
         """
         Check how many times the traceback is written in timeout x 2.5 seconds,
         or timeout x 3.5 seconds if cancel is True: 1, 2 or 3 times depending
@@ -445,6 +483,14 @@ class FaultHandlerTests(unittest.TestCase):
         code = """
             import faulthandler
             import time
+            import sys
+
+            timeout = {timeout}
+            repeat = {repeat}
+            cancel = {cancel}
+            loops = {loops}
+            filename = {filename!r}
+            fd = {fd}
 
             def func(timeout, repeat, cancel, file, loops):
                 for loop in range(loops):
@@ -454,16 +500,14 @@ class FaultHandlerTests(unittest.TestCase):
                     time.sleep(timeout * 5)
                     faulthandler.cancel_dump_traceback_later()
 
-            timeout = {timeout}
-            repeat = {repeat}
-            cancel = {cancel}
-            loops = {loops}
-            if {has_filename}:
-                file = open({filename}, "wb")
+            if filename:
+                file = open(filename, "wb")
+            elif fd is not None:
+                file = sys.stderr.fileno()
             else:
                 file = None
             func(timeout, repeat, cancel, file, loops)
-            if file is not None:
+            if filename:
                 file.close()
             """
         code = code.format(
@@ -471,8 +515,8 @@ class FaultHandlerTests(unittest.TestCase):
             repeat=repeat,
             cancel=cancel,
             loops=loops,
-            has_filename=bool(filename),
-            filename=repr(filename),
+            filename=filename,
+            fd=fd,
         )
         trace, exitcode = self.get_output(code, filename)
         trace = '\n'.join(trace)
@@ -482,27 +526,12 @@ class FaultHandlerTests(unittest.TestCase):
             if repeat:
                 count *= 2
             header = r'Timeout \(%s\)!\nThread 0x[0-9a-f]+ \(most recent call first\):\n' % timeout_str
-            regex = expected_traceback(9, 20, header, min_count=count)
+            regex = expected_traceback(17, 26, header, min_count=count)
             self.assertRegex(trace, regex)
         else:
             self.assertEqual(trace, '')
         self.assertEqual(exitcode, 0)
 
-    @unittest.skipIf(not hasattr(faulthandler, 'dump_traceback_later'),
-                     'need faulthandler.dump_traceback_later()')
-    def check_dump_traceback_later(self, repeat=False, cancel=False,
-                                    file=False, twice=False):
-        if twice:
-            loops = 2
-        else:
-            loops = 1
-        if file:
-            with temporary_filename() as filename:
-                self._check_dump_traceback_later(repeat, cancel,
-                                                  filename, loops)
-        else:
-            self._check_dump_traceback_later(repeat, cancel, None, loops)
-
     def test_dump_traceback_later(self):
         self.check_dump_traceback_later()
 
@@ -513,15 +542,20 @@ class FaultHandlerTests(unittest.TestCase):
         self.check_dump_traceback_later(cancel=True)
 
     def test_dump_traceback_later_file(self):
-        self.check_dump_traceback_later(file=True)
+        with temporary_filename() as filename:
+            self.check_dump_traceback_later(filename=filename)
+
+    def test_dump_traceback_later_fd(self):
+        with tempfile.TemporaryFile('wb+') as fp:
+            self.check_dump_traceback_later(fd=fp.fileno())
 
     def test_dump_traceback_later_twice(self):
-        self.check_dump_traceback_later(twice=True)
+        self.check_dump_traceback_later(loops=2)
 
     @unittest.skipIf(not hasattr(faulthandler, "register"),
                      "need faulthandler.register")
     def check_register(self, filename=False, all_threads=False,
-                       unregister=False, chain=False):
+                       unregister=False, chain=False, fd=None):
         """
         Register a handler displaying the traceback on a user signal. Raise the
         signal and check the written traceback.
@@ -537,6 +571,13 @@ class FaultHandlerTests(unittest.TestCase):
             import signal
             import sys
 
+            all_threads = {all_threads}
+            signum = {signum}
+            unregister = {unregister}
+            chain = {chain}
+            filename = {filename!r}
+            fd = {fd}
+
             def func(signum):
                 os.kill(os.getpid(), signum)
 
@@ -544,19 +585,16 @@ class FaultHandlerTests(unittest.TestCase):
                 handler.called = True
             handler.called = False
 
-            exitcode = 0
-            signum = {signum}
-            unregister = {unregister}
-            chain = {chain}
-
-            if {has_filename}:
-                file = open({filename}, "wb")
+            if filename:
+                file = open(filename, "wb")
+            elif fd is not None:
+                file = sys.stderr.fileno()
             else:
                 file = None
             if chain:
                 signal.signal(signum, handler)
             faulthandler.register(signum, file=file,
-                                  all_threads={all_threads}, chain={chain})
+                                  all_threads=all_threads, chain={chain})
             if unregister:
                 faulthandler.unregister(signum)
             func(signum)
@@ -567,17 +605,19 @@ class FaultHandlerTests(unittest.TestCase):
                     output = sys.stderr
                 print("Error: signal handler not called!", file=output)
                 exitcode = 1
-            if file is not None:
+            else:
+                exitcode = 0
+            if filename:
                 file.close()
             sys.exit(exitcode)
             """
         code = code.format(
-            filename=repr(filename),
-            has_filename=bool(filename),
             all_threads=all_threads,
             signum=signum,
             unregister=unregister,
             chain=chain,
+            filename=filename,
+            fd=fd,
         )
         trace, exitcode = self.get_output(code, filename)
         trace = '\n'.join(trace)
@@ -586,7 +626,7 @@ class FaultHandlerTests(unittest.TestCase):
                 regex = 'Current thread XXX \(most recent call first\):\n'
             else:
                 regex = 'Stack \(most recent call first\):\n'
-            regex = expected_traceback(7, 28, regex)
+            regex = expected_traceback(14, 32, regex)
             self.assertRegex(trace, regex)
         else:
             self.assertEqual(trace, '')
@@ -605,6 +645,10 @@ class FaultHandlerTests(unittest.TestCase):
         with temporary_filename() as filename:
             self.check_register(filename=filename)
 
+    def test_register_fd(self):
+        with tempfile.TemporaryFile('wb+') as fp:
+            self.check_register(fd=fp.fileno())
+
     def test_register_threads(self):
         self.check_register(all_threads=True)
 
diff --git a/Misc/NEWS b/Misc/NEWS
index a07af34..5c87046 100644
--- a/Misc/NEWS
+++ b/Misc/NEWS
@@ -18,6 +18,10 @@ Core and Builtins
 Library
 -------
 
+- Issue #23566: enable(), register(), dump_traceback() and
+  dump_traceback_later() functions of faulthandler now accept file
+  descriptors. Patch by Wei Wu.
+
 - Issue #22928: Disabled HTTP header injections in http.client.
   Original patch by Demian Brecht.
 
diff --git a/Modules/faulthandler.c b/Modules/faulthandler.c
index 4643c0e..def7b29 100644
--- a/Modules/faulthandler.c
+++ b/Modules/faulthandler.c
@@ -133,32 +133,46 @@ static stack_t stack;
    call its flush() method.
 
    If file is NULL or Py_None, use sys.stderr as the new file.
+   If file is an integer, it will be treated as file descriptor.
 
-   On success, return the new file and write the file descriptor into *p_fd.
-   On error, return NULL. */
+   On success, return the file descriptor and write the new file into *file_ptr.
+   On error, return -1. */
 
-static PyObject*
-faulthandler_get_fileno(PyObject *file, int *p_fd)
+static int
+faulthandler_get_fileno(PyObject **file_ptr)
 {
     PyObject *result;
     long fd_long;
     int fd;
+    PyObject *file = *file_ptr;
 
     if (file == NULL || file == Py_None) {
         file = _PySys_GetObjectId(&PyId_stderr);
         if (file == NULL) {
             PyErr_SetString(PyExc_RuntimeError, "unable to get sys.stderr");
-            return NULL;
+            return -1;
         }
         if (file == Py_None) {
             PyErr_SetString(PyExc_RuntimeError, "sys.stderr is None");
-            return NULL;
+            return -1;
+        }
+    }
+    else if (PyLong_Check(file)) {
+        fd = _PyLong_AsInt(file);
+        if (fd == -1 && PyErr_Occurred())
+            return -1;
+        if (fd < 0 || !_PyVerify_fd(fd)) {
+            PyErr_SetString(PyExc_ValueError,
+                            "file is not a valid file descripter");
+            return -1;
         }
+        *file_ptr = NULL;
+        return fd;
     }
 
     result = _PyObject_CallMethodId(file, &PyId_fileno, "");
     if (result == NULL)
-        return NULL;
+        return -1;
 
     fd = -1;
     if (PyLong_Check(result)) {
@@ -171,7 +185,7 @@ faulthandler_get_fileno(PyObject *file, int *p_fd)
     if (fd == -1) {
         PyErr_SetString(PyExc_RuntimeError,
                         "file.fileno() is not a valid file descriptor");
-        return NULL;
+        return -1;
     }
 
     result = _PyObject_CallMethodId(file, &PyId_flush, "");
@@ -181,8 +195,8 @@ faulthandler_get_fileno(PyObject *file, int *p_fd)
         /* ignore flush() error */
         PyErr_Clear();
     }
-    *p_fd = fd;
-    return file;
+    *file_ptr = file;
+    return fd;
 }
 
 /* Get the state of the current thread: only call this function if the current
@@ -215,8 +229,8 @@ faulthandler_dump_traceback_py(PyObject *self,
         &file, &all_threads))
         return NULL;
 
-    file = faulthandler_get_fileno(file, &fd);
-    if (file == NULL)
+    fd = faulthandler_get_fileno(&file);
+    if (fd < 0)
         return NULL;
 
     tstate = get_thread_state();
@@ -339,8 +353,8 @@ faulthandler_enable(PyObject *self, PyObject *args, PyObject *kwargs)
         "|Oi:enable", kwlist, &file, &all_threads))
         return NULL;
 
-    file = faulthandler_get_fileno(file, &fd);
-    if (file == NULL)
+    fd = faulthandler_get_fileno(&file);
+    if (fd < 0)
         return NULL;
 
     tstate = get_thread_state();
@@ -348,7 +362,7 @@ faulthandler_enable(PyObject *self, PyObject *args, PyObject *kwargs)
         return NULL;
 
     Py_XDECREF(fatal_error.file);
-    Py_INCREF(file);
+    Py_XINCREF(file);
     fatal_error.file = file;
     fatal_error.fd = fd;
     fatal_error.all_threads = all_threads;
@@ -553,8 +567,8 @@ faulthandler_dump_traceback_later(PyObject *self,
     if (tstate == NULL)
         return NULL;
 
-    file = faulthandler_get_fileno(file, &fd);
-    if (file == NULL)
+    fd = faulthandler_get_fileno(&file);
+    if (fd < 0)
         return NULL;
 
     /* format the timeout */
@@ -567,7 +581,7 @@ faulthandler_dump_traceback_later(PyObject *self,
     cancel_dump_traceback_later();
 
     Py_XDECREF(thread.file);
-    Py_INCREF(file);
+    Py_XINCREF(file);
     thread.file = file;
     thread.fd = fd;
     thread.timeout_us = timeout_us;
@@ -737,8 +751,8 @@ faulthandler_register_py(PyObject *self,
     if (tstate == NULL)
         return NULL;
 
-    file = faulthandler_get_fileno(file, &fd);
-    if (file == NULL)
+    fd = faulthandler_get_fileno(&file);
+    if (fd < 0)
         return NULL;
 
     if (user_signals == NULL) {
@@ -760,7 +774,7 @@ faulthandler_register_py(PyObject *self,
     }
 
     Py_XDECREF(user->file);
-    Py_INCREF(file);
+    Py_XINCREF(file);
     user->file = file;
     user->fd = fd;
     user->all_threads = all_threads;