bpo-36475: Finalize PyEval_AcquireLock() and PyEval_AcquireThread() properly by nanjekyejoannah · Pull Request #12667 · python/cpython · GitHub

I have added changes to finalize finalize PyEval_AcquireLock() and PyEval_AcquireThread() properly.

https://bugs.python.org/issue36475

LGTM, but I'd like Eric to take a look as well. In particular, I'm not sure if we have any existing test cases for this behaviour, and whether it might make sense to add one to the embedding tests.

@pablogsal @pitrou: Would you mind to review this change? It seems like you know the modified functions :-)

I would recommend adding a note in the doc for these functions as we did in fde9b33 .

Also, this code appears 3 times now in ceval.c so we could refactor this into a macro/inline function.

Hmm... this will exit any thread during finalization, not just daemon threads? I'm not sure why this is a good idea.

I have made the requested changes; please review again.

Thanks for making the requested changes!

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

@pitrou PyEval_RestoreThread, and hence the Py_END_ALLOW_THREADS macro and the GILState APIs, already cause all threads to exit if they find the interpreter is already finalizing, but the interpreter joins all of its own non-daemon threads before it marks itself as finalizing:

cpython/Python/pylifecycle.c

Line 1150 in 0ef8c15

Py_FinalizeEx(void)

This PR just makes the lower level GIL acquisition APIs behave the same way PyEval_RestoreThread already does so that surviving daemon threads will exit rather than risking deadlocking based on exactly which API they call.

This does highlight that the change is worth mentioning in the porting section of the Python 3.8 What's New, though, along with "versionchanged" notes on the affected APIs.

I see. I'm always wary of the potential issues with such low-level changes, but you're right that we're already half way to the behaviour established by the PR, so I guess it's fine.

I guess it's ok on the principle. Please see comments below.

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.

The core functionality of the change still looks good to me, just some refactoring suggestions for the code, and a couple more documentation tweaks.

My proposed wording for the NEWS entry can be re-used as a bullet point in the What's New porting section, with an extra sentence on what to do about it:

PyEval_AcquireLock and PyEval_AcquireThread now terminate the current thread if called while the interpreter is finalizing, making them consistent with PyEval_RestoreThread, Py_END_ALLOW_THREADS, and PyGILState_Ensure. If this behaviour is not desired, guard the call by checking _Py_IsFinalizing or sys.is_finalizing.

@vstinner Are you aware of any particular reason for _Py_IsFinalizing being a nominally private API? If we're recommending that folks call it in the documentation, that's a pretty strong sign that it should really be public.

