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.

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

bpo-33612: Remove PyThreadState_Clear() assertion #7069

Merged
merged 1 commit into from May 23, 2018

Conversation

Projects
None yet
6 participants
@vstinner
Member

vstinner commented May 23, 2018

bpo-25612, bpo-33612: Remove an assertion from PyThreadState_Clear()
which failed at Python shutdown or on fork if a thread was running a
generator.

https://bugs.python.org/issue33612

bpo-33612: Remove PyThreadState_Clear() assertion
bpo-25612, bpo-33612: Remove an assertion from PyThreadState_Clear()
which failed at Python shutdown or on fork if a thread was running a
generator.
@markshannon

This comment has been minimized.

Show comment
Hide comment
@markshannon

markshannon May 23, 2018

Contributor

LGTM.
The tests tstate->exc_info->previous_item == NULL and tstate->exc_info == &tstate->exc_state are equivalent, so the assert is redundant if correct and needlessly causes a crash when wrong.

Contributor

markshannon commented May 23, 2018

LGTM.
The tests tstate->exc_info->previous_item == NULL and tstate->exc_info == &tstate->exc_state are equivalent, so the assert is redundant if correct and needlessly causes a crash when wrong.

@serhiy-storchaka

I agree, it is safe to clear just tstate->exc_state and remove the assertion.

@vstinner

This comment has been minimized.

Show comment
Hide comment
@vstinner

vstinner May 23, 2018

Member

I agree, it is safe to clear just tstate->exc_state and remove the assertion.

Wait, do you mean that exc_state and/or exc_info must be explicitly cleared? These fields are pointers to data hold by a generator. The best that we can do is to set these pointers to NULL, but it is very likely the the whole tstate memory is going to be freed anyway, so I'm not sure that it's useful.

I'm not sure that I understood you correctly. Do you think that the current PR is correct? Or do we need extra changes?

Member

vstinner commented May 23, 2018

I agree, it is safe to clear just tstate->exc_state and remove the assertion.

Wait, do you mean that exc_state and/or exc_info must be explicitly cleared? These fields are pointers to data hold by a generator. The best that we can do is to set these pointers to NULL, but it is very likely the the whole tstate memory is going to be freed anyway, so I'm not sure that it's useful.

I'm not sure that I understood you correctly. Do you think that the current PR is correct? Or do we need extra changes?

@serhiy-storchaka

This comment has been minimized.

Show comment
Hide comment
@serhiy-storchaka

serhiy-storchaka May 23, 2018

Member

I think that the current PR is correct.

exc_state is cleared few lines above, exc_info is a link to the state owned by the generator, and doesn't need any action. But setting it to &exc_state wouldn't make the code worse.

Member

serhiy-storchaka commented May 23, 2018

I think that the current PR is correct.

exc_state is cleared few lines above, exc_info is a link to the state owned by the generator, and doesn't need any action. But setting it to &exc_state wouldn't make the code worse.

@vstinner vstinner merged commit b6dccf5 into python:master May 23, 2018

9 checks passed

VSTS: Linux-PR Linux-PR_20180523.18 succeeded
Details
VSTS: Linux-PR-Coverage Linux-PR-Coverage_20180523.20 succeeded
Details
VSTS: Windows-PR Windows-PR_20180523.18 succeeded
Details
VSTS: docs docs_20180523.21 succeeded
Details
VSTS: macOS-PR macOS-PR_20180523.18 succeeded
Details
bedevere/issue-number Issue number 33612 found
Details
bedevere/news "skip news" label found
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@bedevere-bot

This comment has been minimized.

Show comment
Hide comment
@bedevere-bot

bedevere-bot May 23, 2018

@vstinner: Please replace # with GH- in the commit message next time. Thanks!

bedevere-bot commented May 23, 2018

@vstinner: Please replace # with GH- in the commit message next time. Thanks!

@miss-islington

This comment has been minimized.

Show comment
Hide comment
@miss-islington

miss-islington May 23, 2018

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

miss-islington commented May 23, 2018

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

@vstinner vstinner deleted the vstinner:thread_clear branch May 23, 2018

@bedevere-bot

This comment has been minimized.

Show comment
Hide comment
@bedevere-bot

bedevere-bot May 23, 2018

GH-7074 is a backport of this pull request to the 3.7 branch.

bedevere-bot commented May 23, 2018

GH-7074 is a backport of this pull request to the 3.7 branch.

miss-islington added a commit to miss-islington/cpython that referenced this pull request May 23, 2018

bpo-33612: Remove PyThreadState_Clear() assertion (pythonGH-7069)
bpo-25612, bpo-33612: Remove an assertion from PyThreadState_Clear()
which failed at Python shutdown or on fork if a thread was running a
generator.
(cherry picked from commit b6dccf5)

Co-authored-by: Victor Stinner <vstinner@redhat.com>
@vstinner

This comment has been minimized.

Show comment
Hide comment
@vstinner

vstinner May 23, 2018

Member

exc_state is cleared few lines above

exc_state fields are cleared, not exc_state itself.

exc_info is a link to the state owned by the generator, and doesn't need any action. But setting it to &exc_state wouldn't make the code worse.

I don't feel confortable to make this change. Please write a separated PR if you want to do that.

Member

vstinner commented May 23, 2018

exc_state is cleared few lines above

exc_state fields are cleared, not exc_state itself.

exc_info is a link to the state owned by the generator, and doesn't need any action. But setting it to &exc_state wouldn't make the code worse.

I don't feel confortable to make this change. Please write a separated PR if you want to do that.

ned-deily added a commit that referenced this pull request May 23, 2018

bpo-33612: Remove PyThreadState_Clear() assertion (GH-7069) (GH-7074)
bpo-25612, bpo-33612: Remove an assertion from PyThreadState_Clear()
which failed at Python shutdown or on fork if a thread was running a
generator.
(cherry picked from commit b6dccf5)

Co-authored-by: Victor Stinner <vstinner@redhat.com>

lisroach pushed a commit to lisroach/cpython that referenced this pull request Sep 10, 2018

bpo-33612: Remove PyThreadState_Clear() assertion (python#7069)
bpo-25612, bpo-33612: Remove an assertion from PyThreadState_Clear()
which failed at Python shutdown or on fork if a thread was running a
generator.

dacut added a commit to dacut/cpython that referenced this pull request Sep 16, 2018

bpo-33612: Remove PyThreadState_Clear() assertion (python#7069)
bpo-25612, bpo-33612: Remove an assertion from PyThreadState_Clear()
which failed at Python shutdown or on fork if a thread was running a
generator.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment