bpo-37878: Make PyThreadState_DeleteCurrent() Internal#15315

nanjekyejoannah

Make PyThreadState_DeleteCurrent() Internal.

https://bugs.python.org/issue37878

nanjekyejoannah

cc @ericsnowcurrently

willingc

Thanks @nanjekyejoannah for the PR. I've made some suggested wording changes for reader clarity.

Co-Authored-By: Carol Willing <carolcode@willingconsulting.com>

nanjekyejoannah

@willingc Changes committed.

willingc

Let's give @ericsnowcurrently a chance to review too. Thanks. Please ping me if this is still open later this week and I will merge.

vstinner

I'm not sure that it's a good idea to document this function: https://bugs.python.org/issue37878#msg349954

bedevere-bot

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

nanjekyejoannah

@vstinner, I made the function internal instead. PTAL

I dismiss Carol's review since Joannah rewrote her PR, and Carol approved the previous version of the PR.

vstinner

Since it is a public function, even if it's not documented, please document the removal at:
https://docs.python.org/dev/whatsnew/3.9.html#removed

You can mention that the removed function was not documented.

For example, in Python 3.8, I wrote:

"The PyByteArray_Init() and PyByteArray_Fini() functions have been removed. They did nothing since Python 2.7.4 and Python 3.2.0, were excluded from the limited API (stable ABI), and were not documented. (Contributed by Victor Stinner in bpo-35713.)"

nanjekyejoannah

I have made the requested changes; please review again .

bedevere-bot

Thanks for making the requested changes!

@vstinner: please review the changes made to this pull request.

vstinner

LGTM.

vstinner

Thanks @nanjekyejoannah! It now looks better: remove the function rather than documenting it :-) I prefer to have a smaller C API!

) * Rename PyThreadState_DeleteCurrent() to _PyThreadState_DeleteCurrent() * Move it to the internal C API Co-Authored-By: Carol Willing <carolcode@willingconsulting.com>

tacaswell

This breaks pybind 11. Opened an issue with them (pybind/pybind11#1932). Where this should be fixed is a bit over my head at the moment.

vstinner

This breaks pybind 11. Opened an issue with them (pybind/pybind11#1932). Where this should be fixed is a bit over my head at the moment.

Your gil_scoped_acquire class implementation is scary! Would you mind to open a new issue at bugs.python.org to describe your use case and ask to revert the change which removed PyThreadState_DeleteCurrent?

wjakob

Pybind11 supports some advanced GIL-related tricks that require access this function. For instance, pybind11 can intercept a Python function call on the main() thread and delete the associated thread state, launching a new thread and continuing Python execution there (with a newly created thread state).

Kind of crazy, but why is this useful? UI libraries. On some platforms, it is not legal to poll UI events on any thread other than the main thread. This means that it's impossible to implement a proper UI event loop because Python is hogging the main thread. With the functionality in pybind11's gil_scoped_acquire, I can launch an event polling loop on the main thread, continue running an interactive Python session on another thread, and even swap them back e.g. when the user interface is no longer needed.

wjakob

@vstinner: I provided some feedback on the Python bugtracker and here.

wjakob

See https://bugs.python.org/issue38266.

) * Rename PyThreadState_DeleteCurrent() to _PyThreadState_DeleteCurrent() * Move it to the internal C API Co-Authored-By: Carol Willing <carolcode@willingconsulting.com>

henryiii

I'm confused by the attached issue; it looks like the decision was to simply revert the change with the function moved, and not add the underscore; and maybe even make it slightly easier to use. Now that 3.9b1 is out, PyBind11-based libraries cannot build, and the only fix would be to start using a now internal function in CPython. SciPy is an example of a PyBind11-using library.

vstinner

PyThreadState_DeleteCurrent() is declared by the public C API, but it's now excluded from the limited C API. Does pybind11 uses the limited C API? What is your issue? Please comment https://bugs.python.org/issue37878 instead of this merged PR (if you have a bugs.python.org account).

henryiii

I'm sorry, I'll add a bugs.python.org account or see if I had one in the past if I need to comment there. I realize, from looking through the code, that an underscore was not added, the function was moved intact (which was not clear from the PR), exactly as it was supposed to be in the issue. The only remaining problem is that the release notes here are wrong: https://docs.python.org/3.9/whatsnew/3.9.html#removed (which is a much better problem).

I'd love it if PyBind11 could use the limited API, but it can't.

Thank you, @vstinner !

vstinner

The only remaining problem is that the release notes here are wrong: https://docs.python.org/3.9/whatsnew/3.9.html#removed (which is a much better problem).

I created #20489 to update the What's New in Python 3.9 document. Would you mind to review my PR?

) * Rename PyThreadState_DeleteCurrent() to _PyThreadState_DeleteCurrent() * Move it to the internal C API Co-Authored-By: Carol Willing <carolcode@willingconsulting.com>

