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

[nanjekyejoannah]

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

https://bugs.python.org/issue36475

[nanjekyejoannah]

cc @ericsnowcurrently , @ncoghlan .

[ncoghlan]

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.

[vstinner]

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

[pablogsal]

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.

[pitrou]

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

[nanjekyejoannah]

I have made the requested changes; please review again.

[bedevere-bot]

Thanks for making the requested changes!

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

[ncoghlan]

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

[pitrou]

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.

[pitrou]

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

[bedevere-bot]

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.

[ncoghlan]

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.

[ncoghlan]

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

[vstinner]

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

[nanjekyejoannah]

I have made the requested changes; please review again

[bedevere-bot]

Thanks for making the requested changes!

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

[vstinner]

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

[ncoghlan]

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)

[ncoghlan]

Ended up with two copies of this note.

[nanjekyejoannah]

I have made the requested changes; please review again

[bedevere-bot]

Thanks for making the requested changes!

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

Joannah addressed Antoine's comments.

[vstinner]

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.

[vstinner]

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.