Skip to content
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.

Sign up for GitHub

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

Jump to bottom

gh-104341: Add a Separate "Running" Lock for Each Thread #104754

Merged
merged 4 commits into from
May 23, 2023
Merged

gh-104341: Add a Separate "Running" Lock for Each Thread #104754

merged 4 commits into from
May 23, 2023

Conversation

ericsnowcurrently
Copy link
Member

@ericsnowcurrently ericsnowcurrently commented May 22, 2023 •
edited
Loading

Having a separate lock means Thread.join() doesn't need to wait for the thread to be cleaned up first. It can wait for the thread's Python target to finish running. This gives us some flexibility in how we clean up threads.

(This is a minor cleanup as part of a fix for gh-104341.)

@bedevere-bot bedevere-bot mentioned this pull request May 22, 2023
Closed
@ericsnowcurrently ericsnowcurrently added the needs backport to 3.12 bug and security fixes label May 23, 2023
@ericsnowcurrently ericsnowcurrently merged commit 097b783 into python:main May 23, 2023
@miss-islington
Copy link
Contributor

Thanks @ericsnowcurrently for the PR 🌮🎉.. I'm working now to backport this PR to: 3.12.
🐍🍒⛏🤖

@ericsnowcurrently ericsnowcurrently deleted the threading-running-lock branch May 23, 2023 20:29
@bedevere-bot
Copy link

GH-104817 is a backport of this pull request to the 3.12 branch.

@bedevere-bot bedevere-bot removed the needs backport to 3.12 bug and security fixes label May 23, 2023
@terryjreedy
Copy link
Member

This merge is likely the cause of multiple Address sanitizer CI failures afterwards. See the core discord, core-workflow-and-bots. For me, threading_cleanup left 2 dangling thread 4 times, once in test_importlib, making an Environment Change failure in 3 of 4 runs.

@ericsnowcurrently ericsnowcurrently restored the threading-running-lock branch May 24, 2023 04:40
@ericsnowcurrently
Copy link
Member Author

I'll take a look first thing in the morning.

@gpshead
Copy link
Member

gpshead commented May 24, 2023

I'd revert this PR. This was not a "minor cleanup".

@gpshead gpshead mentioned this pull request May 24, 2023
Closed
gpshead added a commit to gpshead/cpython that referenced this pull request May 24, 2023
@gpshead
pythongh-104837: Revert "pythongh-104341: Add a Separate "Running" Lo…
5ceae3c 
…ck for Each Thread (pythongh-104754)"

This reverts commit 097b783.
@gpshead gpshead mentioned this pull request May 24, 2023
Merged
gpshead added a commit that referenced this pull request May 24, 2023
@gpshead
gh-104837: Revert "gh-104341: Add a Separate "Running" Lock for Each …
4b56e56 
…Thread (gh-104754) (#104838)

gh-104837: Revert "gh-104341: Add a Separate "Running" Lock for Each Thread (gh-104754)"

This reverts commit 097b783.
@gpshead
Copy link
Member

gpshead commented May 24, 2023

Reverted to unblock everybody else. Please do not make changes to threading without code review and buildbot tests.

@ericsnowcurrently
Copy link
Member Author

thanks

@ericsnowcurrently ericsnowcurrently mentioned this pull request Jun 5, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
skip news
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@ericsnowcurrently @miss-islington @bedevere-bot @terryjreedy @gpshead