PEP 788: Address feedback from first discussion round

peps/pep-0788.rst

@@ -333,8 +333,8 @@ object are wrong! There isn't any synchronization between the two GILs, so both
 the thread (who thinks it's in the subinterpreter) and the main thread could try
 to increment the reference count at the same time, causing a data race!
 
-Concurrent Interpreter Deallocation is Frustrating
---------------------------------------------------
+An Interpreter Can Concurrently Deallocate
+------------------------------------------
 
 The other way of creating a native thread that can invoke Python,
 :c:func:`PyThreadState_New` and :c:func:`PyThreadState_Swap`, is a lot better
@@ -431,25 +431,32 @@ Strong Interpreter References
 
    This type is guaranteed to be pointer-sized.
 
-.. c:function:: PyInterpreterRef PyInterpreterRef_Get(void)
+.. c:function:: int PyInterpreterRef_Get(PyInterpreterRef *ref_ptr)
 
     Acquire a strong reference to the current interpreter.
 
-    This function cannot fail, other than with a fatal error when the caller
-    doesn't hold an :term:`attached thread state`.
+    On success, this function returns ``0`` and sets *ref_ptr*
+    to a strong reference to the interpreter, and returns ``-1``
+    with an exception set on failure.
+
+    Failure typically indicates that the interpreter has
+    already finished waiting on strong references.
+
+    The caller must hold an :term:`attached thread state`.
 
-.. c:function:: int PyInterpreterState_AsStrong(PyInterpreterState *interp, PyInterpreterRef *ref_ptr)
+.. c:function:: int PyInterpreterRef_Main(PyInterpreterRef *ref_ptr)
 
-    Acquire a strong reference to *interp*.
+    Acquire a strong reference to the main interpreter.
 
-    Unless *interp* is the main interpreter, this function can cause crashes
-    if *interp* shuts down in another thread! Prefer safely acquiring a
-    reference through :c:func:`PyInterpreterRef_Get` whenever possible.
+    This function only exists for special cases where a specific interpreter
+    can't be saved. Prefer safely acquiring a reference through
+    :c:func:`PyInterpreterRef_Get` whenever possible.
 
     On success, this function will return ``0`` and set *ref_ptr* to a strong
     reference, and on failure, this function will return ``-1``.
-    (Failure typically indicates that *interp* has already finished
-    waiting on its reference count.)
+
+    Failure typically indicates that the main interpreter has already finished
+    waiting on its reference count.
 
     The caller does not need to hold an :term:`attached thread state`.
 
@@ -484,21 +491,22 @@ Weak Interpreter References
     The interpreter will *not* wait for the reference to be
     released before shutting down.
 
-.. c:function:: PyInterpreterWeakRef PyInterpreterWeakRef_Get(void)
+.. c:function:: int PyInterpreterWeakRef_Get(PyInterpreterWeakRef *wref_ptr)
 
     Acquire a weak reference to the current interpreter.
 
     This function is generally meant to be used in tandem with
-    :c:func:`PyInterpreterWeakRef_AsStrong`, and cannot fail.
+    :c:func:`PyInterpreterWeakRef_AsStrong`.
+
+    On success, this function returns ``0`` and sets *wref_ptr* to a
+    weak reference to the interpreter, and returns ``-1`` with an exception
+    set on failure.
 
     The caller must hold an :term:`attached thread state`.
 
 .. c:function:: PyInterpreterWeakRef PyInterpreterWeakRef_Dup(PyInterpreterWeakRef wref)
 
-    Duplicate a weak reference to *wref*.
-
-    This function is generally meant to be used in tandem with
-    :c:func:`PyInterpreterWeakRef_AsStrong`.
+    Duplicate a weak reference to an interpreter.
 
     This function cannot fail, and the caller doesn't need to hold an
     :term:`attached thread state`.
@@ -519,7 +527,7 @@ Weak Interpreter References
 
 .. c:function:: void PyInterpreterWeakRef_Close(PyInterpreterWeakRef wref)
 
-    Release a weak reference, possibly deallocating it.
+    Release a weak reference to an interpreter.
 
     This function cannot fail, and the caller doesn't need to hold an
     :term:`attached thread state`.
@@ -547,7 +555,7 @@ replace :c:func:`PyGILState_Ensure` and :c:func:`PyGILState_Release`.
     if the interpreter matches *ref*, it is attached, and otherwise a new
     thread state is created.
 
-    Return zero on success, and non-zero on failure.
+    Return ``0`` on success, and ``-1`` on failure.
 
 .. c:function:: void PyThreadState_Release()
 
@@ -620,7 +628,7 @@ With this PEP, you'd implement it like this:
     {
         PyInterpreterRef ref;
         if (PyInterpreterWeakRef_AsStrong(wref, &ref) < 0) {
-            // Python interpreter has shut down
+            /* Python interpreter has shut down */
             return -1;
         }
 
@@ -662,7 +670,11 @@ held. Any future finalizer that wanted to acquire the lock would be deadlocked!
     my_critical_operation(PyObject *self, PyObject *unused)
     {
         assert(PyThreadState_GetUnchecked() != NULL);
-        PyInterpreterRef ref = PyInterpreterRef_Get();
+        PyInterpreterRef ref;
+        if (PyInterpreterRef_Get(&ref) < 0) {
+            /* Python interpreter has shut down */
+            return NULL;
+        }
         /* Temporarily hold a strong reference to ensure that the
            lock is released. */
         if (PyThreadState_Ensure(ref) < 0) {
@@ -748,7 +760,11 @@ This is the same code, rewritten to use the new functions:
         PyThread_handle_t handle;
         PyThead_indent_t indent;
 
-        PyInterpreterRef ref = PyInterpreterRef_Get();
+        PyInterpreterRef ref;
+        if (PyInterpreterRef_Get(&ref) < 0) {
+            return NULL;
+        }
+
         if (PyThread_start_joinable_thread(thread_func, (void *)ref, &ident, &handle) < 0) {
             PyInterpreterRef_Close(ref);
             return NULL;
@@ -792,7 +808,11 @@ they can still be used with this API:
         PyThread_handle_t handle;
         PyThead_indent_t indent;
 
-        PyInterpreterRef ref = PyInterpreterRef_Get();
+        PyInterpreterRef ref;
+        if (PyInterpreterRef_Get(&ref) < 0) {
+            return NULL;
+        }
+
         if (PyThread_start_joinable_thread(thread_func, (void *)ref, &ident, &handle) < 0) {
             PyInterpreterRef_Close(ref);
             return NULL;
@@ -846,7 +866,10 @@ deadlock the interpreter if it's not released.
             PyErr_NoMemory();
             return NULL;
         }
-        PyInterpreterWeakRef wref = PyInterpreterWeakRef_Get();
+        PyInterpreterWeakRef wref;
+        if (PyInterpreterWeakRef_Get(&wref) < 0) {
+            return NULL;
+        }
         tdata->wref = wref;
         register_callback(async_callback, tdata);
 
@@ -859,7 +882,7 @@ Example: Calling Python Without a Callback Parameter
 There are a few cases where callback functions don't take a callback parameter
 (``void *arg``), so it's impossible to acquire a reference to any specific
 interpreter. The solution to this problem is to acquire a reference to the main
-interpreter through :c:func:`PyInterpreterState_AsStrong`.
+interpreter through :c:func:`PyInterpreterRef_Main`.
 
 But wait, won't that break with subinterpreters, per
 :ref:`pep-788-subinterpreters-gilstate`? Fortunately, since the callback has
@@ -873,7 +896,7 @@ interpreter here.
     call_python(void)
     {
         PyInterpreterRef ref;
-        if (PyInterpreterState_AsStrong(PyInterpreterState_Main(), &ref) < 0) {
+        if (PyInterpreterRef_Main(&ref) < 0) {
             fputs("Python has shut down!", stderr);
             return;
         }