GV

GeezerVison

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

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — bpo-44422: threading.Thread reuses the _delete() method by vstinner · Pull Request #26741 · python/cpython

Conversation

@vstinner

@vstinner vstinner commented Jun 15, 2021 •
edited by bedevere-bot
Loading

Uh oh!

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

Copy link
Copy Markdown
Member

The _bootstrap_inner() method of threading.Thread now reuses its
_delete() method rather than accessing _active() directly. It became
possible since _active_limbo_lock became reentrant. Moreover, it no
longer ignores any exception when deleting the thread from the
_active dictionary.

https://bugs.python.org/issue44422

Sorry, something went wrong.

Uh oh!

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

All reactions
@vstinner
bpo-44422: threading.Thread reuses the _delete() method
d32b7fc
The _bootstrap_inner() method of threading.Thread now reuses its
_delete() method rather than accessing _active() directly. It became
possible since _active_limbo_lock became reentrant. Moreover, it no
longer ignores any exception when deleting the thread from the
_active dictionary.
@the-knights-who-say-ni the-knights-who-say-ni added the CLA signed label Jun 15, 2021
@bedevere-bot bedevere-bot added the awaiting core review label Jun 15, 2021
@vstinner

vstinner commented Jun 15, 2021

Copy link
Copy Markdown
Member Author

History:

  • (B) commit 95cd5c0 replaces the _delete() call with inlined code (modify directly _active dictionary)
    - Fix Issue #1703448: A joined thread could show up in the
      threading.enumerate() list after the join() for a brief period until
      it actually exited.
  • (A) commit f21b2aa added try: ... except: pass
    Thread.__bootstrap(): ignore exceptions in the self.__delete() call in
    the finally clause.  An exception here could happen when a daemon
    thread exits after the threading module has already been trashed by
    the import finalization, and there's not much of a point in trying to
    insist doing the cleanup in that stage.
    
    This should fix SF bug ##497111: active_limbo_lock error at program
    exit.
    
    2.1.2 and 2.2.1 Bugfix candidate!
All reactions

Sorry, something went wrong.

Uh oh!

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

@vstinner

vstinner commented Jun 15, 2021

Copy link
Copy Markdown
Member Author

(B) commit 95cd5c0 replaces the _delete() call with inlined code (modify directly _active dictionary)

This is https://bugs.python.org/issue1703448

The fix adds test_enumerate_after_join() to Lib/test/test_threading.py which still exists. Running test_threading should be enough to check for non-regression.

(A) commit f21b2aa added try: ... except: pass

This is https://bugs.python.org/issue497111

All reactions

Sorry, something went wrong.

Uh oh!

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

@vstinner vstinner added the skip news label Jun 15, 2021
@vstinner

vstinner commented Jun 15, 2021

Copy link
Copy Markdown
Member Author

cc @iritkatriel

All reactions

Sorry, something went wrong.

Uh oh!

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

@vstinner vstinner mentioned this pull request Jun 15, 2021
bpo-44422: Fix threading.enumerate() reentrant call #26727
Merged
@iritkatriel

iritkatriel commented Jun 15, 2021

Copy link
Copy Markdown
Member

How did you conclude that the try-except is no longer needed?

All reactions

Sorry, something went wrong.

Uh oh!

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

erlend-aasland
erlend-aasland approved these changes Jun 15, 2021
View reviewed changes
@vstinner

vstinner commented Jun 15, 2021

Copy link
Copy Markdown
Member Author

How did you conclude that the try-except is no longer needed?

except: pass ignores any exception. At least, we should only catch KeyError.

I don't understand in which case an exception can occur. In case of doubt, I prefer to remove it. We can add the try/except KeyError again if someone hits the issue.

https://bugs.python.org/issue497111 was reported 20 years ago. The Python code base changed a lot in 20 years.

For example, I modified the Python finalization to clear daemon threads earlier: commit 9ad58ac.

All reactions

Sorry, something went wrong.

Uh oh!

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

@vstinner vstinner added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Jun 15, 2021
@bedevere-bot

bedevere-bot commented Jun 15, 2021

Copy link
Copy Markdown

🤖 New build scheduled with the buildbot fleet by @vstinner for commit d32b7fc 🤖

If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again.

All reactions

Sorry, something went wrong.

Uh oh!

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

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Jun 15, 2021
iritkatriel
iritkatriel approved these changes Jun 15, 2021
View reviewed changes

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

Makes sense.

Sorry, something went wrong.

Uh oh!

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

All reactions
@bedevere-bot bedevere-bot added awaiting merge and removed awaiting core review labels Jun 15, 2021
@vstinner
vstinner merged commit 0729694 into python:main Jun 16, 2021
@bedevere-bot bedevere-bot removed the awaiting merge label Jun 16, 2021
@vstinner
vstinner deleted the thread_delete branch June 16, 2021 09:43
@vstinner

vstinner commented Jun 16, 2021

Copy link
Copy Markdown
Member Author

@iritkatriel: Thanks for the review. I merged my PR.

All reactions

Sorry, something went wrong.

Uh oh!

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

jdevries3133 pushed a commit to jdevries3133/cpython that referenced this pull request Jun 19, 2021
@vstinner @jdevries3133
bpo-44422: threading.Thread reuses the _delete() method (pythonGH-26741)
8fc6dd3
The _bootstrap_inner() method of threading.Thread now reuses its
_delete() method rather than accessing _active() directly. It became
possible since _active_limbo_lock became reentrant. Moreover, it no
longer ignores any exception when deleting the thread from the
_active dictionary.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@iritkatriel iritkatriel iritkatriel approved these changes
@erlend-aasland erlend-aasland erlend-aasland approved these changes

Uh oh!

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

Assignees

No one assigned

Labels

skip news

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.

5 participants

@vstinner @iritkatriel @bedevere-bot @erlend-aasland @the-knights-who-say-ni
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.