bpo-37878: Make PyThreadState_DeleteCurrent() Internal#15315

nanjekyejoannah · 2019-08-16T20:00:05Z

Make PyThreadState_DeleteCurrent() Internal.

https://bugs.python.org/issue37878

nanjekyejoannah · 2019-08-16T20:03:33Z

cc @ericsnowcurrently

willingc

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

Doc/c-api/init.rst

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

nanjekyejoannah · 2019-08-17T23:09:29Z

@willingc Changes committed.

willingc · 2019-08-17T23:23:48Z

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 · 2019-08-19T10:40:27Z

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 · 2019-08-28T16:53:46Z

@vstinner, I made the function internal instead. PTAL

vstinner · 2019-08-29T12:58:59Z

Modules/_threadmodule.c

 tstate->interp->num_threads--;
 PyThreadState_Clear(tstate);
-PyThreadState_DeleteCurrent();
+_PyThreadState_DeleteCurrent(&_PyRuntime);


Please add "_PyRuntimeState *runtime = &_PyRuntime;" at the beginning of the function, and replace &_PyRuntime with runtime.
Or maybe even better: add the field to "struct bootstate" and fill the field in thread_PyThread_start_new_thread(). It would make t_bootstrap() more "state-less" (don't access directly global variables).

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 · 2019-09-05T15:28:34Z

I have made the requested changes; please review again .

bedevere-bot · 2019-09-05T15:28:37Z

Thanks for making the requested changes!

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

vstinner

LGTM.

vstinner · 2019-09-05T16:07:20Z

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 · 2019-09-24T11:52:03Z

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 · 2019-09-24T12:15:20Z

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 · 2019-09-24T12:18:08Z

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 · 2019-09-24T12:21:00Z

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

wjakob · 2019-09-24T19:22:17Z

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 · 2020-05-26T02:30:27Z

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 · 2020-05-26T16:27:46Z

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 · 2020-05-28T03:06:28Z

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 · 2020-05-28T14:34:53Z

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

Doc/c-api/init.rst Outdated Show resolvedHide resolved
Doc/c-api/init.rst Outdated Show resolvedHide resolved
Doc/c-api/init.rst Outdated Show resolvedHide resolved

nanjekyejoannahand 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 b4d0cceCompare August 28, 2019 16:26

nanjekyejoannah changed the title ~~bpo-37878: Document PyThreadState_DeleteCurrent()~~bpo-37878: Make PyThreadState_DeleteCurrent() InternalAug 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' ofhttps://github.com/python/cpythoninto 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:masterSep 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

Uh oh!

bpo-37878: Make PyThreadState_DeleteCurrent() Internal#15315

bpo-37878: Make PyThreadState_DeleteCurrent() Internal #15315

Uh oh!

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!

Uh oh!

Uh oh!

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!

vstinnerAug 29, 2019

Choose a reason for hiding this comment

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