Skip to content

bpo-42639: Move atexit state to PyInterpreterState #23763

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Dec 15, 2020
Merged

bpo-42639: Move atexit state to PyInterpreterState #23763

merged 1 commit into from
Dec 15, 2020

Conversation

vstinner
Copy link
Member

@vstinner vstinner commented Dec 14, 2020

  • Add _PyAtExit_Call() function and remove pyexitfunc and
    pyexitmodule members of PyInterpreterState. The function
    logs atexit callback errors using _PyErr_WriteUnraisableMsg().
  • Add _PyAtExit_Init() and _PyAtExit_Fini() functions.
  • Remove traverse, clear and free functions of the atexit module.
  • Remove _Py_PyAtExit() function.
  • test_atexit uses textwrap.dedent().
  • setup.py no longer tries to build atexit: it is always a built-in
    module.

Co-Authored-By: Dong-hee Na donghee.na@python.org

https://bugs.python.org/issue42639

@vstinner vstinner changed the title [WIP] bpo-40600: Move atexit state to PyInterpreterState [WIP] bpo-42639: Move atexit state to PyInterpreterState Dec 14, 2020
@vstinner
Copy link
Member Author

cc @corona10

@vstinner
Copy link
Member Author

I included multiple refactoring changes in this PR. I marked the PR as a draft. If the overall approach is accepted, I will merge this PR as multiple commits.

@corona10 corona10 requested review from encukou and corona10 December 14, 2020 12:42
Copy link
Member

@corona10 corona10 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am +1 on this PR than my PR.
I will take a look at this PR

@corona10
Copy link
Member

--- a/Modules/atexitmodule.c
+++ b/Modules/atexitmodule.c
@@ -10,6 +10,7 @@
 #include "pycore_initconfig.h"    // _PyStatus_NO_MEMORY
 #include "pycore_interp.h"        // PyInterpreterState.atexits
 #include "pycore_pystate.h"       // _PyInterpreterState_GET
+#include "pycore_pyerrors.h"


 static inline struct atexit_state*
@@ -93,10 +94,6 @@ atexit_callfuncs(struct atexit_state *state, int ignore_exc)

         PyObject *res = PyObject_Call(cb->func, cb->args, cb->kwargs);
         if (res == NULL) {
-            if (ignore_exc) {
-                _PyErr_WriteUnraisableMsg("in atexit callback", cb->func);
-            }
-            else {
                 /* Maintain the last exception, but don't leak if there are
                    multiple exceptions. */
                 if (exc_type) {
@@ -110,7 +107,6 @@ atexit_callfuncs(struct atexit_state *state, int ignore_exc)
                     PyErr_NormalizeException(&exc_type, &exc_value, &exc_tb);
                     PyErr_Display(exc_type, exc_value, exc_tb);
                 }
-            }
         }
         else {
             Py_DECREF(res);
@@ -135,6 +131,7 @@ _PyAtExit_Call(PyThreadState *tstate)
 {
     struct atexit_state *state = &tstate->interp->atexits;
     atexit_callfuncs(state, 1);
+    _PyErr_Clear(tstate);
 }

may fix the CI

@vstinner vstinner changed the title [WIP] bpo-42639: Move atexit state to PyInterpreterState bpo-42639: Move atexit state to PyInterpreterState Dec 14, 2020
@vstinner vstinner marked this pull request as ready for review December 14, 2020 22:08
@vstinner
Copy link
Member Author

I pushed two changes to prepare this PR (to make this PR shorter): https://bugs.python.org/issue42639

The PR is now ready for review.

* Add _PyAtExit_Call() function and remove pyexitfunc and
  pyexitmodule members of PyInterpreterState. The function
  logs atexit callback errors using _PyErr_WriteUnraisableMsg().
* Add _PyAtExit_Init() and _PyAtExit_Fini() functions.
* Remove traverse, clear and free functions of the atexit module.
* Remove _Py_PyAtExit() function.
* test_atexit uses textwrap.dedent().
* setup.py no longer tries to build atexit: it is always a built-in
  module.

Co-Authored-By: Dong-hee Na <donghee.na@python.org>
@vstinner
Copy link
Member Author

@corona10: Would you mind to review the updated (shorter) PR?

Copy link
Member

@shihai1991 shihai1991 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No other comments in here, LGTM.

Copy link
Member

@encukou encukou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Two nitpicks; one of them below.
While you're here, consider changing "Error in atexit._run_exitfuncs:\n" to "Error in atexit callback:\n" on line 111. _run_exitfuncs is private test-only API; the label in the _PyErr_WriteUnraisableMsg branch is nicer.

{
struct atexit_state *state = &interp->atexit;
atexit_cleanup(state);
PyMem_Free(state->callbacks);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be safer to set it to NULL after freeing.

@vstinner vstinner merged commit b8fa135 into python:master Dec 15, 2020
@vstinner vstinner deleted the atexit_state branch December 15, 2020 13:34
@vstinner
Copy link
Member Author

I merged my PR, thanks for the reviews!

@encukou: Oh, good idea. But I prefer to write a separated PR for that.

I wrote it locally. I started with PR #23777 which adds script_helper.run_test_script().

adorilson pushed a commit to adorilson/cpython that referenced this pull request Mar 13, 2021
* Add _PyAtExit_Call() function and remove pyexitfunc and
  pyexitmodule members of PyInterpreterState. The function
  logs atexit callback errors using _PyErr_WriteUnraisableMsg().
* Add _PyAtExit_Init() and _PyAtExit_Fini() functions.
* Remove traverse, clear and free functions of the atexit module.

Co-authored-by: Dong-hee Na <donghee.na@python.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants