Skip to content

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 <vstinner@redhat.com>
@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 <vstinner@redhat.com>
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

6 participants