Skip to content

Conversation

@ghost
Copy link

@ghostghost commented Nov 28, 2017

This PR moves the atexit callbacks from Python runtime context to interpreter state.
Also, Py_EndInterpreter() now calls atexit callbacks of the interpreter.

These changes make it possible to implement PEP-489 multiphase initialization for atexit module, which is also done in this PR.

https://bugs.python.org/issue31901

Copy link
Member

Choose a reason for hiding this comment

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

A nit, but it would seem to me more conventional to take the user data (the PyObject *) as second argument.

Copy link
Member

Choose a reason for hiding this comment

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

Is it actually necessary to use C code for this? As opposed to calling support.run_in_subinterp from test_atexit (as already done in two other tests).

Copy link
Author

Choose a reason for hiding this comment

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

Awesome, didn't know about this thing, it is rewritten in Python now.

@pitrou
Copy link
Member

Thank you for the PR! I posted a couple comments.

@pitrou
Copy link
Member

There was a crash on AppVeyor. I've retried the bulld in case it's an unrelated sporadic issue (?).

@pitrou
Copy link
Member

Looks like the AppVeyor crash isn't sporadic after all (and your PR is the only one to trigger it). Do you have a Windows machine or VM to test?

@pitrou
Copy link
Member

By the way, analyzing this PR led me to #4826 :-)

@ghost
Copy link
Author

ghost commented Dec 13, 2017

Unfortunately, I don't have any Windows machine.
About #4826, the _Py_PyAtExit should really not be called from anywhere else than atexit module, otherwise, some sort of list instead of single pointer would be needed. Should I have a look at that as a part of this PR?

@pitrou
Copy link
Member

PR #4826 was merged, so you just have to rebase/merge this PR on git master (you will have to resolve conflicts, which should be relatively easy here).

@ghost
Copy link
Author

And about the Windows crash?

@pitrou
Copy link
Member

If it persists after git master is merged in, I'll try to reproduce on my Windows VM (if I manage to unstuck it).

@pitrou
Copy link
Member

Ok, I think I've found the culprit for the Windows crash: now that you're using PEP 489 multiphase init support, modstate can be NULL in m_traverse and m_clear (and probably m_free too), so you have to guard against this.

@pitrou
Copy link
Member

Note I don't know exactly why that happens, so I asked at https://mail.python.org/pipermail/python-dev/2017-December/151238.html

@ghost
Copy link
Author

@pitrou this PR is ready for review

@pitrou
Copy link
Member

This looks good to me. I'm gonna merge soon if no other core developer chimes in.

@encukou
Copy link
Member

The documentation issue is tracked in bpo-32374.

Sign up for freeto 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.

4 participants

@pitrou@encukou@the-knights-who-say-ni@bedevere-bot