From de10f4054b6bd6dd286559d7bbaaff7364282fc7 Mon Sep 17 00:00:00 2001 From: Victor Stinner Date: Fri, 8 Apr 2011 12:57:06 +0200 Subject: [PATCH] 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 | 45 ++++++++++++++++----------- Modules/faulthandler.c | 57 +++++++++++++++++------------------ 2 files changed, 55 insertions(+), 47 deletions(-) diff --git a/Lib/test/test_faulthandler.py b/Lib/test/test_faulthandler.py index bfe662eb021..483c7f1a29f 100644 --- a/Lib/test/test_faulthandler.py +++ b/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): - if cancel: +def func(timeout, repeat, cancel, file, loops): + 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, - repeat=repeat, file=file) -func(repeat, cancel, timeout) +func(timeout, repeat, cancel, file, loops) if file is not None: file.close() """.strip() code = code.format( - filename=repr(filename), - has_filename=bool(filename), + timeout=TIMEOUT, 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 - else: - count = 1 + count *= 2 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, diff --git a/Modules/faulthandler.c b/Modules/faulthandler.c index 113203692cc..d2852f9ab58 100644 --- a/Modules/faulthandler.c +++ b/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.join_event); + PyThread_release_lock(thread.running); } static void -faulthandler_cancel_dump_tracebacks_later(void) +cancel_dump_tracebacks_later(void) { - if (thread.running) { - /* Notify cancellation */ - PyThread_release_lock(thread.cancel_event); - } + /* notify cancellation */ + PyThread_release_lock(thread.cancel_event); + /* Wait for thread to join */ - PyThread_acquire_lock(thread.join_event, 1); - PyThread_release_lock(thread.join_event); - thread.running = 0; + PyThread_acquire_lock(thread.running, 1); + PyThread_release_lock(thread.running); + + /* 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.cancel_event, 1); + PyThread_acquire_lock(thread.running, 1); - thread.running = 1; if (PyThread_start_new_thread(faulthandler_thread, NULL) == -1) { - thread.running = 0; - PyThread_release_lock(thread.join_event); - PyThread_release_lock(thread.cancel_event); + PyThread_release_lock(thread.running); 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(); - if (!thread.cancel_event || !thread.join_event) { + thread.running = PyThread_allocate_lock(); + 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) { - PyThread_free_lock(thread.join_event); - thread.join_event = NULL; + if (thread.running) { + PyThread_free_lock(thread.running); + thread.running = NULL; } #endif