[Python-checkins] bpo-33615: Re-enable a subinterpreter test. (gh-7251)

[Eric Snow]

https://github.com/python/cpython/commit/63799136e6c0491bb5d6f4a234d5a775db3458db
commit: 63799136e6c0491bb5d6f4a234d5a775db3458db
branch: master
author: Eric Snow <ericsnowcurrently at gmail.com>
committer: GitHub <noreply at github.com>
date: 2018-06-01T18:45:20-06:00
summary:

bpo-33615: Re-enable a subinterpreter test. (gh-7251)

For bpo-32604 I added extra subinterpreter-related tests (see #6914), which caused a few buildbots to crash. This patch fixes the crash by ensuring that refcounts in channels are handled properly.

files:
M Include/internal/pystate.h
M Lib/test/test__xxsubinterpreters.py
M Modules/_xxsubinterpretersmodule.c
M Python/pystate.c

diff --git a/Include/internal/pystate.h b/Include/internal/pystate.h
index da642c6fd007..70e066662582 100644
--- a/Include/internal/pystate.h
+++ b/Include/internal/pystate.h
@@ -80,7 +80,7 @@ struct _xid;
 
 // _PyCrossInterpreterData is similar to Py_buffer as an effectively
 // opaque struct that holds data outside the object machinery.  This
-// is necessary to pass between interpreters in the same process.
+// is necessary to pass safely between interpreters in the same process.
 typedef struct _xid {
     // data is the cross-interpreter-safe derivation of a Python object
     // (see _PyObject_GetCrossInterpreterData).  It will be NULL if the
@@ -89,8 +89,9 @@ typedef struct _xid {
     // obj is the Python object from which the data was derived.  This
     // is non-NULL only if the data remains bound to the object in some
     // way, such that the object must be "released" (via a decref) when
-    // the data is released.  In that case it is automatically
-    // incref'ed (to match the automatic decref when releaed).
+    // the data is released.  In that case the code that sets the field,
+    // likely a registered "crossinterpdatafunc", is responsible for
+    // ensuring it owns the reference (i.e. incref).
     PyObject *obj;
     // interp is the ID of the owning interpreter of the original
     // object.  It corresponds to the active interpreter when
diff --git a/Lib/test/test__xxsubinterpreters.py b/Lib/test/test__xxsubinterpreters.py
index 0667f14f7bf4..f66cc9516926 100644
--- a/Lib/test/test__xxsubinterpreters.py
+++ b/Lib/test/test__xxsubinterpreters.py
@@ -1315,8 +1315,6 @@ def test_run_string_arg_unresolved(self):
         self.assertEqual(obj, b'spam')
         self.assertEqual(out.strip(), 'send')
 
-    # XXX Fix the crashes.
-    @unittest.skip('bpo-33615: triggering crashes so temporarily disabled')
     def test_run_string_arg_resolved(self):
         cid = interpreters.channel_create()
         cid = interpreters._channel_id(cid, _resolve=True)
diff --git a/Modules/_xxsubinterpretersmodule.c b/Modules/_xxsubinterpretersmodule.c
index f3e65cd187ed..129067cbd192 100644
--- a/Modules/_xxsubinterpretersmodule.c
+++ b/Modules/_xxsubinterpretersmodule.c
@@ -1712,6 +1712,7 @@ _channelid_shared(PyObject *obj, _PyCrossInterpreterData *data)
     xid->resolve = ((channelid *)obj)->resolve;
 
     data->data = xid;
+    Py_INCREF(obj);
     data->obj = obj;
     data->new_object = _channelid_from_xid;
     data->free = PyMem_Free;
diff --git a/Python/pystate.c b/Python/pystate.c
index 4534c4770f24..629598e215ef 100644
--- a/Python/pystate.c
+++ b/Python/pystate.c
@@ -1205,7 +1205,6 @@ _PyObject_GetCrossInterpreterData(PyObject *obj, _PyCrossInterpreterData *data)
     }
 
     // Fill in the blanks and validate the result.
-    Py_XINCREF(data->obj);
     data->interp = interp->id;
     if (_check_xidata(data) != 0) {
         _PyCrossInterpreterData_Release(data);
@@ -1215,6 +1214,40 @@ _PyObject_GetCrossInterpreterData(PyObject *obj, _PyCrossInterpreterData *data)
     return 0;
 }
 
+static void
+_release_xidata(void *arg)
+{
+    _PyCrossInterpreterData *data = (_PyCrossInterpreterData *)arg;
+    if (data->free != NULL) {
+        data->free(data->data);
+    }
+    Py_XDECREF(data->obj);
+}
+
+static void
+_call_in_interpreter(PyInterpreterState *interp,
+                     void (*func)(void *), void *arg)
+{
+    /* We would use Py_AddPendingCall() if it weren't specific to the
+     * main interpreter (see bpo-33608).  In the meantime we take a
+     * naive approach.
+     */
+    PyThreadState *save_tstate = NULL;
+    if (interp != PyThreadState_Get()->interp) {
+        // XXX Using the "head" thread isn't strictly correct.
+        PyThreadState *tstate = PyInterpreterState_ThreadHead(interp);
+        // XXX Possible GILState issues?
+        save_tstate = PyThreadState_Swap(tstate);
+    }
+
+    func(arg);
+
+    // Switch back.
+    if (save_tstate != NULL) {
+        PyThreadState_Swap(save_tstate);
+    }
+}
+
 void
 _PyCrossInterpreterData_Release(_PyCrossInterpreterData *data)
 {
@@ -1233,24 +1266,8 @@ _PyCrossInterpreterData_Release(_PyCrossInterpreterData *data)
         return;
     }
 
-    PyThreadState *save_tstate = NULL;
-    if (interp != PyThreadState_Get()->interp) {
-        // XXX Using the "head" thread isn't strictly correct.
-        PyThreadState *tstate = PyInterpreterState_ThreadHead(interp);
-        // XXX Possible GILState issues?
-        save_tstate = PyThreadState_Swap(tstate);
-    }
-
     // "Release" the data and/or the object.
-    if (data->free != NULL) {
-        data->free(data->data);
-    }
-    Py_XDECREF(data->obj);
-
-    // Switch back.
-    if (save_tstate != NULL) {
-        PyThreadState_Swap(save_tstate);
-    }
+    _call_in_interpreter(interp, _release_xidata, data);
 }
 
 PyObject *
@@ -1355,6 +1372,7 @@ _bytes_shared(PyObject *obj, _PyCrossInterpreterData *data)
         return -1;
     }
     data->data = (void *)shared;
+    Py_INCREF(obj);
     data->obj = obj;  // Will be "released" (decref'ed) when data released.
     data->new_object = _new_bytes_object;
     data->free = PyMem_Free;
@@ -1382,6 +1400,7 @@ _str_shared(PyObject *obj, _PyCrossInterpreterData *data)
     shared->buffer = PyUnicode_DATA(obj);
     shared->len = PyUnicode_GET_LENGTH(obj) - 1;
     data->data = (void *)shared;
+    Py_INCREF(obj);
     data->obj = obj;  // Will be "released" (decref'ed) when data released.
     data->new_object = _new_str_object;
     data->free = PyMem_Free;