Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Test atexit shutdown mechanism in a subprocess #4828

Merged
merged 3 commits into from
Dec 13, 2017
Merged

Test atexit shutdown mechanism in a subprocess #4828

merged 3 commits into from
Dec 13, 2017

Conversation

pitrou
Copy link
Member

@pitrou pitrou commented Dec 13, 2017

Needs #4826 to work.

@pitrou
Copy link
Member Author

pitrou commented Dec 13, 2017

@vstinner

@vstinner vstinner mentioned this pull request Dec 13, 2017
Merged
vstinner
vstinner reviewed Dec 13, 2017
View reviewed changes
Python/pylifecycle.c Outdated
@@ -2086,6 +2086,8 @@ _Py_FatalInitError(_PyInitError err)
/* For the atexit module. */
void _Py_PyAtExit(void (*func)(void))
{
/* Guard against API misuse */
Copy link
Member

Choose a reason for hiding this comment

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

Maybe add "bpo-17852: " prefix. Otherwise, the purpose of the assertion is not obvious.

Or explain that calling this function to register a new callback replaces the previous callback, which is unlikely to be the expected behaviour.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, adding a bpo references sounds good to me.

vstinner
vstinner reviewed Dec 13, 2017
View reviewed changes
Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

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

test_atexit test LGTM, I just have a question/comment on the added assertion.

I merged your PR #4826, so you can rebase this one on top of it.

@vstinner
Copy link
Member

At least, the assertion worked as expected before you rebased this PR on top of PR #4826, Travis CI failed with:

python: Python/pylifecycle.c:2090: void _Py_PyAtExit(void (*)(void)): Assertion `_PyRuntime.pyexitfunc == ((void*)0) || _PyRuntime.pyexitfunc == func' failed.

vstinner
vstinner approved these changes Dec 13, 2017
View reviewed changes
Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

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

LGTM, thanks.

@pitrou pitrou merged commit fc5db95 into python:master Dec 13, 2017
@miss-islington
Copy link
Contributor

Thanks @pitrou for the PR 🌮🎉.. I'm working now to backport this PR to: 3.6.
🐍🍒⛏🤖

@pitrou pitrou deleted the atexit_check_subprocess branch December 13, 2017 01:29
@miss-islington
Copy link
Contributor

Sorry, @pitrou, I could not cleanly backport this to 3.6 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker fc5db95e0063eafa2bfb7f487fcaad5a7c4b65a1 3.6

pitrou added a commit to pitrou/cpython that referenced this pull request Dec 13, 2017
@pitrou
[3.6] Test atexit shutdown mechanism in a subprocess (pythonGH-4828)
245a646 
* Test atexit shutdown mechanism in a subprocess.
(cherry picked from commit fc5db95)
@bedevere-bot
Copy link

GH-4829 is a backport of this pull request to the 3.6 branch.

pitrou added a commit that referenced this pull request Dec 13, 2017
@pitrou
[3.6] Test atexit shutdown mechanism in a subprocess (GH-4828) (#4829)
b6263ca 
* Test atexit shutdown mechanism in a subprocess.
(cherry picked from commit fc5db95)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vstinner vstinner vstinner approved these changes

Assignees
No one assigned
Labels
skip issue skip news
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@pitrou @vstinner @miss-islington @bedevere-bot @the-knights-who-say-ni