GeezerVison

cc @ericsnowcurrently , @ncoghlan .

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

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.

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.

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

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.

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.