(That can be broken out to a separate issue, though - it doesn't need to be resolved here)

I don't know. Feel free to open an issue to propose to make it public if you want.

In my PR #12934, I modified the internal _Py_CURRENTLY_FINALIZING() macro to add a runtime parameter :-) (PR currently under review.)

I have made the requested changes; please review again

Thanks for making the requested changes!

@pitrou, @ncoghlan: please review the changes made to this pull request.

FYI I wrote PR #12453 "Destroy the GIL at exit".

This looks good to me now (noting the follow-up PR in #12679 to switch the thread cleanup to be per-interpreter rather than per-runtime)

Ended up with two copies of this note.

… into issue36475

I have made the requested changes; please review again

Thanks for making the requested changes!

@pitrou, @ncoghlan: please review the changes made to this pull request.

Joannah addressed Antoine's comments.

I am very scared by this change touching critical parts of Python, but I'm somehow confident that it's the right thing to do.

Antoine: Py_Finalize() explicitly waits until all non-daemon threads complete, so this change should impact "regular" threads: only daemon threads.

Note: I have no idea of the impact on performances, but IMHO correctness matters more than performance here. If there is an overhead, I expect it to be not significant on macro benchmarks, and low on micro-benchmarks.

--

Side note. I really hate the whole concept of daemon threads. Kill a thread anytime is just a very high risk of inconsistency. The thread can be killed while writing in the middle of a file, it can leave temporary files, etc. I would prefer to remove the concept of daemon threads and require developers to call pthread_kill() to make them understand what they are doing (something stupid IMHO :-)). Daemon threads make Python finalization very fragile, but the GIL + this PR make the finalization less fragile. Hint: search for "daemon" in Google Image. IMHO the name is well chosen.

I am not comfortable to backport this change to Python 3.7. It's too early to know how it will impact applications and how many complains we will get :-) If someone really wants to backport this scary change to 3.7, I would suggest to wait for 1 month after Python 3.8.0 final release.

the-knights-who-say-ni added the CLA signed label Apr 2, 2019

bedevere-bot added the awaiting review label Apr 2, 2019

ncoghlan requested a review from ericsnowcurrently April 7, 2019 07:38

ncoghlan approved these changes Apr 7, 2019

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting review labels Apr 7, 2019

ZackerySpytz reviewed Apr 22, 2019

View reviewed changes

Comment thread Misc/NEWS.d/next/Core and Builtins/2019-04-02-20-02-22.bpo-36475.CjRps3.rst Outdated

vstinner requested review from pablogsal and pitrou April 24, 2019 13:27

vstinner reviewed Apr 24, 2019

View reviewed changes

Comment thread Misc/NEWS.d/next/Core and Builtins/2019-04-02-20-02-22.bpo-36475.CjRps3.rst Outdated

bedevere-bot added awaiting change review and removed awaiting merge labels Apr 24, 2019

pitrou previously requested changes Apr 25, 2019

View reviewed changes

Comment thread Python/ceval.c Outdated

bedevere-bot added awaiting changes and removed awaiting change review labels Apr 25, 2019

ncoghlan requested changes Apr 25, 2019

View reviewed changes

nanjekyejoannah mentioned this pull request Apr 25, 2019

bpo-36479: Exit threads when interpreter is finalizing rather than runtime #12679

Closed

Finalize PyEval_AcquireLock() and PyEval_AcquireThread() properly

4f6b32d

nanjekyejoannah force-pushed the issue36475 branch from fe8a3aa to 4f6b32d Compare April 25, 2019 21:49

nanjekyejoannah added 2 commits April 25, 2019 18:02

Finalize PyEval_AcquireLock() and PyEval_AcquireThread() properly

2c02944

space fix

b8a1690

Remove new line.

f2337be

bedevere-bot added awaiting change review and removed awaiting changes labels Apr 25, 2019

vstinner mentioned this pull request Apr 26, 2019

bpo-36356: Destroy the GIL at exit #12453

Merged

vstinner reviewed Apr 26, 2019

View reviewed changes

Comment thread Python/ceval.c Outdated

ncoghlan approved these changes Apr 26, 2019

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting change review labels Apr 26, 2019

nanjekyejoannah added 3 commits April 26, 2019 13:52

Finalize PyEval_AcquireLock() and PyEval_AcquireThread() properly

5daac23

Merge branch 'issue36475' of https://github.com/nanjekyejoannah/cpython …

d14cedb

… into issue36475

Finalize PyEval_AcquireLock() and PyEval_AcquireThread() properly

4e1d820

bedevere-bot added awaiting change review and removed awaiting merge labels Apr 26, 2019

vstinner reviewed Apr 26, 2019

View reviewed changes

Comment thread Python/ceval.c

vstinner merged commit f781d20 into python:master Apr 29, 2019

bedevere-bot removed the awaiting change review label Apr 29, 2019

nanjekyejoannah deleted the issue36475 branch April 29, 2019 16:46

vstinner mentioned this pull request May 2, 2019

bpo-36475: Make PyThread_exit_thread with _Py_NO_RETURN #13068

Merged

ericsnowcurrently mentioned this pull request Sep 23, 2023

PyThreadState_Swap() During Finalization Causes Immediate Exit (AKA Daemon Threads Are Still the Worst!) #109793

Closed

                  :c:func:`PyEval_RestoreThread` is a higher-level function which is always
                  available (even when threads have not been initialized).
+                 .. note::

bpo-36475: Finalize PyEval_AcquireLock() and PyEval_AcquireThread() properly by nanjekyejoannah · Pull Request #12667 · python/cpython · GitHub

Uh oh!

Uh oh!

bpo-36475: Finalize PyEval_AcquireLock() and PyEval_AcquireThread() properly - #12667

bpo-36475: Finalize PyEval_AcquireLock() and PyEval_AcquireThread() properly#12667vstinner merged 7 commits intopython:masterpython/cpython:masterfrom nanjekyejoannah:issue36475nanjekyejoannah/cpython:issue36475Copy head branch name to clipboard

Conversation

nanjekyejoannah commented Apr 2, 2019 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nanjekyejoannah commented Apr 2, 2019

Uh oh!

ncoghlan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

vstinner commented Apr 24, 2019

Uh oh!

pablogsal commented Apr 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

pitrou commented Apr 24, 2019

Uh oh!

nanjekyejoannah commented Apr 24, 2019

Uh oh!

bedevere-bot commented Apr 24, 2019

Uh oh!

ncoghlan commented Apr 25, 2019

Uh oh!

pitrou commented Apr 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pitrou left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bedevere-bot commented Apr 25, 2019

Uh oh!

ncoghlan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ncoghlan Apr 25, 2019

Choose a reason for hiding this comment

Uh oh!

vstinner Apr 25, 2019

Choose a reason for hiding this comment

Uh oh!

nanjekyejoannah commented Apr 25, 2019

Uh oh!

bedevere-bot commented Apr 25, 2019

Uh oh!

vstinner commented Apr 26, 2019

Uh oh!

Uh oh!

ncoghlan left a comment

Choose a reason for hiding this comment

Uh oh!

ncoghlan Apr 26, 2019

Choose a reason for hiding this comment

Uh oh!

nanjekyejoannah commented Apr 26, 2019

Uh oh!

bedevere-bot commented Apr 26, 2019

Uh oh!

Uh oh!

vstinner commented Apr 29, 2019

Uh oh!

vstinner commented Apr 29, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

bpo-36475: Finalize PyEval_AcquireLock() and PyEval_AcquireThread() properly#12667
vstinner merged 7 commits into
python:masterfrom
nanjekyejoannah:issue36475

nanjekyejoannah commented Apr 2, 2019 •
edited by bedevere-bot
Loading

pablogsal commented Apr 24, 2019 •
edited
Loading

pitrou commented Apr 25, 2019 •
edited
Loading