Document PyThreadState_DeleteCurrent()

35941ef

the-knights-who-say-ni added the CLA signed label Aug 16, 2019

bedevere-bot added docs Documentation in the Doc dir awaiting review labels Aug 16, 2019

📜🤖 Added by blurb_it.

d27549f

willingc reviewed Aug 17, 2019

View reviewed changes

nanjekyejoannah and others added 2 commits August 17, 2019 20:07

Update Doc/c-api/init.rst …

61ab0c3

Co-Authored-By: Carol Willing <carolcode@willingconsulting.com>

Update Doc/c-api/init.rst …

5034af8

Co-Authored-By: Carol Willing <carolcode@willingconsulting.com>

willingc previously approved these changes Aug 17, 2019

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting review labels Aug 17, 2019

vstinner requested changes Aug 19, 2019

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting merge labels Aug 19, 2019

make pyThreadState_DeleteCurrent internal

b4d0cce

nanjekyejoannah force-pushed the doc_1 branch from 6fd5f7c to b4d0cce Compare August 28, 2019 16:26

nanjekyejoannah changed the title ~~bpo-37878: Document PyThreadState_DeleteCurrent()~~ Aug 28, 2019

Update 2019-08-16-20-01-10.bpo-37878.MvA6rZ.rst

04677dd

vstinner requested changes Aug 29, 2019

View reviewed changes

vstinner reviewed Aug 29, 2019

View reviewed changes

nanjekyejoannah added 4 commits September 5, 2019 13:26

remove state from t_bootstrap()

a38b2b8

Remove update

19c7468

Merge branch 'master' of https://github.com/python/cpython into doc_1

7972bb4

resolve merge conflict

7acf1cb

bedevere-bot added awaiting change review and removed awaiting changes labels Sep 5, 2019

vstinner approved these changes Sep 5, 2019

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting change review labels Sep 5, 2019

vstinner merged commit 2bc43cd into python:master Sep 5, 2019

bedevere-bot removed the awaiting merge label Sep 5, 2019

tacaswell mentioned this pull request Sep 24, 2019

compilation fails with cpython master due to removed function pybind/pybind11#1932

Closed

bstaletic mentioned this pull request May 22, 2020

Pybind11 doesn't compile with CPython 3.9b1 pybind/pybind11#2224

Closed

vstinner mentioned this pull request May 28, 2020

bpo-37878: PyThreadState_DeleteCurrent() was not removed #20489

Merged

Conversation

nanjekyejoannah commented Aug 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nanjekyejoannah commented Aug 16, 2019

Uh oh!

willingc left a comment

Choose a reason for hiding this comment

Uh oh!

nanjekyejoannah commented Aug 17, 2019

Uh oh!

willingc commented Aug 17, 2019

Uh oh!

vstinner left a comment

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented Aug 19, 2019

Uh oh!

nanjekyejoannah commented Aug 28, 2019

Uh oh!

vstinner left a comment

Choose a reason for hiding this comment

Uh oh!

nanjekyejoannah commented Sep 5, 2019

Uh oh!

bedevere-bot commented Sep 5, 2019

Uh oh!

vstinner left a comment

Choose a reason for hiding this comment

Uh oh!

vstinner commented Sep 5, 2019

Uh oh!

tacaswell commented Sep 24, 2019

Uh oh!

vstinner commented Sep 24, 2019

Uh oh!

wjakob commented Sep 24, 2019

Uh oh!

wjakob commented Sep 24, 2019

Uh oh!

wjakob commented Sep 24, 2019

Uh oh!

henryiii commented May 26, 2020

Uh oh!

vstinner commented May 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

henryiii commented May 28, 2020

Uh oh!

vstinner commented May 28, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

nanjekyejoannah commented Aug 16, 2019 •

edited

Loading

vstinner commented May 26, 2020 •

edited

Loading