Skip to content
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-39511: PyThreadState_Clear() calls on_delete #18296

Merged
merged 1 commit into from
Feb 1, 2020
Merged

bpo-39511: PyThreadState_Clear() calls on_delete #18296

merged 1 commit into from
Feb 1, 2020

Conversation

vstinner
Copy link
Member

@vstinner vstinner commented Jan 31, 2020

PyThreadState.on_delete is a callback used to notify Python when a
thread completes. _thread._set_sentinel() function creates a lock
which is released when the thread completes. It sets on_delete
callback to the internal release_sentinel() function. This lock is
known as Threading._tstate_lock in the threading module.

The release_sentinel() function uses the Python C API. The problem is
that on_delete is called late in the Python finalization, when the C
API is no longer fully working.

The PyThreadState_Clear() function is now responsible to call
PyThreadState.on_delete callback. Previously, PyThreadState_Delete()
was responsible for that.

The release_sentinel() function is now called when the C API is still
fully working.

https://bugs.python.org/issue39511

@vstinner
Copy link
Member Author

Eric snow @ericsnowcurrently already faced this issue when trying to fix https://bugs.python.org/issue36854 with his PR #13219. I managed to work around this issue by clearing the Python thread state later: commit 9da7430.

But to implement https://bugs.python.org/issue39511 correctly (clear singletons at exit), I have to fix this issue.

@vstinner
Copy link
Member Author

I decided to not document the change in What's New in Python 3.9 since I don't think that PyThreadState_Clear() should be called directly. By the way, I'm not sure why it's documented at all nor why it's a public function. For me, it belongs more to the internal C API.

Copy link
Member

@ericsnowcurrently ericsnowcurrently left a comment

Choose a reason for hiding this comment

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

LGTM

I only have one suggestion on the wording of the docs.

Comment on lines 1052 to 1054
This function is now responsible to call
:c:member:`PyThreadState.on_delete` callback. Previously,
:c:func:`PyThreadState_Delete` was responsible for that.
Copy link
Member

Choose a reason for hiding this comment

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

This usage of "responsible" could be confusing. Consider instead:

   This function now calls the :c:member:`PyThreadState.on_delete`
   callback.  Previously, that happened in :c:func:`PyThreadState_Delete`.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, I updated the doc.

PyThreadState.on_delete is a callback used to notify Python when a
thread completes. _thread._set_sentinel() function creates a lock
which is released when the thread completes. It sets on_delete
callback to the internal release_sentinel() function. This lock is
known as Threading._tstate_lock in the threading module.

The release_sentinel() function uses the Python C API. The problem is
that on_delete is called late in the Python finalization, when the C
API is no longer fully working.

The PyThreadState_Clear() function now calls the
PyThreadState.on_delete callback. Previously, that happened in
PyThreadState_Delete().

The release_sentinel() function is now called when the C API is still
fully working.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants