Skip to content

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

Merged
cjw296 merged 8 commits into
python: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 into
python:mainfrom
noah-weingarden:gh-98624-add-lock-to-unittest-mock

Conversation

@noah-weingarden

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

Copy link
Copy Markdown
Contributor

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

@ghost

ghost commented Oct 26, 2022 •
edited by ghost
Loading

Copy link
Copy Markdown

All commit authors signed the Contributor License Agreement.
CLA signed

Jason-Y-Z
Jason-Y-Z reviewed Oct 26, 2022
View reviewed changes

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

Copy link
Copy Markdown
Contributor

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?

Comment thread Lib/unittest/mock.py Outdated
Comment thread Lib/unittest/mock.py
Comment thread Lib/unittest/mock.py Outdated
@noah-weingarden

noah-weingarden commented Oct 26, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

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

noah-weingarden commented Oct 26, 2022

Copy link
Copy Markdown
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

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

Copy link
Copy Markdown
Contributor

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

@cjw296 cjw296 left a comment •
edited
Loading

Copy link
Copy Markdown
Contributor

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

miss-islington commented Oct 28, 2022

Copy link
Copy Markdown
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

miss-islington commented Oct 28, 2022

Copy link
Copy Markdown
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

bedevere-bot commented Oct 28, 2022

Copy link
Copy Markdown

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

bedevere-bot commented Oct 28, 2022

Copy link
Copy Markdown

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

cjw296 commented Oct 28, 2022

Copy link
Copy Markdown
Contributor

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

@ambv

ambv commented Oct 28, 2022

Copy link
Copy Markdown
Contributor

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

noah-weingarden commented Oct 28, 2022

Copy link
Copy Markdown
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

cjw296 commented Oct 28, 2022

Copy link
Copy Markdown
Contributor

@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