Skip to content

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

Merged
cjw296 merged 8 commits intopython:mainfrom
noah-weingarden:gh-98624-add-lock-to-unittest-mock
Oct 28, 2022
Merged

gh-98624 Add mutex to unittest.mock.NonCallableMock#98688
cjw296 merged 8 commits intopython:mainfrom
noah-weingarden:gh-98624-add-lock-to-unittest-mock

Conversation

@noah-weingarden
Copy link
Contributor

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

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

@ghost
Copy link

ghost commented Oct 26, 2022 •
edited by ghost
Loading

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?

@noah-weingarden
Copy link
Contributor Author

noah-weingarden commented Oct 26, 2022 •
edited
Loading

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

noah-weingarden commented Oct 26, 2022

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
Loading

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

miss-islington commented Oct 28, 2022

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

miss-islington commented Oct 28, 2022

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

bedevere-bot commented Oct 28, 2022

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

bedevere-bot commented Oct 28, 2022

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

noah-weingarden commented Oct 28, 2022

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

@cjw296 cjw296 cjw296 approved these changes

+1 more reviewer

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

Reviewers whose approvals may not affect merge requirements

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