Skip to content

bpo-44422: threading.Thread reuses the _delete() method #26741

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jun 16, 2021
Merged

bpo-44422: threading.Thread reuses the _delete() method #26741

merged 1 commit into from
Jun 16, 2021

Conversation

vstinner
Copy link
Member

@vstinner vstinner commented Jun 15, 2021

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

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.
@vstinner
Copy link
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!

@vstinner
Copy link
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

@vstinner
Copy link
Member Author

cc @iritkatriel

@iritkatriel
Copy link
Member

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

@vstinner
Copy link
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.

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

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

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Jun 15, 2021
Copy link
Member

@iritkatriel iritkatriel left a comment

Choose a reason for hiding this comment

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

Makes sense.

@vstinner vstinner merged commit 0729694 into python:main Jun 16, 2021
@vstinner vstinner deleted the thread_delete branch June 16, 2021 09:43
@vstinner
Copy link
Member Author

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

jdevries3133 pushed a commit to jdevries3133/cpython that referenced this pull request Jun 19, 2021
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants