Try to repair at least one segfault on the Mac buildbot,

as diagnosed by Nick Coghlan.

test_capi.py:  A test module should never spawn a thread as
a side effect of being imported.  Because this one did, the
segfault one of its thread tests caused didn't occur until
a few tests after test_regrtest.py thought test_capi was
finished.  Repair that.  Also join() the thread spawned
at the end, so that test_capi is truly finished when
regrtest reports that it's done.

_testcapimodule.c test_thread_state():  this spawns a
couple of non-threading.py threads, passing them a PyObject*
argument, but did nothing to ensure that those threads
finished before returning.  As a result, the PyObject*
_could_ (although this was unlikely) get decref'ed out of
existence before the threads got around to using it.
Added explicit synchronization (via a Python mutex) so
that test_thread_state can reliably wait for its spawned
threads to finish.

diff --git a/Lib/test/test_capi.py b/Lib/test/test_capi.py
index 1dd24616a25ff0e0df896cef589c8282335af77f..cdd84bb222e24fb3477fba28b891a3429395ba36 100644 (file)
--- a/Lib/test/test_capi.py
+++ b/Lib/test/test_capi.py
@@ -5,44 +5,51 @@ import sys
 from test import test_support
 import _testcapi
 
-for name in dir(_testcapi):
-    if name.startswith('test_'):
-        test = getattr(_testcapi, name)
+def test_main():
+
+    for name in dir(_testcapi):
+        if name.startswith('test_'):
+            test = getattr(_testcapi, name)
+            if test_support.verbose:
+                print "internal", name
+            try:
+                test()
+            except _testcapi.error:
+                raise test_support.TestFailed, sys.exc_info()[1]
+
+    # some extra thread-state tests driven via _testcapi
+    def TestThreadState():
+        import thread
+        import time
+
         if test_support.verbose:
-            print "internal", name
-        try:
-            test()
-        except _testcapi.error:
-            raise test_support.TestFailed, sys.exc_info()[1]
-
-# some extra thread-state tests driven via _testcapi
-def TestThreadState():
-    import thread
-    import time
-
-    if test_support.verbose:
-        print "auto-thread-state"
-
-    idents = []
-
-    def callback():
-        idents.append(thread.get_ident())
-
-    _testcapi._test_thread_state(callback)
-    time.sleep(1)
-    # Check our main thread is in the list exactly 3 times.
-    if idents.count(thread.get_ident()) != 3:
-        raise test_support.TestFailed, \
-              "Couldn't find main thread correctly in the list"
-
-try:
-    _testcapi._test_thread_state
-    have_thread_state = True
-except AttributeError:
-    have_thread_state = False
-
-if have_thread_state:
-    TestThreadState()
-    import threading
-    t=threading.Thread(target=TestThreadState)
-    t.start()
+            print "auto-thread-state"
+
+        idents = []
+
+        def callback():
+            idents.append(thread.get_ident())
+
+        _testcapi._test_thread_state(callback)
+        a = b = callback
+        time.sleep(1)
+        # Check our main thread is in the list exactly 3 times.
+        if idents.count(thread.get_ident()) != 3:
+            raise test_support.TestFailed, \
+                  "Couldn't find main thread correctly in the list"
+
+    try:
+        _testcapi._test_thread_state
+        have_thread_state = True
+    except AttributeError:
+        have_thread_state = False
+
+    if have_thread_state:
+        TestThreadState()
+        import threading
+        t=threading.Thread(target=TestThreadState)
+        t.start()
+        t.join()
+
+if __name__ == "__main__":
+    test_main()

diff --git a/Modules/_testcapimodule.c b/Modules/_testcapimodule.c
index 263c61e0b544c815d9de79f50ac3fdf357359eb5..60c71d7886b063d8e04afd53bb26badfbf478074 100644 (file)
--- a/Modules/_testcapimodule.c
+++ b/Modules/_testcapimodule.c
@@ -10,7 +10,6 @@
 #ifdef WITH_THREAD
 #include "pythread.h"
 #endif /* WITH_THREAD */
