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.

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

Race condition in unittest.mock #98624

Closed
noah-weingarden opened this issue Oct 24, 2022 · 4 comments · Fixed by #98688
Closed

Race condition in unittest.mock #98624

noah-weingarden opened this issue Oct 24, 2022 · 4 comments · Fixed by #98688
Labels
type-bug An unexpected behavior, bug, or error

Comments

@noah-weingarden
Copy link
Contributor

noah-weingarden commented Oct 24, 2022

Bug report

Concurrent calls to NonCallableMock.__getattr__() result in a race condition.

NonCallableMock implements __getattr__(), which instantiates a new mock object for an attribute if it doesn't exist yet when it's accessed. However, if the code under test is multi-threaded and multiple threads try to access the same attribute, each thread may call __getattr__() separately and instantiate a different mock object representing the same attribute.

I created a minimal reproducible example here: https://github.com/noah-weingarden/unittest.mock-race-condition

The test and "code-under-test" are both in mre.py. events.py contains two threading.Event objects used to synchronize access to the mock objects. I copy-pasted the Python 3.10 unittest.mock module into mock.py and added code which forces a specific order of events. I also added logging so that it's clear what's going on. Search "CHANGES START" to see where my modifications are. Even though the code-under-test calls sendall(), this specific order of events always creates two different MagicMock instances for sendall(), causing the test to fail because it doesn't see that sendall() was called.

Run python3 mre.py to observe the logs and test failure. If you remove my synchronization, the test will fail non-deterministically as opposed to every time.

Here's the order of events:

  1. test_message_sent() tries to access socket().__enter__().sendall, which doesn't exist, so __getattr__() is called on the mock for socket().__enter__().
  2. __getattr__() sees that no mock object for socket().__enter__().sendall exists, so it calls _get_child_mock().
  3. Before _get_child_mock() runs, the context switches to the thread running main().
  4. main() calls sendall().
  5. socket().__enter__() still doesn't have a sendall attribute, so __getattr__() and then _get_child_mock() are called.
  6. _get_child_mock() creates a new MagicMock and sets it as the mock object for sendall(). Now main() has successfully called sendall() and its call is recorded for this mock object.
  7. The context switches back to test_message_sent(), which continues running _get_child_mock(). It creates a new MagicMock and overwrites the earlier one.
  8. Now when the test retrieves socket().__enter__().sendall , it's accessing a different mock object than the one main() used. It has no way to tell that main() actually made a call.

Your environment

At a minimum, this race condition can occur on any version of Python between 3.6 and 3.10.

Motivation for solving

Mocking is frequently used to test multi-threaded programs, so it's important that unittest.mock be thread-safe. It's not intuitive that I would need to protect code for mocking in tests from race conditions. I spent months trying to hunt down a race condition within my organization's code before I realized that the issue was our tests' interactions with unittest.mock.

Suggested solution

Synchronize access to NonCallableMock.__getattr__() with a mutex. Either create one lock shared by all NonCallableMock objects and acquire it during each call to __getattr__(), or use fine-grained locking by creating one for each top-level NonCallableMock object or even every NonCallableMock object. Mocking is used almost exclusively in unit tests, so I believe the performance hit from synchronization would be worth it. I have a solution along these lines and would be happy to contribute it.

@noah-weingarden noah-weingarden added the type-bug An unexpected behavior, bug, or error label Oct 24, 2022
@Jason-Y-Z
Copy link
Contributor

Thanks for pointing this out.

Sorry if I missed something, but from the example given, I can see that we are waiting for the main_event at line 11, but I haven't found the corresponding main_event.set() to unblock this wait. If so, how would we proceed to line 12 in the thread?

@noah-weingarden
Copy link
Contributor Author

The main_event.set() is here. This just ensures that the test and main() are both able to enter _get_child_mock() before one of them finishes.

@awdeorio
Copy link

My students in EECS 485 Web Systems at the University of Michigan ran into this bug while writing a MapReduce Framework (project spec). We unit test student solutions using the Mock library and ran into this problem along the way.

I can confirm that @noah-weingarden's suggested solution works for us. We monkey-patched the library.

Thank you to the cpython team for taking a look at this issue, and thanks to @noah-weingarden for taking the time to narrow this down and provide a suggested fix.

@Jason-Y-Z
Copy link
Contributor

The main_event.set() is here. This just ensures that the test and main() are both able to enter _get_child_mock() before one of them finishes.

Thanks for the clarification; it makes sense to me now.

cjw296 pushed a commit that referenced this issue Oct 28, 2022

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
* 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
miss-islington pushed a commit to miss-islington/cpython that referenced this issue Oct 28, 2022
…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>
miss-islington pushed a commit to miss-islington/cpython that referenced this issue Oct 28, 2022
…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>
ambv pushed a commit that referenced this issue Oct 28, 2022
…#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 issue Oct 28, 2022
…#98797)

(cherry picked from commit 0346edd)

Co-authored-by: noah-weingarden <33741795+noah-weingarden@users.noreply.github.com>
gvanrossum pushed a commit to gvanrossum/cpython that referenced this issue Oct 28, 2022
* 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
Labels
type-bug An unexpected behavior, bug, or error
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants