Skip to content

bpo-42639: Move atexit state to PyInterpreterState#23763

Merged
vstinner merged 1 commit intopython:masterfrom
vstinner:atexit_state
Dec 15, 2020
Merged

bpo-42639: Move atexit state to PyInterpreterState#23763
vstinner merged 1 commit intopython:masterfrom
vstinner:atexit_state

Conversation

@vstinner
Copy link
Copy Markdown
Member

@vstinner vstinner commented Dec 14, 2020 •
edited
Loading

  • 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
Copy Markdown
Member Author

vstinner commented Dec 14, 2020

cc @corona10

@vstinner
Copy link
Copy Markdown
Member Author

vstinner commented Dec 14, 2020

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 corona10 and encukou December 14, 2020 12:42
corona10
corona10 reviewed Dec 14, 2020
View reviewed changes
Copy link
Copy Markdown
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
Copy Markdown
Member

corona10 commented Dec 14, 2020 

--- 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
Copy Markdown
Member Author

vstinner commented Dec 14, 2020

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.

@corona10 @vstinner
bpo-42639: Move atexit state to PyInterpreterState
553a14d 
* 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
Copy Markdown
Member Author

vstinner commented Dec 14, 2020

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

shihai1991
shihai1991 approved these changes Dec 15, 2020
View reviewed changes
Copy link
Copy Markdown
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.

encukou
encukou reviewed Dec 15, 2020
View reviewed changes
Copy link
Copy Markdown
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.

Modules/atexitmodule.c
{
struct atexit_state *state = &interp->atexit;
atexit_cleanup(state);
PyMem_Free(state->callbacks);
Copy link
Copy Markdown
Member

@encukou encukou Dec 15, 2020

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
Copy Markdown
Member Author

vstinner commented Dec 15, 2020

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
@vstinner @corona10 @adorilson
bpo-42639: Move atexit state to PyInterpreterState (pythonGH-23763)
44d7e67 
* 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

Reviewers

@encukou encukou encukou left review comments

@corona10 corona10 corona10 approved these changes

@shihai1991 shihai1991 shihai1991 approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@vstinner @corona10 @encukou @shihai1991 @the-knights-who-say-ni @bedevere-bot