-
 static PyObject *TestError;    /* set to exception object in init */
 
 /* Raise TestError with test_name + ": " + msg, and return NULL. */
@@ -482,7 +481,7 @@ static
 PyObject *codec_incrementalencoder(PyObject *self, PyObject *args)
 {
        const char *encoding, *errors = NULL;
-       if (!PyArg_ParseTuple(args, "s|s:test_incrementalencoder", 
+       if (!PyArg_ParseTuple(args, "s|s:test_incrementalencoder",
                              &encoding, &errors))
                return NULL;
        return PyCodec_IncrementalEncoder(encoding, errors);
@@ -492,7 +491,7 @@ static
 PyObject *codec_incrementaldecoder(PyObject *self, PyObject *args)
 {
        const char *encoding, *errors = NULL;
-       if (!PyArg_ParseTuple(args, "s|s:test_incrementaldecoder", 
+       if (!PyArg_ParseTuple(args, "s|s:test_incrementaldecoder",
                              &encoding, &errors))
                return NULL;
        return PyCodec_IncrementalDecoder(encoding, errors);
@@ -583,7 +582,17 @@ raise_exception(PyObject *self, PyObject *args)
 
 #ifdef WITH_THREAD
 
-void _make_call(void *callable)
+/* test_thread_state spawns a thread of its own, and that thread releases
+ * `thread_done` when it's finished.  The driver code has to know when the
+ * thread finishes, because the thread uses a PyObject (the callable) that
+ * may go away when the driver finishes.  The former lack of this explicit
+ * synchronization caused rare segfaults, so rare that they were seen only
+ * on a Mac buildbot (although they were possible on any box).
+ */
+static PyThread_type_lock thread_done = NULL;
+
+static void
+_make_call(void *callable)
 {
        PyObject *rc;
        PyGILState_STATE s = PyGILState_Ensure();
@@ -592,32 +601,53 @@ void _make_call(void *callable)
        PyGILState_Release(s);
 }
 
+/* Same thing, but releases `thread_done` when it returns.  This variant
+ * should be called only from threads spawned by test_thread_state().
+ */
+static void
+_make_call_from_thread(void *callable)
+{
+       _make_call(callable);
+       PyThread_release_lock(thread_done);
+}
+
 static PyObject *
 test_thread_state(PyObject *self, PyObject *args)
 {
        PyObject *fn;
+
        if (!PyArg_ParseTuple(args, "O:test_thread_state", &fn))
                return NULL;
-       /* Ensure Python is setup for threading */
+
+       /* Ensure Python is set up for threading */
        PyEval_InitThreads();
-       /* Start a new thread for our callback. */
-       PyThread_start_new_thread( _make_call, fn);
+       thread_done = PyThread_allocate_lock();
+       if (thread_done == NULL)
+               return PyErr_NoMemory();
+       PyThread_acquire_lock(thread_done, 1);
+
+       /* Start a new thread with our callback. */
+       PyThread_start_new_thread(_make_call_from_thread, fn);
        /* Make the callback with the thread lock held by this thread */
        _make_call(fn);
        /* Do it all again, but this time with the thread-lock released */
        Py_BEGIN_ALLOW_THREADS
        _make_call(fn);
+       PyThread_acquire_lock(thread_done, 1);  /* wait for thread to finish */
        Py_END_ALLOW_THREADS
+
        /* And once more with and without a thread
-          XXX - should use a lock and work out exactly what we are trying 
-          to test <wink> 
+          XXX - should use a lock and work out exactly what we are trying
+          to test <wink>
        */
        Py_BEGIN_ALLOW_THREADS
-       PyThread_start_new_thread( _make_call, fn);
+       PyThread_start_new_thread(_make_call_from_thread, fn);
        _make_call(fn);
+       PyThread_acquire_lock(thread_done, 1);  /* wait for thread to finish */
        Py_END_ALLOW_THREADS
-       Py_INCREF(Py_None);
-       return Py_None;
+
+       PyThread_free_lock(thread_done);
+       Py_RETURN_NONE;
 }
 #endif