python / cpython

The Python programming language
https://www.python.org
Other
62.34k stars 29.94k forks source link

`test_asyncio` `test_to_thread_concurrent` flaky #122957

Closed colesbury closed 3 days ago

colesbury commented 1 month ago

The call_count counter on mock.Mock is not atomic, so updates may be lost:

https://github.com/python/cpython/blob/ab094d1b2b348f670743f88e52d1a3f2cab5abd5/Lib/test/test_asyncio/test_threads.py#L32-L41

This leads to occasional errors like:

File "Lib/test/test_asyncio/test_threads.py", line 41, in test_to_thread_concurrent
self.assertEqual(func.call_count, 10)
~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^
AssertionError: 9 != 10

I think the update is not actually atomic even with the GIL because of the use of _delegating_property in Mock, but I'm not entirely sure. The failures are definitely more likely to occur in the free-threaded build.

Linked PRs

kumaraditya303 commented 1 month ago

I can't think of good way to fix this, maybe mock should be made atomic?

colesbury commented 1 month ago

Seems easier to just use a regular function instead of a mock.Mock(). In other places, Python has for a long time used lists to count atomically. It's not clean, but it works:

        calls = []
        def func():
            calls.append(1)
...

        self.assertEqual(sum(calls), 10)
lesteve commented 3 days ago

I was able to reproduce the failures locally with a free-threaded build so I opened https://github.com/python/cpython/pull/124039 with the recommended fix (using a list instead of a Mock).