Skip to content

gh-115942: Add locked to several multiprocessing locks - #115944

Merged
sobolevn merged 10 commits into
python:mainfrom
sobolevn:issue-115942
Apr 8, 2025
Merged

gh-115942: Add locked to several multiprocessing locks#115944
sobolevn merged 10 commits into
python:mainfrom
sobolevn:issue-115942

Conversation

@sobolevn

@sobolevnsobolevn commented Feb 26, 2024•
edited
Loading

Copy link
Copy Markdown
Member

TODO:

  • Get field-expert's approval
  • Docs
  • News

@bedevere-appbedevere-appBot mentioned this pull request Feb 26, 2024
Closed
@sobolevn
sobolevn marked this pull request as ready for review March 13, 2024 06:44
@sobolevn
sobolevn requested a review from gpshead as a code ownerMarch 13, 2024 06:44
@sobolevn

sobolevn commented Jun 3, 2024

Copy link
Copy Markdown
MemberAuthor

@gpshead do you have an opinion about this? :)

@sobolevn

sobolevn commented Jun 21, 2024

Copy link
Copy Markdown
MemberAuthor

friendly ping @gpshead

mpage
mpage reviewed Jun 21, 2024
View reviewed changes
Comment threadModules/_threadmodule.c Outdated
@sobolevn@mpage
Update Modules/_threadmodule.c
646c650
Co-authored-by: mpage <mpage@cs.stanford.edu>
vstinner
vstinner reviewed Mar 13, 2025
View reviewed changes
Comment threadModules/_threadmodule.c Outdated
@gpshead

gpshead commented Mar 13, 2025

Copy link
Copy Markdown
Member

FWIW this PR makes sense to me.

@sobolevn

sobolevn commented Mar 13, 2025

Copy link
Copy Markdown
MemberAuthor

Ok, then I will add all the missing details. Thanks a lot! 👍

picnixz
picnixz reviewed Mar 13, 2025
View reviewed changes
Comment threadLib/test/_test_multiprocessing.py
vstinner
vstinner reviewed Apr 1, 2025
View reviewed changes

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

Please add tests on threading.RLock.locked().

picnixz
picnixz reviewed Apr 1, 2025
View reviewed changes

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

Just some rewording. Otherwise, once tests requested by Victor are added, LGTM.

Comment threadDoc/library/multiprocessing.rst Outdated
Comment threadDoc/library/multiprocessing.rst Outdated
Comment threadDoc/library/threading.rst Outdated
Comment threadModules/_threadmodule.c Outdated
@sobolevn

sobolevn commented Apr 1, 2025

Copy link
Copy Markdown
MemberAuthor

@vstinner@gpshead this is now ready to be reviewed :)

@sobolevn
sobolevn requested review from mpage and vstinnerApril 1, 2025 09:43
picnixz
picnixz approved these changes Apr 1, 2025
View reviewed changes
Comment threadLib/threading.py Outdated
vstinner
vstinner reviewed Apr 1, 2025
View reviewed changes
Comment threadLib/importlib/_bootstrap.py
vstinner
vstinner approved these changes Apr 1, 2025
View reviewed changes

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

LGTM

hugovk
hugovk reviewed Apr 7, 2025
View reviewed changes
Comment threadDoc/library/multiprocessing.rst Outdated
Comment threadDoc/library/multiprocessing.rst Outdated
Comment threadDoc/library/threading.rst Outdated
Comment threadDoc/library/threading.rst Outdated
Comment threadLib/threading.py Outdated
Comment threadModules/_threadmodule.c Outdated
@sobolevn@hugovk
Apply suggestions from code review
1a50a20
Co-authored-by: Hugo van Kemenade <1324225+hugovk@users.noreply.github.com>
gpshead
gpshead approved these changes Apr 8, 2025
View reviewed changes

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

I wish I could object to this being a locked() method instead of a .locked property... but this is matching an existing ancient API on purpose so... LGTM.

@sobolevn
sobolevn merged commit f7305a0 into python:mainApr 8, 2025
@sobolevn

sobolevn commented Apr 8, 2025

Copy link
Copy Markdown
MemberAuthor

Thanks everyone! 🎉

This was referenced Apr 15, 2025
Closed
Closed
@YvesDupYvesDup mentioned this pull request May 20, 2025
Closed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@hugovkhugovkhugovk left review comments
@gpsheadgpsheadgpshead approved these changes
@vstinnervstinnervstinner approved these changes
@picnixzpicnixzpicnixz approved these changes
@mpagempageAwaiting requested review from mpage
@brettcannonbrettcannonAwaiting requested review from brettcannonbrettcannon is a code owner
@ericsnowcurrentlyericsnowcurrentlyAwaiting requested review from ericsnowcurrentlyericsnowcurrently is a code owner
@ncoghlanncoghlanAwaiting requested review from ncoghlanncoghlan is a code owner
@warsawwarsawAwaiting requested review from warsawwarsaw is a code owner

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@sobolevn@gpshead@vstinner@mpage@hugovk@picnixz