faulthandler: one more time, fix usage of locks in the watchdog thread

* Write a new test to ensure that dump_tracebacks_later() still works if
   it was already called and then cancelled before
 * Don't use a variable to check the status of the thread, only rely on locks
 * The thread only releases cancel_event if it was able to acquire it (if
   the timer was interrupted)
 * The main thread always hold this lock. It is only released when
   faulthandler_thread() is interrupted until this thread exits, or at Python
   exit.

Lib/test/test_faulthandler.py

@@ -352,7 +352,7 @@ Current thread XXX:
         with temporary_filename() as filename:
             self.check_dump_traceback_threads(filename)
 
-    def _check_dump_tracebacks_later(self, repeat, cancel, filename):
+    def _check_dump_tracebacks_later(self, repeat, cancel, filename, loops):
         """
         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
@@ -364,42 +364,43 @@ Current thread XXX:
 import faulthandler
 import time
 
-def func(repeat, cancel, timeout):
+def func(timeout, repeat, cancel, file, loops):
-    if cancel:
+    for loop in range(loops):
+        faulthandler.dump_tracebacks_later(timeout, repeat=repeat, file=file)
+        if cancel:
+            faulthandler.cancel_dump_tracebacks_later()
+        time.sleep(timeout * 2.5)
         faulthandler.cancel_dump_tracebacks_later()
-    time.sleep(timeout * 2.5)
-    faulthandler.cancel_dump_tracebacks_later()
 
 timeout = {timeout}
 repeat = {repeat}
 cancel = {cancel}
+loops = {loops}
 if {has_filename}:
     file = open({filename}, "wb")
 else:
     file = None
-faulthandler.dump_tracebacks_later(timeout,
+func(timeout, repeat, cancel, file, loops)
-    repeat=repeat, file=file)
-func(repeat, cancel, timeout)
 if file is not None:
     file.close()
 """.strip()
         code = code.format(
-            filename=repr(filename),
+            timeout=TIMEOUT,
-            has_filename=bool(filename),
             repeat=repeat,
             cancel=cancel,
-            timeout=TIMEOUT,
+            loops=loops,
+            has_filename=bool(filename),
+            filename=repr(filename),
         )
         trace, exitcode = self.get_output(code, filename)
         trace = '\n'.join(trace)
 
         if not cancel:
+            count = loops
             if repeat:
-                count = 2
+                count *= 2
-            else:
-                count = 1
             header = 'Thread 0x[0-9a-f]+:\n'
-            regex = expected_traceback(7, 19, header, count=count)
+            regex = expected_traceback(9, 20, header, count=count)
             self.assertRegex(trace, regex)
         else:
             self.assertEqual(trace, '')
@@ -408,12 +409,17 @@ if file is not None:
     @unittest.skipIf(not hasattr(faulthandler, 'dump_tracebacks_later'),
                      'need faulthandler.dump_tracebacks_later()')
     def check_dump_tracebacks_later(self, repeat=False, cancel=False,
-                                  file=False):
+                                    file=False, twice=False):
+        if twice:
+            loops = 2
+        else:
+            loops = 1
         if file:
             with temporary_filename() as filename:
-                self._check_dump_tracebacks_later(repeat, cancel, filename)
+                self._check_dump_tracebacks_later(repeat, cancel,
+                                                  filename, loops)
         else:
-            self._check_dump_tracebacks_later(repeat, cancel, None)
+            self._check_dump_tracebacks_later(repeat, cancel, None, loops)
 
     def test_dump_tracebacks_later(self):
         self.check_dump_tracebacks_later()
@@ -427,6 +433,9 @@ if file is not None:
     def test_dump_tracebacks_later_file(self):
         self.check_dump_tracebacks_later(file=True)
 
+    def test_dump_tracebacks_later_twice(self):
+        self.check_dump_tracebacks_later(twice=True)
+
     @unittest.skipIf(not hasattr(faulthandler, "register"),
                      "need faulthandler.register")
     def check_register(self, filename=False, all_threads=False,

Modules/faulthandler.c

@@ -48,13 +48,14 @@ static struct {
     int fd;
     PY_TIMEOUT_T timeout_ms;   /* timeout in microseconds */
     int repeat;
-    int running;
     PyInterpreterState *interp;
     int exit;
-    /* released by parent thread when cancel request */
+    /* The main thread always hold this lock. It is only released when
+       faulthandler_thread() is interrupted until this thread exits, or at
+       Python exit. */
     PyThread_type_lock cancel_event;
     /* released by child thread when joined */
-    PyThread_type_lock join_event;
+    PyThread_type_lock running;
 } thread;
 #endif
 
@@ -414,7 +415,7 @@ faulthandler_thread(void *unused)
         st = PyThread_acquire_lock_timed(thread.cancel_event,
                                          thread.timeout_ms, 0);
         if (st == PY_LOCK_ACQUIRED) {
-            /* Cancelled by user */
+            PyThread_release_lock(thread.cancel_event);
             break;
         }
         /* Timeout => dump traceback */
@@ -431,21 +432,22 @@ faulthandler_thread(void *unused)
     } while (ok && thread.repeat);
 
     /* The only way out */
