Skip to content

bpo-33612: Remove PyThreadState_Clear() assertion #7069

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

Merged
merged 1 commit into from
May 23, 2018
Merged

bpo-33612: Remove PyThreadState_Clear() assertion #7069

merged 1 commit into from
May 23, 2018

Conversation

vstinner
Copy link
Member

@vstinner 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-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
Copy link
Member

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.

Copy link
Member

@serhiy-storchaka serhiy-storchaka left a comment

Choose a reason for hiding this comment

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

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

@serhiy-storchaka serhiy-storchaka added type-bug An unexpected behavior, bug, or error needs backport to 3.7 labels May 23, 2018
@vstinner
Copy link
Member Author

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

@vstinner vstinner merged commit b6dccf5 into python:master May 23, 2018
@bedevere-bot
Copy link

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

@miss-islington
Copy link
Contributor

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

@vstinner vstinner deleted the thread_clear branch May 23, 2018 16:01
@bedevere-bot
Copy link

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

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request 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.
(cherry picked from commit b6dccf5)

Co-authored-by: Victor Stinner <[email protected]>
@vstinner
Copy link
Member Author

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 pushed a commit that referenced this pull request 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.
(cherry picked from commit b6dccf5)

Co-authored-by: Victor Stinner <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
skip news type-bug An unexpected behavior, bug, or error
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants