GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — bpo-39877: Fix PyEval_RestoreThread() for daemon threads by vstinner · Pull Request #18811 · python/cpython

Conversation

vstinner commented Mar 6, 2020 •

edited by bedevere-bot

Loading

Uh oh!

There was an error while loading. Please reload this page.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

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

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

pitrou requested changes

Mar 6, 2020

View reviewed changes

pitrou left a comment

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Looks good on the principle, some comments below.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

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

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

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

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

Python/ceval.c Outdated

pitrou Mar 6, 2020

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Why does this function always return 1?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

vstinner Mar 6, 2020

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

vstinner Mar 6, 2020

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

Python/ceval.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

Python/ceval.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

Python/ceval.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

Python/ceval.c Outdated

pitrou Mar 6, 2020

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

vstinner Mar 6, 2020

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

pitrou Mar 6, 2020

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

vstinner Mar 6, 2020

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

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

Uh oh!

There was an error while loading. Please reload this page.

bedevere-bot added awaiting changes and removed awaiting core review labels

Mar 6, 2020

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

vstinner commented Mar 6, 2020

Copy link

Copy Markdown

Member Author

I have made the requested changes; please review again.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

bedevere-bot added awaiting change review and removed awaiting changes labels

Mar 6, 2020

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

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

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.


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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

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?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

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

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

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

Reviewers

pitrou

pitrou approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

4 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

Conversation

vstinner commented Mar 6, 2020 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vstinner commented Mar 6, 2020

Uh oh!

vstinner commented Mar 6, 2020

Uh oh!

pitrou left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bedevere-bot commented Mar 6, 2020

Uh oh!

vstinner commented Mar 6, 2020

Uh oh!

bedevere-bot commented Mar 6, 2020

Uh oh!

vstinner commented Mar 6, 2020

Uh oh!

vstinner commented Mar 6, 2020

Uh oh!

vstinner commented Mar 6, 2020

Uh oh!

vstinner commented Mar 8, 2020

Uh oh!

vstinner commented Mar 8, 2020

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

vstinner commented Mar 6, 2020 •

edited by bedevere-bot

Loading

Uh oh!

There was an error while loading. Please reload this page.