bpo-39877: Fix PyEval_RestoreThread() for daemon threads #18811

vstinner · 2020-03-06T17:31:14Z

exit_thread_if_finalizing() does now access directly _PyRuntime
variable, rather than using tstate->interp->runtime since tstate
can be a dangling pointer after Py_Finalize() has been called.
exit_thread_if_finalizing() is now called before calling
take_gil(). There is no need to protect the call with the GIL.
Add ENSURE_TSTATE_NOT_NULL() macro to check that tstate is not NULL
at runtime. Check tstate earlier. take_gil() is now responsible to
check that tstate is not NULL.

Cleanup:

PyEval_RestoreThread() no longer saves/restores errno: it's already
done inside take_gil().
PyEval_AcquireLock(), PyEval_AcquireThread(),
PyEval_RestoreThread() and _PyEval_EvalFrameDefault() now check if
tstate is valid with the new is_tstate_valid() function which uses
_PyMem_IsPtrFreed().

https://bugs.python.org/issue39877

vstinner · 2020-03-06T17:34:52Z

Without this change, after Py_Finalize() has been called, daemon threads are not exited by exit_thread_if_finalizing(): exit_thread_if_finalizing() is not called. Instead, daemon threads loop on attempting to take the GIL :-(

With this PR, daemon threads exit before attempting to take the GIL. Moreover, the PR better validates tstate which can help to detect misusage of modified functions of the C API like PyEval_RestoreThread().

vstinner · 2020-03-06T17:47:55Z

@ericsnowcurrently @nanjekyejoannah @pablogsal @pitrou @methane @serhiy-storchaka: Would you mind to review this PR? In short, this PR fix the code to exit daemon threads after Py_Finalize() has been called. Currently, it's broken in multiple different ways. But it worked somehow before my latest refactoring (see https://bugs.python.org/issue39877 for the cause of the regression).

Main changes:

exit_thread_if_finalizing() is now called before take_gil(), instead of being called after
exit_thread_if_finalizing() now access directly _PyRuntime rather than dereferencing tstate to get runtime (tstate->interp->runtime)

I chose to include "cleanup" changes in the PR: harden checks on tstate. I prefer to change all code at once. It should help to detect future bugs.

pitrou

Looks good on the principle, some comments below.

Misc/NEWS.d/next/Core and Builtins/2020-03-06-18-30-00.bpo-39877.bzd1y0.rst

pitrou · 2020-03-06T17:50:17Z

Python/ceval.c

+{
+    assert(!_PyMem_IsPtrFreed(tstate));
+    assert(!_PyMem_IsPtrFreed(tstate->interp));
+    return 1;


Why does this function always return 1?

It's to be able to use it with assert(...). I like to use assert(), since it's removed in release mode. I'm using this pattern with _PyObject_CheckConsistency() and similar _PyXXX_CheckConsistency() functions.

If an assertion fails, the function displays the expression which is false with the expected line number. That's why the function does use assert() directly rather than returning a value.

Python/ceval.c

pitrou · 2020-03-06T17:56:47Z

Python/ceval.c

+       tstate->interp->runtime to support calls from Python daemon threads.
+       After Py_Finalize() has been called, tstate can be a dangling pointer:
+       point to PyThreadState freed memory. */
+    _PyRuntimeState *runtime = &_PyRuntime;
    if (runtime->finalizing != NULL && !_Py_CURRENTLY_FINALIZING(runtime, tstate)) {


Since you're now accessing this without the GIL, should this field be manipulated only with atomic ops?

I don't think that it's worth it. If you consider that it should be done: can it be done in a separated PR after this one is merged? Does atomic operations provide any additional warranty compared to regular variables?

If you're not using an atomic variable, you may be getting a partially written value (it's very unlikely for a pointer-sized value, but theoretically possible). In other words, the code here is probably incorrect (ThreadSanitizer may help detect this).

Ok, I see. I wrote PR #18816 to use an atomic variable. I would prefer to first merge PR #18816, and then this PR.

Misc/NEWS.d/next/Core and Builtins/2020-03-06-18-30-00.bpo-39877.bzd1y0.rst

bedevere-bot · 2020-03-06T17:59:55Z

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

vstinner · 2020-03-06T18:31:29Z

I have made the requested changes; please review again.

bedevere-bot · 2020-03-06T18:31:32Z

Thanks for making the requested changes!

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

vstinner · 2020-03-06T23:28:46Z

I rebased the PR on top the just merged commit 7b3c252 (GH-18816): _PyRuntime.finalizing is now an atomic variable.

* exit_thread_if_finalizing() does now access directly _PyRuntime variable, rather than using tstate->interp->runtime since tstate can be a dangling pointer after Py_Finalize() has been called. * exit_thread_if_finalizing() is now called *before* calling take_gil(). There is no need to protect the call with the GIL. * Add ensure_tstate_not_null() function to check that tstate is not NULL at runtime. Check tstate earlier. take_gil() is now responsible to check that tstate is not NULL. Cleanup: * PyEval_RestoreThread() no longer saves/restores errno: it's already done inside take_gil(). * PyEval_AcquireLock(), PyEval_AcquireThread(), PyEval_RestoreThread() and _PyEval_EvalFrameDefault() now check if tstate is valid with the new is_tstate_valid() function which uses _PyMem_IsPtrFreed().

vstinner · 2020-03-06T23:57:12Z

I rebased the PR on top of commit 9e5d30c (GH-18819) so ensure_tstate_not_null() now logs the correct function name in the Fatal Python error.

vstinner · 2020-03-06T23:58:20Z

@pitrou: _PyRuntime.finalizing is now atomic. Would you mind to review the updated PR?

vstinner · 2020-03-08T10:58:11Z

Thanks for the review @python/buildbot-owners

Daemon threads should be a little bit more reliable during Python finalization with this change.

vstinner · 2020-03-08T11:19:04Z

Thanks for the review @python/buildbot-owners

Oops, I wanted to write: Thanks for the review @pitrou! (Firefox picked the wrong completion, sorry.)

bpo-39877: Fix PyEval_RestoreThread() for daemon threads (pythonGH-18811)

the-knights-who-say-ni added the CLA signed label Mar 6, 2020

bedevere-bot added the awaiting core review label Mar 6, 2020

vstinner mentioned this pull request Mar 6, 2020

bpo-39877: Fix PyEval_RestoreThread() for daemon threads #18808

Closed

pitrou requested changes Mar 6, 2020

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting core review labels Mar 6, 2020

bedevere-bot added awaiting change review and removed awaiting changes labels Mar 6, 2020

bedevere-bot requested a review from pitrou March 6, 2020 18:31

vstinner mentioned this pull request Mar 6, 2020

bpo-39877: _PyRuntimeState.finalizing becomes atomic #18816

Merged

pitrou approved these changes Mar 7, 2020

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting change review labels Mar 7, 2020

vstinner merged commit eb4e2ae into python:master Mar 8, 2020

bedevere-bot removed the awaiting merge label Mar 8, 2020

vstinner deleted the daemon_thread_runtime2 branch March 8, 2020 10:57

sthagen added a commit to sthagen/python-cpython that referenced this pull request Mar 8, 2020

Merge pull request #313 from python/master

febde1a

bpo-39877: Fix PyEval_RestoreThread() for daemon threads (pythonGH-18811)

ericsnowcurrently mentioned this pull request Sep 23, 2023

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

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bpo-39877: Fix PyEval_RestoreThread() for daemon threads #18811

bpo-39877: Fix PyEval_RestoreThread() for daemon threads #18811

vstinner commented Mar 6, 2020 •

edited by bedevere-bot

Loading

vstinner commented Mar 6, 2020

vstinner commented Mar 6, 2020

pitrou left a comment

pitrou Mar 6, 2020

vstinner Mar 6, 2020

vstinner Mar 6, 2020

pitrou Mar 6, 2020

vstinner Mar 6, 2020

pitrou Mar 6, 2020

vstinner Mar 6, 2020

bedevere-bot commented Mar 6, 2020

vstinner commented Mar 6, 2020

bedevere-bot commented Mar 6, 2020

vstinner commented Mar 6, 2020

vstinner commented Mar 6, 2020

vstinner commented Mar 6, 2020

vstinner commented Mar 8, 2020

vstinner commented Mar 8, 2020

bpo-39877: Fix PyEval_RestoreThread() for daemon threads #18811

bpo-39877: Fix PyEval_RestoreThread() for daemon threads #18811

Conversation

vstinner commented Mar 6, 2020 • edited by bedevere-bot Loading

vstinner commented Mar 6, 2020

vstinner commented Mar 6, 2020

pitrou left a comment

Choose a reason for hiding this comment

pitrou Mar 6, 2020

Choose a reason for hiding this comment

vstinner Mar 6, 2020

Choose a reason for hiding this comment

vstinner Mar 6, 2020

Choose a reason for hiding this comment

pitrou Mar 6, 2020

Choose a reason for hiding this comment

vstinner Mar 6, 2020

Choose a reason for hiding this comment

pitrou Mar 6, 2020

Choose a reason for hiding this comment

vstinner Mar 6, 2020

Choose a reason for hiding this comment

bedevere-bot commented Mar 6, 2020

vstinner commented Mar 6, 2020

bedevere-bot commented Mar 6, 2020

vstinner commented Mar 6, 2020

vstinner commented Mar 6, 2020

vstinner commented Mar 6, 2020

vstinner commented Mar 8, 2020

vstinner commented Mar 8, 2020

vstinner commented Mar 6, 2020 •

edited by bedevere-bot

Loading