classification
Title: Revert bpo-37878: Make PyThreadState_DeleteCurrent() Internal
Type: behavior Stage: resolved
Components: Interpreter Core Versions: Python 3.9
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: eric.snow, lukasz.langa, nanjekyejoannah, ncoghlan, ned.deily, tcaswell, vstinner, wenzel
Priority: Keywords: patch

Created on 2019-09-24 19:20 by wenzel, last changed 2019-10-05 09:57 by vstinner. This issue is now closed.

Pull Requests
URL Status Linked Edit
PR 16558 merged nanjekyejoannah, 2019-10-03 12:52
Messages (16)
msg353114 - (view) Author: Wenzel Jakob (wenzel) Date: 2019-09-24 19:20
A commit from a few days ago and discussed in issue #37878 removed an undocumented function PyThreadState_DeleteCurrent() from Python's public API.

This function was exposed for good reasons and simply removing it because it is undocumented strikes me as a somewhat brutal solution to achieve documentation coverage. This function is useful -- even essential -- to delete the current thread's thread state, which cannot be done using the still-public PyThreadState_Delete(). The documentation could simply be a line stating this explicitly.

I was asked to provide an example of an actual usage: this function is used by pybind11, which is a widely used library for creating Python bindings to C++ code. pybind11 only calls PyThreadState_DeleteCurrent() in a rare set of circumstances, but there is no alternative in this case. Specifically, 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, so why is this useful? The answer is 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 asynchronous UI event loop because Python is hogging the main thread. With the functionality in pybind11's gil_scoped_acquire, it is possible can launch an event polling loop on the main thread, continue running an interactive Python REPL on another thread, and even swap them back e.g. when the user interface is no longer needed.

Best,
Wenzel
msg353117 - (view) Author: Joannah Nanjekye (nanjekyejoannah) * (Python committer) Date: 2019-09-24 20:10
I am okay with the revert but @victor may still have some reservations on this.
msg353361 - (view) Author: Eric Snow (eric.snow) * (Python committer) Date: 2019-09-27 14:06
FWIW, I support reverting the removal of PyThreadState_DeleteCurrent().  We only reverted under the assumption that no one was using this function.  Clearly we were mistaken. :)

I'll re-open #37878 to revive the discussion about documenting the function (which we still may or may not want to do).
msg353362 - (view) Author: Eric Snow (eric.snow) * (Python committer) Date: 2019-09-27 14:10
FWIW, when we un-revert we should be sure to move PyThreadState_DeleteCurrent() from Include/pystate.h to Include/cpython/pystate.h.  (I suppose that could be done in a separate PR to keep the git history clear.)
msg353363 - (view) Author: Eric Snow (eric.snow) * (Python committer) Date: 2019-09-27 14:11
s/un-revert/revert/
msg353364 - (view) Author: Joannah Nanjekye (nanjekyejoannah) * (Python committer) Date: 2019-09-27 14:14
+1 for moving from Include/pystate.h to Include/cpython/pystate.h
msg353365 - (view) Author: Joannah Nanjekye (nanjekyejoannah) * (Python committer) Date: 2019-09-27 14:15
I will handle this later today.
msg353366 - (view) Author: Eric Snow (eric.snow) * (Python committer) Date: 2019-09-27 14:16
I *was* going to suggest that we also put an underscore prefix on the name, but I couldn't think of a reason why we would want to discourage use (other than to reduce the size of the [supported] public API).  Moving it to Include/cpython/pystate.h is probably enough.
msg353367 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2019-09-27 14:16
If we want to make the function "fully public", I suggest to write documentation and have a simple unit test.
msg353368 - (view) Author: Eric Snow (eric.snow) * (Python committer) Date: 2019-09-27 14:16
@Joannah, sounds good.  Thanks!
msg353868 - (view) Author: Ned Deily (ned.deily) * (Python committer) Date: 2019-10-03 18:10
Since this sounds like a regression being introduced by 3.8.0, should the reversion be included in 3.8.0 final or can it wait for 3.8.1?
msg353877 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2019-10-03 21:02
> Since this sounds like a regression being introduced by 3.8.0, should the reversion be included in 3.8.0 final or can it wait for 3.8.1?

IMHO PR 16558 is safe: it adds code which already existed in Python 2.7, and likely previously. For me it's a low risk of regression between rc1 and final.

I set the priority to release blocker, but Lukasz is the last one to take the final decision on including the fix or not.

Well, right now, I don't consider that PR 16558 is ready to be merged.
msg353922 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2019-10-04 11:35
New changeset 8855e47d09d4db1206c65b24efc8ad0df585ac46 by Victor Stinner (Joannah Nanjekye) in branch 'master':
bpo-38266: Revert bpo-37878: Make PyThreadState_DeleteCurrent() Internal (GH-16558)
https://github.com/python/cpython/commit/8855e47d09d4db1206c65b24efc8ad0df585ac46
msg353924 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2019-10-04 11:40
> Since this sounds like a regression being introduced by 3.8.0, should the reversion be included in 3.8.0 final or can it wait for 3.8.1?

Oh, in fact, the change was only made in the master branch: after the 3.8 branch was created. The 3.8 branch still contains PyThreadState_DeleteCurrent() ;-) There is no regression in 3.8, only in master, and the regression in master has been fixed.
msg353975 - (view) Author: Wenzel Jakob (wenzel) Date: 2019-10-04 22:18
This is great -- thank you for handling this so quickly.
msg354000 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2019-10-05 09:57
> This is great -- thank you for handling this so quickly.

Thanks for testing the master branch and reporting regressions quickly as well ;-)
History
Date User Action Args
2019-10-05 09:57:50vstinnersetmessages: + msg354000
2019-10-04 22:18:26wenzelsetmessages: + msg353975
2019-10-04 12:56:47ned.deilysetkeywords: - 3.8regression
versions: - Python 3.8
2019-10-04 11:40:02vstinnersetstatus: open -> closed
priority: release blocker ->
messages: + msg353924

resolution: fixed
stage: patch review -> resolved
2019-10-04 11:35:53vstinnersetmessages: + msg353922
2019-10-03 21:02:12vstinnersetpriority: normal -> release blocker

messages: + msg353877
2019-10-03 18:10:36ned.deilysetkeywords: + 3.8regression
nosy: + lukasz.langa, ned.deily
messages: + msg353868

2019-10-03 12:52:18nanjekyejoannahsetkeywords: + patch
stage: patch review
pull_requests: + pull_request16148
2019-09-27 14:16:36eric.snowsetmessages: + msg353368
2019-09-27 14:16:33vstinnersetmessages: + msg353367
2019-09-27 14:16:05eric.snowsetmessages: + msg353366
2019-09-27 14:15:16nanjekyejoannahsetmessages: + msg353365
2019-09-27 14:14:30nanjekyejoannahsetmessages: + msg353364
2019-09-27 14:11:44eric.snowsetmessages: + msg353363
2019-09-27 14:10:19eric.snowsetmessages: + msg353362
2019-09-27 14:06:51eric.snowsetmessages: + msg353361
2019-09-24 20:10:03nanjekyejoannahsetmessages: + msg353117
2019-09-24 19:20:41wenzelcreate