Skip to content

Test atexit shutdown mechanism in a subprocess - #4828

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

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

Conversation

@pitrou

@pitroupitrou commented Dec 13, 2017

Copy link
Copy Markdown
Member

Needs #4826 to work.

@pitrou

pitrou commented Dec 13, 2017

Copy link
Copy Markdown
MemberAuthor

@vstinner

@vstinnervstinner mentioned this pull request Dec 13, 2017
Merged
vstinner
vstinner reviewed Dec 13, 2017
View reviewed changes
Comment threadPython/pylifecycle.c Outdated
/* For the atexit module. */
void _Py_PyAtExit(void (*func)(void))
{
/* Guard against API misuse */

@vstinnervstinnerDec 13, 2017

Copy link
Copy Markdown
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.

@pitroupitrouDec 13, 2017

Copy link
Copy Markdown
MemberAuthor

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

@vstinnervstinner left a comment

Copy link
Copy Markdown
Member

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

vstinner commented Dec 13, 2017

Copy link
Copy Markdown
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

@vstinnervstinner left a comment

Copy link
Copy Markdown
Member

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:masterDec 13, 2017
@miss-islington

miss-islington commented Dec 13, 2017

Copy link
Copy Markdown
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

miss-islington commented Dec 13, 2017

Copy link
Copy Markdown
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

bedevere-bot commented Dec 13, 2017

Copy link
Copy Markdown

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 freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vstinnervstinnervstinner approved these changes

Assignees

No one assigned

Labels

skip issueskip 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