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-98624 Add mutex to unittest.mock.NonCallableMock #98688

Merged
merged 8 commits into from
Oct 28, 2022
Merged

gh-98624 Add mutex to unittest.mock.NonCallableMock #98688

merged 8 commits into from
Oct 28, 2022

Conversation

noah-weingarden
Copy link
Contributor

@noah-weingarden noah-weingarden commented Oct 26, 2022 •
edited by bedevere-bot

Closes #98624. I'm open to any and all feedback!

@cpython-cla-bot
Copy link

cpython-cla-bot bot commented Oct 26, 2022 •
edited

All commit authors signed the Contributor License Agreement.
CLA signed

Jason-Y-Z
Jason-Y-Z reviewed Oct 26, 2022
View reviewed changes
Copy link
Contributor

@Jason-Y-Z Jason-Y-Z left a comment

Choose a reason for hiding this comment

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

Thanks for the PR.
I left a few comments from my side and please feel free to take them or leave them.
A more general question would be - do we have a way to automated-test this?

Lib/unittest/mock.py Outdated Show resolved Hide resolved
Lib/unittest/mock.py Show resolved Hide resolved
Lib/unittest/mock.py Outdated Show resolved Hide resolved
@noah-weingarden
Copy link
Contributor Author

noah-weingarden commented Oct 26, 2022 •
edited

A more general question would be - do we have a way to automated-test this?

I've been thinking about that too. It's really hard to test because it's such a subtle race condition. The test I created in the minimal reproducible example for #98624 only consistently demonstrates the bug because I added synchronization directly inside the standard library. Without doing that, I believe any test would only fail non-deterministically rather than every time. Would it be ok to just add a link to the issue in the comments? Unless you or someone else can think of a way to test this bug consistently?

@noah-weingarden noah-weingarden requested review from Jason-Y-Z and removed request for cjw296 October 26, 2022 20:17
@noah-weingarden
Copy link
Contributor Author

Oops, I didn't realize re-requesting a review would remove the request for @cjw296!

Jason-Y-Z
Jason-Y-Z reviewed Oct 27, 2022
View reviewed changes
Copy link
Contributor

@Jason-Y-Z Jason-Y-Z left a comment

Choose a reason for hiding this comment

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

Look good to me. Let's see what Chris @cjw296 says.

cjw296
cjw296 approved these changes Oct 28, 2022
View reviewed changes
Copy link
Contributor

@cjw296 cjw296 left a comment •
edited

Choose a reason for hiding this comment

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

Looks good to me :-)

@cjw296 cjw296 merged commit 0346edd into python:main Oct 28, 2022
@cjw296 cjw296 added needs backport to 3.10 only security fixes needs backport to 3.11 only security fixes labels Oct 28, 2022
@miss-islington
Copy link
Contributor

Thanks @noah-weingarden for the PR, and @cjw296 for merging it 🌮🎉.. I'm working now to backport this PR to: 3.11.
🐍🍒⛏🤖

@miss-islington
Copy link
Contributor

Thanks @noah-weingarden for the PR, and @cjw296 for merging it 🌮🎉.. I'm working now to backport this PR to: 3.10.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Oct 28, 2022
@noah-weingarden @miss-islington
pythongh-98624 Add mutex to unittest.mock.NonCallableMock (pythonGH-9…
d9d19ac 
…8688)

* Added lock to NonCallableMock in unittest.mock

* Add blurb

* Nitpick blurb

* Edit comment based on @Jason-Y-Z's review

* Add link to GH issue
(cherry picked from commit 0346edd)

Co-authored-by: noah-weingarden <33741795+noah-weingarden@users.noreply.github.com>
@bedevere-bot
Copy link

GH-98797 is a backport of this pull request to the 3.11 branch.

@bedevere-bot bedevere-bot removed the needs backport to 3.11 only security fixes label Oct 28, 2022
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Oct 28, 2022
@noah-weingarden @miss-islington
pythongh-98624 Add mutex to unittest.mock.NonCallableMock (pythonGH-9…
cbf6e73 
…8688)

* Added lock to NonCallableMock in unittest.mock

* Add blurb

* Nitpick blurb

* Edit comment based on @Jason-Y-Z's review

* Add link to GH issue
(cherry picked from commit 0346edd)

Co-authored-by: noah-weingarden <33741795+noah-weingarden@users.noreply.github.com>
@bedevere-bot
Copy link

GH-98798 is a backport of this pull request to the 3.10 branch.

@bedevere-bot bedevere-bot removed the needs backport to 3.10 only security fixes label Oct 28, 2022
@cjw296
Copy link
Contributor

cjw296 commented Oct 28, 2022

@pablogsal - how far are we backporting bugfixes now? 3.6 onwards right? ;-)

@ambv
Copy link
Contributor

ambv commented Oct 28, 2022

3.10 onwards unless it's security-related.

ambv pushed a commit that referenced this pull request Oct 28, 2022
@miss-islington @noah-weingarden
[3.10] gh-98624 Add mutex to unittest.mock.NonCallableMock (GH-98688) (…
3f0f743 
…#98798)

(cherry picked from commit 0346edd)

Co-authored-by: noah-weingarden <33741795+noah-weingarden@users.noreply.github.com>
ambv pushed a commit that referenced this pull request Oct 28, 2022
@miss-islington @noah-weingarden
[3.11] gh-98624 Add mutex to unittest.mock.NonCallableMock (GH-98688) (…
725efe4 
…#98797)

(cherry picked from commit 0346edd)

Co-authored-by: noah-weingarden <33741795+noah-weingarden@users.noreply.github.com>
@noah-weingarden
Copy link
Contributor Author

Thanks for looking into this! :)

@noah-weingarden noah-weingarden deleted the gh-98624-add-lock-to-unittest-mock branch October 28, 2022 14:13
@cjw296
Copy link
Contributor

cjw296 commented Oct 28, 2022

@ambv - wow, for many of us, 3.10 is the "new shiny we're super excited about eventually moving on to"!

gvanrossum pushed a commit to gvanrossum/cpython that referenced this pull request Oct 28, 2022
@noah-weingarden @gvanrossum
pythongh-98624 Add mutex to unittest.mock.NonCallableMock (python#98688)
955c293 
* Added lock to NonCallableMock in unittest.mock

* Add blurb

* Nitpick blurb

* Edit comment based on @Jason-Y-Z's review

* Add link to GH issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Jason-Y-Z Jason-Y-Z Jason-Y-Z left review comments

@cjw296 cjw296 cjw296 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Race condition in unittest.mock
6 participants
@noah-weingarden @miss-islington @bedevere-bot @cjw296 @ambv @Jason-Y-Z