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

@@ -1025,7 +1025,7 @@ t_bootstrap(void *boot_raw)
    PyMem_DEL(boot_raw);
    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 resolved Hide resolved

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

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

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()~~ bpo-37878: Make PyThreadState_DeleteCurrent() Internal 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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bpo-37878: Make PyThreadState_DeleteCurrent() Internal #15315

bpo-37878: Make PyThreadState_DeleteCurrent() Internal #15315

nanjekyejoannah commented Aug 16, 2019 •

edited

Loading

nanjekyejoannah commented Aug 16, 2019

willingc left a comment

nanjekyejoannah commented Aug 17, 2019

willingc commented Aug 17, 2019

vstinner left a comment

bedevere-bot commented Aug 19, 2019

nanjekyejoannah commented Aug 28, 2019

vstinner Aug 29, 2019

vstinner left a comment

nanjekyejoannah commented Sep 5, 2019

bedevere-bot commented Sep 5, 2019

vstinner left a comment

vstinner commented Sep 5, 2019

tacaswell commented Sep 24, 2019

vstinner commented Sep 24, 2019

wjakob commented Sep 24, 2019

wjakob commented Sep 24, 2019

wjakob commented Sep 24, 2019

henryiii commented May 26, 2020

vstinner commented May 26, 2020 •

edited

Loading

henryiii commented May 28, 2020

vstinner commented May 28, 2020

bpo-37878: Make PyThreadState_DeleteCurrent() Internal #15315

bpo-37878: Make PyThreadState_DeleteCurrent() Internal #15315

Conversation

nanjekyejoannah commented Aug 16, 2019 • edited Loading

nanjekyejoannah commented Aug 16, 2019

willingc left a comment

Choose a reason for hiding this comment

nanjekyejoannah commented Aug 17, 2019

willingc commented Aug 17, 2019

vstinner left a comment

Choose a reason for hiding this comment

bedevere-bot commented Aug 19, 2019

nanjekyejoannah commented Aug 28, 2019

vstinner Aug 29, 2019

Choose a reason for hiding this comment

vstinner left a comment

Choose a reason for hiding this comment

nanjekyejoannah commented Sep 5, 2019

bedevere-bot commented Sep 5, 2019

vstinner left a comment

Choose a reason for hiding this comment

vstinner commented Sep 5, 2019

tacaswell commented Sep 24, 2019

vstinner commented Sep 24, 2019

wjakob commented Sep 24, 2019

wjakob commented Sep 24, 2019

wjakob commented Sep 24, 2019

henryiii commented May 26, 2020

vstinner commented May 26, 2020 • edited Loading

henryiii commented May 28, 2020

vstinner commented May 28, 2020

nanjekyejoannah commented Aug 16, 2019 •

edited

Loading

vstinner commented May 26, 2020 •

edited

Loading