bpo-42208: Call GC collect earlier in PyInterpreterState_Clear() (GH-23044)

vstinner · web-flow · commit eba5bf2f5672 · 2020-10-30T22:51:02.000+01:00
The last GC collection is now done before clearing builtins and sys
dictionaries. Add also assertions to ensure that gc.collect() is no
longer called after _PyGC_Fini().

Pass also the tstate to PyInterpreterState_Clear() to pass the
correct tstate to _PyGC_CollectNoFail() and _PyGC_Fini().
diff --git a/Include/internal/pycore_interp.h b/Include/internal/pycore_interp.h
@@ -267,6 +267,7 @@ extern PyStatus _PyInterpreterState_SetConfig(
     PyInterpreterState *interp,
     const PyConfig *config);
 
+extern void _PyInterpreterState_Clear(PyThreadState *tstate);
 
 
 /* cross-interpreter data registry */
diff --git a/Modules/gcmodule.c b/Modules/gcmodule.c
@@ -1191,6 +1191,11 @@ gc_collect_main(PyThreadState *tstate, int generation,
     _PyTime_t t1 = 0;   /* initialize to prevent a compiler warning */
     GCState *gcstate = &tstate->interp->gc;
 
+    // gc_collect_main() must not be called before _PyGC_Init
+    // or after _PyGC_Fini()
+    assert(gcstate->garbage != NULL);
+    assert(!_PyErr_Occurred(tstate));
+
 #ifdef EXPERIMENTAL_ISOLATED_SUBINTERPRETERS
     if (tstate->interp->config._isolated_interpreter) {
         // bpo-40533: The garbage collector must not be run on parallel on
@@ -2073,16 +2078,13 @@ PyGC_Collect(void)
 Py_ssize_t
 _PyGC_CollectNoFail(PyThreadState *tstate)
 {
-    assert(!_PyErr_Occurred(tstate));
-
-    GCState *gcstate = &tstate->interp->gc;
-
     /* Ideally, this function is only called on interpreter shutdown,
        and therefore not recursively.  Unfortunately, when there are daemon
        threads, a daemon thread can start a cyclic garbage collection
        during interpreter shutdown (and then never finish it).
        See http://bugs.python.org/issue8713#msg195178 for an example.
        */
+    GCState *gcstate = &tstate->interp->gc;
     if (gcstate->collecting) {
         return 0;
     }
diff --git a/Python/pylifecycle.c b/Python/pylifecycle.c
@@ -1576,10 +1576,7 @@ finalize_interp_clear(PyThreadState *tstate)
     int is_main_interp = _Py_IsMainInterpreter(tstate);
 
     /* Clear interpreter state and all thread states */
-    PyInterpreterState_Clear(tstate->interp);
-
-    /* Last explicit GC collection */
-    _PyGC_CollectNoFail(tstate);
+    _PyInterpreterState_Clear(tstate);
 
     /* Clear all loghooks */
     /* Both _PySys_Audit function and users still need PyObject, such as tuple.
@@ -1588,8 +1585,6 @@ finalize_interp_clear(PyThreadState *tstate)
         _PySys_ClearAuditHooks(tstate);
     }
 
-    _PyGC_Fini(tstate);
-
     if (is_main_interp) {
         _Py_HashRandomization_Fini();
         _PyArg_Fini();
diff --git a/Python/pystate.c b/Python/pystate.c
@@ -268,14 +268,11 @@ PyInterpreterState_New(void)
 }
 
 
-void
-PyInterpreterState_Clear(PyInterpreterState *interp)
+static void
+interpreter_clear(PyInterpreterState *interp, PyThreadState *tstate)
 {
     _PyRuntimeState *runtime = interp->runtime;
 
-    /* Use the current Python thread state to call audit hooks,
-       not the current Python thread state of 'interp'. */
-    PyThreadState *tstate = _PyThreadState_GET();
     if (_PySys_Audit(tstate, "cpython.PyInterpreterState_Clear", NULL) < 0) {
         _PyErr_Clear(tstate);
     }
@@ -306,6 +303,12 @@ PyInterpreterState_Clear(PyInterpreterState *interp)
     if (_PyRuntimeState_GetFinalizing(runtime) == NULL) {
         _PyWarnings_Fini(interp);
     }
+
+    /* Last garbage collection on this interpreter */
+    _PyGC_CollectNoFail(tstate);
+
+    _PyGC_Fini(tstate);
+
     /* We don't clear sysdict and builtins until the end of this function.
        Because clearing other attributes can execute arbitrary Python code
        which requires sysdict and builtins. */
@@ -320,6 +323,25 @@ PyInterpreterState_Clear(PyInterpreterState *interp)
 }
 
 
+void
+PyInterpreterState_Clear(PyInterpreterState *interp)
+{
+    // Use the current Python thread state to call audit hooks and to collect
+    // garbage. It can be different than the current Python thread state
+    // of 'interp'.
+    PyThreadState *current_tstate = _PyThreadState_GET();
+
+    interpreter_clear(interp, current_tstate);
+}
+
+
+void
+_PyInterpreterState_Clear(PyThreadState *tstate)
+{
+    interpreter_clear(tstate->interp, tstate);
+}
+
+
 static void
 zapthreads(PyInterpreterState *interp, int check_current)
 {
