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.

Author: ericsnowcurrently

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

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)

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;

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;