◐ Shell
reader mode source ↗
Skip to content

bpo-33612: Remove PyThreadState_Clear() assertion#7069

Merged
vstinner merged 1 commit into
python:masterfrom
vstinner:thread_clear
May 23, 2018
Merged

bpo-33612: Remove PyThreadState_Clear() assertion#7069
vstinner merged 1 commit into
python:masterfrom
vstinner:thread_clear

Conversation

@vstinner

@vstinner vstinner commented May 23, 2018 •
edited by bedevere-bot
Loading

Copy link
Copy Markdown
Member

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

@vstinner
bpo-33612: Remove PyThreadState_Clear() assertion …
af7a081 
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

markshannon commented May 23, 2018

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

serhiy-storchaka
serhiy-storchaka approved these changes May 23, 2018
View reviewed changes

@serhiy-storchaka serhiy-storchaka 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.

Hide comment

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

vstinner commented May 23, 2018

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

serhiy-storchaka commented May 23, 2018

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

bedevere-bot commented May 23, 2018

Copy link
Copy Markdown

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

@miss-islington

miss-islington commented May 23, 2018

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

bedevere-bot commented May 23, 2018

Copy link
Copy Markdown

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
@vstinner @miss-islington
bpo-33612: Remove PyThreadState_Clear() assertion (pythonGH-7069) …
33d1e8c 
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

vstinner commented May 23, 2018

Copy link
Copy Markdown
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
@miss-islington @vstinner @ned-deily
bpo-33612: Remove PyThreadState_Clear() assertion (GH-7069) (GH-7074) …
508d769 
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

Reviewers

@serhiy-storchaka serhiy-storchaka serhiy-storchaka approved these changes

@markshannon markshannon markshannon approved these changes

Assignees

No one assigned

Labels

skip news type-bug An unexpected behavior, bug, or error

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@vstinner @markshannon @serhiy-storchaka @bedevere-bot @miss-islington @the-knights-who-say-ni