-    PyThread_release_lock(thread.cancel_event);
+    PyThread_release_lock(thread.running);
-    PyThread_release_lock(thread.join_event);
 }
 
 static void
-faulthandler_cancel_dump_tracebacks_later(void)
+cancel_dump_tracebacks_later(void)
 {
-    if (thread.running) {
+    /* notify cancellation */
-        /* Notify cancellation */
+    PyThread_release_lock(thread.cancel_event);
-        PyThread_release_lock(thread.cancel_event);
+
-    }
     /* Wait for thread to join */
-    PyThread_acquire_lock(thread.join_event, 1);
+    PyThread_acquire_lock(thread.running, 1);
-    PyThread_release_lock(thread.join_event);
+    PyThread_release_lock(thread.running);
-    thread.running = 0;
+
+    /* The main thread should always hold the cancel_event lock */
+    PyThread_acquire_lock(thread.cancel_event, 1);
+
     Py_CLEAR(thread.file);
 }
 
@@ -489,7 +491,7 @@ faulthandler_dump_tracebacks_later(PyObject *self,
         return NULL;
 
     /* Cancel previous thread, if running */
-    faulthandler_cancel_dump_tracebacks_later();
+    cancel_dump_tracebacks_later();
 
     Py_XDECREF(thread.file);
     Py_INCREF(file);
@@ -501,14 +503,10 @@ faulthandler_dump_tracebacks_later(PyObject *self,
     thread.exit = exit;
 
     /* Arm these locks to serve as events when released */
-    PyThread_acquire_lock(thread.join_event, 1);
+    PyThread_acquire_lock(thread.running, 1);
-    PyThread_acquire_lock(thread.cancel_event, 1);
 
-    thread.running = 1;
     if (PyThread_start_new_thread(faulthandler_thread, NULL) == -1) {
-        thread.running = 0;
+        PyThread_release_lock(thread.running);
-        PyThread_release_lock(thread.join_event);
-        PyThread_release_lock(thread.cancel_event);
         Py_CLEAR(thread.file);
         PyErr_SetString(PyExc_RuntimeError,
                         "unable to start watchdog thread");
@@ -521,7 +519,7 @@ faulthandler_dump_tracebacks_later(PyObject *self,
 static PyObject*
 faulthandler_cancel_dump_tracebacks_later_py(PyObject *self)
 {
-    faulthandler_cancel_dump_tracebacks_later();
+    cancel_dump_tracebacks_later();
     Py_RETURN_NONE;
 }
 #endif /* FAULTHANDLER_LATER */
@@ -1001,15 +999,15 @@ int _PyFaulthandler_Init(void)
     }
 #endif
 #ifdef FAULTHANDLER_LATER
-    thread.running = 0;
     thread.file = NULL;
     thread.cancel_event = PyThread_allocate_lock();
-    thread.join_event = PyThread_allocate_lock();
+    thread.running = PyThread_allocate_lock();
-    if (!thread.cancel_event || !thread.join_event) {
+    if (!thread.cancel_event || !thread.running) {
         PyErr_SetString(PyExc_RuntimeError,
                         "could not allocate locks for faulthandler");
         return -1;
     }
+    PyThread_acquire_lock(thread.cancel_event, 1);
 #endif
 
     return faulthandler_env_options();
@@ -1023,14 +1021,15 @@ void _PyFaulthandler_Fini(void)
 
 #ifdef FAULTHANDLER_LATER
     /* later */
-    faulthandler_cancel_dump_tracebacks_later();
+    cancel_dump_tracebacks_later();
     if (thread.cancel_event) {
+        PyThread_release_lock(thread.cancel_event);
         PyThread_free_lock(thread.cancel_event);
         thread.cancel_event = NULL;
     }
-    if (thread.join_event) {
+    if (thread.running) {
-        PyThread_free_lock(thread.join_event);
+        PyThread_free_lock(thread.running);
-        thread.join_event = NULL;
+        thread.running = NULL;
     }
 #endif