AttributeError from stopping stopped patch which was started more than once

[Red4Ru]

Bug report

It seems, such bug appeared after 4b222c9
Suppose we have initialized patch, than call start more than once and than call stop more than once:

>>> from unittest import mock
... class Foo:
...     bar = None
... patch = mock.patch.object(Foo, 'bar', 'x')
>>> patch.start()
'x'
>>> patch.start()
'x'
>>> patch.stop()
False
>>> patch.stop()
Traceback (most recent call last):
  File "...", line ..., in runcode
    coro = func()
  File "<input>", line 1, in <module>
  File "/usr/lib/python3.8/unittest/mock.py", line 1542, in stop
    return self.__exit__(None, None, None)
  File "/usr/lib/python3.8/unittest/mock.py", line 1508, in __exit__
    if self.is_local and self.temp_original is not DEFAULT:
AttributeError: '_patch' object has no attribute 'is_local'

But if we call start only once, multiple stops won't cause such error.
For a first glance it is due to stop excepting ValueError on removing patch from _active_patches and if ValueError was not raised it proceeds assuming patch have attribute is_local.

Your environment

• CPython versions tested on: Python 3.8
• Operating system: Ubuntu
• I think error exists in newer versions too

Linked PRs

• gh-104745: Limit starting a patcher more than once without stopping it #126649
• [3.13] gh-104745: Limit starting a patcher more than once without stopping it (GH-126649) #126772
• [3.12] gh-104745: Limit starting a patcher more than once without stopping it (GH-126649) #126773

[terryjreedy]

A couple of the ...s in the message should be >>>s. Anyway, reproduced with new 3.13.0a0. Only difference is the added hint " Did you mean: 'has_local'?". I have no idea if this is a bug or not.

[Red4Ru]

As I can see, before 4b222c9 there was a check if attribute is_local exists before referencing it so in aforementioned case any exception were not raised.
It makes me think this behavior was not implemented intentionally and this is a bug.

[tirkarthi]

patch.stop used to raise RuntimeError and was made to return None in #80547 . The issue is that patch.start() sets is_local but once stop is called we do del self.is_local . But multiple starts lead to self._active_patches with same object in the list thus the self._active_patches.remove doesn't raise the ValueError . Maybe we can bring back the is_local attribute check or stop should remove all references of the object from self._active_patches. cc: @serhiy-storchaka

[Kolumbs]

I have also stumbled upon this error. But in a little bit different use case. It was relatively hard to investigate as patch decorator was failing within one method and it also involved timing. Although I used threads and I agree it is dangerous path.

Here I created simple testcase using unittest to show the issue:

"""Hypothetical test using threads within tests."""
import _thread
import time
import unittest
import unittest.mock


class Background(unittest.TestCase):
    """Testcase with parallel calls."""

    @unittest.mock.patch('string.ascii_letters', new='abc')
    def do(self):
        """Do something."""
        time.sleep(2)
    
    def test(self):
        """Test something concurrently."""
        _thread.start_new_thread(self.do, ())
        time.sleep(1)
        self.do()
        time.sleep(1)

What happens here is that this test will fail with following (Python 3.11):

  File "/usr/local/lib/python3.11/unittest/mock.py", line 1356, in patched
    with self.decoration_helper(patched,
  File "/usr/local/lib/python3.11/contextlib.py", line 144, in __exit__
    next(self.gen)
  File "/usr/local/lib/python3.11/unittest/mock.py", line 1336, in decoration_helper
    with contextlib.ExitStack() as exit_stack:
  File "/usr/local/lib/python3.11/contextlib.py", line 586, in __exit__
    raise exc_details[1]
  File "/usr/local/lib/python3.11/contextlib.py", line 571, in __exit__
    if cb(*exc_details):
       ^^^^^^^^^^^^^^^^
  File "/usr/local/lib/python3.11/unittest/mock.py", line 1558, in __exit__
    if self.is_local and self.temp_original is not DEFAULT:
       ^^^^^^^^^^^^^
AttributeError: '_patch' object has no attribute 'is_local'

Although using threads with tests and using patching is with warning attached, it is quite cumbersome to understand why such error can happen. By reading the unittest/mock.py and it's patch exit convention to delete attributes like self.local, self.target and others, one can start to suspect the thread problem to be the issue.

[davidszotten]

patch.stop used to raise RuntimeError and was made to return None in #80547 . The issue is that patch.start() sets is_local but once stop is called we do del self.is_local . But multiple starts lead to self._active_patches with same object in the list thus the self._active_patches.remove doesn't raise the ValueError . Maybe we can bring back the is_local attribute check or stop should remove all references of the object from self._active_patches. cc: @serhiy-storchaka

i started having a look at this. but after reading the code and experimenting a bit i wonder if the bug is allowing start() to be called multiple times. unless i'm misreading, calling start a second time e.g. overwrites the reference to the original object, which breaks unpatching. maybe the second start should raise an exception?

[Red4Ru]

i started having a look at this. but after reading the code and experimenting a bit i wonder if the bug is allowing start() to be called multiple times. unless i'm misreading, calling start a second time e.g. overwrites the reference to the original object, which breaks unpatching. maybe the second start should raise an exception?

I think it would be good to do this together with the implementation of the is_started method so that you can check if the patch has already been started to avoid an exception
I can try to make a PR if it sounds good enough, what do you think? @davidszotten