Skip to content

Test atexit shutdown mechanism in a subprocess#4828

Merged
pitrou merged 3 commits intopython:masterfrom
pitrou:atexit_check_subprocess
Dec 13, 2017
Merged

Test atexit shutdown mechanism in a subprocess#4828
pitrou merged 3 commits intopython:masterfrom
pitrou:atexit_check_subprocess

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
/* For the atexit module. */
void _Py_PyAtExit(void (*func)(void))
{
/* Guard against API misuse */
Copy link
Member

@vstinner vstinner Dec 13, 2017

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

@pitrou pitrou Dec 13, 2017

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

vstinner commented Dec 13, 2017

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

miss-islington commented Dec 13, 2017

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

miss-islington commented Dec 13, 2017

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

bedevere-bot commented Dec 13, 2017

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