Revert bpo-37878: Make PyThreadState_DeleteCurrent() Internal

[wenzel]

BPO	38266
Nosy	@ncoghlan, @vstinner, @ned-deily, @ambv, @ericsnowcurrently, @tacaswell, @nanjekyejoannah
PRs	bpo-38266: Revert bpo-37878: Make PyThreadState_DeleteCurrent() Internal #16558

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = None
closed_at = <Date 2019-10-04.11:40:02.568>
created_at = <Date 2019-09-24.19:20:41.677>
labels = ['interpreter-core', 'type-bug', '3.9']
title = 'Revert bpo-37878: Make PyThreadState_DeleteCurrent() Internal'
updated_at = <Date 2019-10-05.09:57:50.838>
user = 'https://bugs.python.org/wenzel'

bugs.python.org fields:

activity = <Date 2019-10-05.09:57:50.838>
actor = 'vstinner'
assignee = 'none'
closed = True
closed_date = <Date 2019-10-04.11:40:02.568>
closer = 'vstinner'
components = ['Interpreter Core']
creation = <Date 2019-09-24.19:20:41.677>
creator = 'wenzel'
dependencies = []
files = []
hgrepos = []
issue_num = 38266
keywords = ['patch']
message_count = 16.0
messages = ['353114', '353117', '353361', '353362', '353363', '353364', '353365', '353366', '353367', '353368', '353868', '353877', '353922', '353924', '353975', '354000']
nosy_count = 8.0
nosy_names = ['ncoghlan', 'vstinner', 'ned.deily', 'lukasz.langa', 'eric.snow', 'tcaswell', 'wenzel', 'nanjekyejoannah']
pr_nums = ['16558']
priority = None
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue38266'
versions = ['Python 3.9']

[wenzel]

A commit from a few days ago and discussed in issue bpo-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

[nanjekyejoannah]

I am okay with the revert but @victor may still have some reservations on this.

[ericsnowcurrently]

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 bpo-37878 to revive the discussion about documenting the function (which we still may or may not want to do).

[ericsnowcurrently]

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.)

[ericsnowcurrently]

s/un-revert/revert/

[nanjekyejoannah]

+1 for moving from Include/pystate.h to Include/cpython/pystate.h

[nanjekyejoannah]

I will handle this later today.

[ericsnowcurrently]

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.

[vstinner]

If we want to make the function "fully public", I suggest to write documentation and have a simple unit test.

[ericsnowcurrently]

@joannah, sounds good. Thanks!

[ned-deily]

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?

[vstinner]

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.

[vstinner]

New changeset 8855e47 by Victor Stinner (Joannah Nanjekye) in branch 'master':
bpo-38266: Revert bpo-37878: Make PyThreadState_DeleteCurrent() Internal (GH-16558)
8855e47

[vstinner]

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.

[wenzel]

This is great -- thank you for handling this so quickly.

[vstinner]

This is great -- thank you for handling this so quickly.

Thanks for testing the master branch and reporting regressions quickly as well ;-)