◐ Shell
reader mode source ↗
Skip to content

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

Merged
vstinner merged 1 commit into
python:masterfrom
vstinner:daemon_thread_runtime2
Mar 8, 2020
Merged

bpo-39877: Fix PyEval_RestoreThread() for daemon threads#18811
vstinner merged 1 commit into
python:masterfrom
vstinner:daemon_thread_runtime2

Conversation

@vstinner

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

Copy link
Copy Markdown
Member
  • 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 vstinner mentioned this pull request Mar 6, 2020
Closed
@vstinner

vstinner commented Mar 6, 2020

Copy link
Copy Markdown
Member Author

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

vstinner commented Mar 6, 2020

Copy link
Copy Markdown
Member Author

@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
pitrou requested changes Mar 6, 2020
View reviewed changes

@pitrou pitrou left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hide comment

Looks good on the principle, some comments below.

@bedevere-bot

bedevere-bot commented Mar 6, 2020

Copy link
Copy Markdown

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

@vstinner

vstinner commented Mar 6, 2020

Copy link
Copy Markdown
Member Author

I have made the requested changes; please review again.

@bedevere-bot

bedevere-bot commented Mar 6, 2020

Copy link
Copy Markdown

Thanks for making the requested changes!

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

@bedevere-bot bedevere-bot requested a review from pitrou March 6, 2020 18:31
@vstinner vstinner mentioned this pull request Mar 6, 2020
Merged
@vstinner

vstinner commented Mar 6, 2020

Copy link
Copy Markdown
Member Author

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

@vstinner
bpo-39877: Fix PyEval_RestoreThread() for daemon threads …
dbc3dca 
* 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

vstinner commented Mar 6, 2020

Copy link
Copy Markdown
Member Author

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

vstinner commented Mar 6, 2020

Copy link
Copy Markdown
Member Author

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

@vstinner vstinner merged commit eb4e2ae into python:master Mar 8, 2020
@vstinner vstinner deleted the daemon_thread_runtime2 branch March 8, 2020 10:57
@vstinner

vstinner commented Mar 8, 2020

Copy link
Copy Markdown
Member Author

Thanks for the review @python/buildbot-owners

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

@vstinner

vstinner commented Mar 8, 2020

Copy link
Copy Markdown
Member Author

Thanks for the review @python/buildbot-owners

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

sthagen added a commit to sthagen/python-cpython that referenced this pull request Mar 8, 2020
@sthagen
Merge pull request #313 from python/master …
febde1a 
bpo-39877: Fix PyEval_RestoreThread() for daemon threads (pythonGH-18811)
40 hidden items Load more…
@ericsnowcurrently ericsnowcurrently mentioned this pull request Sep 23, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@pitrou pitrou pitrou approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@vstinner @bedevere-bot @pitrou @the-knights-who-say-ni