gh-104745: Limit starting a patcher more than once without stopping it #126649

Red4Ru · 2024-11-10T16:32:38Z

Previously, this would cause an AttributeError if the patch stopped more than once after this, and would also disrupt the original patched object.

Issue: AttributeError from stopping stopped patch which was started more than once #104745

Previously, this would cause an `AttributeError` if the patch stopped more than once after this, and would also disrupt the original patched object.

cpython-cla-bot · 2024-11-10T16:32:40Z

All commit authors signed the Contributor License Agreement.

Misc/NEWS.d/next/Library/2024-11-10-18-14-51.gh-issue-104745.zAa5Ke.rst

picnixz · 2024-11-10T16:43:22Z

Lib/unittest/mock.py

-        self.temp_original = original
-        self.is_local = local
-        self._exit_stack = contextlib.ExitStack()
+        self._started_context = _PatchStartedContext(


I think the naming self._context would be sufficient.

picnixz · 2024-11-10T16:44:28Z

Lib/unittest/mock.py

+            exit_stack=contextlib.ExitStack(),
+            is_local=is_local,
+            target=self.getter(),
+            temp_original=original,


This attribute can be called original since we are anyway wrapping it in a new object.

picnixz · 2024-11-10T16:44:44Z

Lib/unittest/mock.py

@@ -1320,6 +1320,14 @@ def _check_spec_arg_typos(kwargs_to_check):
            )


+class _PatchStartedContext(object):


To reduce the memory footprint, how about using a namedtuple? I would also call it only class _PatchContext because it does not really give more information to have the "Started" IMO.

Lib/unittest/mock.py

picnixz · 2024-11-10T16:46:19Z

Lib/unittest/mock.py

        try:
            setattr(self.target, self.attribute, new_attr)
            if self.attribute_name is not None:
                extra_args = {}
                if self.new is DEFAULT:
                    extra_args[self.attribute_name] =  new
                for patching in self.additional_patchers:
-                    arg = self._exit_stack.enter_context(patching)
+                    arg = self._started_context.exit_stack.enter_context(patching)


Suggested change

arg = self._started_context.exit_stack.enter_context(patching)

arg = exit_stack.enter_context(patching)

picnixz · 2024-11-10T16:46:39Z

Lib/unittest/mock.py

-        self.temp_original = original
-        self.is_local = local
-        self._exit_stack = contextlib.ExitStack()
+        self._started_context = _PatchStartedContext(


Suggested change

self._started_context = _PatchStartedContext(

exit_stack = contextlib.ExitStack()

self._started_context = _PatchStartedContext(

picnixz · 2024-11-10T16:46:45Z

Lib/unittest/mock.py

-        self.is_local = local
-        self._exit_stack = contextlib.ExitStack()
+        self._started_context = _PatchStartedContext(
+            exit_stack=contextlib.ExitStack(),


Suggested change

exit_stack=contextlib.ExitStack(),

exit_stack=exit_stack,

picnixz · 2024-11-10T16:50:04Z

Lib/unittest/mock.py

@@ -1469,13 +1478,31 @@ def get_original(self):
            )
        return original, local

+    @property
+    def is_started(self):


Previously, those were writable attributes and now it's no more the case. Could there be some code in the wild assuming so? (for instance pytest which makes quite hacky things, though I don't know if they do hacky things with this specific part of CPython).

I guess so... I have committed property setters just in case. Will write tests if needed when we figure out what to do with temp_original: do we preserve it and somehow deprecate or anything else

Fixed them. A bit ugly but anyway these setters exist not for an intended usecase but for backwards compatibility only

Misc/NEWS.d/next/Library/2024-11-10-18-14-51.gh-issue-104745.zAa5Ke.rst

…Aa5Ke.rst Co-authored-by: Peter Bierma <[email protected]>

Lib/unittest/mock.py

Co-authored-by: Bénédikt Tran <[email protected]>

I rechecked and get that `unittest.mock.patch.dict` is not involved. Also deleted a trailing comma

picnixz · 2024-11-10T22:48:47Z

Lib/unittest/mock.py

+    def is_local(self, value):
+        self._context = _PatchContext(
+            exit_stack=self._context.exit_stack,
+            is_local=value,
+            original=self._context.original,
+            target=self._context.target,
+        )


Urgh, I forgot that you cannot change the value of namedtuples. Ok, my suggestion using namedtuples was wrong. To reduce memory footprint, we can use __slots__ in a regular class instead like you had before. That way, we save an from collections import namedtuple as well and simplify the property's setter. WDYT? (again sorry for this bad suggestion).

cjw296

Can you explain why we need the whole _PatchContext thing?
What couldn't be achieved by just having a self.is_started boolean?

Red4Ru · 2024-11-11T07:21:15Z

Can you explain why we need the whole _PatchContext thing? What couldn't be achieved by just having a self.is_started boolean?

Consistency, at the first place. Why add another field for that while we already have all the bunch of others which exist only when _patch is started? (They literally were deleted in __exit__)
At first, I even called it _PatchStartedContext to make this whole purpose more obvious

cjw296

This feels like we could just add an is_started attribute, which is set to True when the patcher is started and False when it is stopped.

I'd prefer to see that smaller, simpler change, tbh.

But, great to see the new tests in this area!

bedevere-app · 2024-11-11T07:26:02Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

Red4Ru · 2024-11-11T15:08:16Z

This feels like we could just add an is_started attribute, which is set to True when the patcher is started and False when it is stopped.

I'd prefer to see that smaller, simpler change, tbh.

I think that putting these fields in _context and calculating is_started based on it makes the code easier to maintain
What are the advantages of leaving it as it is by adding the is_started field to _patch?

cjw296 · 2024-11-11T18:40:51Z

What you propose introduces more complexity than is required to fix the bug, and to a piece of code that is already very complex.

The change I think you should make amounts to 3 lines, if I understand correctly, and I would prefer that change to the one currently in this PR.

Red4Ru · 2024-11-12T08:40:34Z

What you propose introduces more complexity than is required to fix the bug, and to a piece of code that is already very complex.

The change I think you should make amounts to 3 lines, if I understand correctly, and I would prefer that change to the one currently in this PR.

Well, if we should minimize impact on existing code, I agree with this suggestion
But if someone wants to refactor this later, penultimate commit might be useful

cjw296 · 2024-11-12T16:28:19Z

Lib/test/test_unittest/testmock/testpatch.py

@@ -1098,7 +1146,7 @@ def test_new_callable_patch(self):

        self.assertIsNot(m1, m2)
        for mock in m1, m2:
-            self.assertNotCallable(m1)
+            self.assertNotCallable(mock)


Lib/unittest/mock.py

miss-islington-app · 2024-11-13T08:20:41Z

Thanks @Red4Ru for the PR, and @cjw296 for merging it 🌮🎉.. I'm working now to backport this PR to: 3.12, 3.13.
🐍🍒⛏🤖

…ping it (pythonGH-126649) Previously, this would cause an `AttributeError` if the patch stopped more than once after this, and would also disrupt the original patched object. --------- (cherry picked from commit 1e40c5b) Co-authored-by: Red4Ru <[email protected]> Co-authored-by: Peter Bierma <[email protected]> Co-authored-by: Bénédikt Tran <[email protected]>

bedevere-app · 2024-11-13T08:21:02Z

GH-126772 is a backport of this pull request to the 3.13 branch.

…ping it (pythonGH-126649) Previously, this would cause an `AttributeError` if the patch stopped more than once after this, and would also disrupt the original patched object. --------- (cherry picked from commit 1e40c5b) Co-authored-by: Red4Ru <[email protected]> Co-authored-by: Peter Bierma <[email protected]> Co-authored-by: Bénédikt Tran <[email protected]>

bedevere-app · 2024-11-13T08:21:14Z

GH-126773 is a backport of this pull request to the 3.12 branch.

…pping it (GH-126649) (#126773) gh-104745: Limit starting a patcher more than once without stopping it (GH-126649) Previously, this would cause an `AttributeError` if the patch stopped more than once after this, and would also disrupt the original patched object. --------- (cherry picked from commit 1e40c5b) Co-authored-by: Red4Ru <[email protected]> Co-authored-by: Peter Bierma <[email protected]> Co-authored-by: Bénédikt Tran <[email protected]>

…pping it (GH-126649) (#126772) gh-104745: Limit starting a patcher more than once without stopping it (GH-126649) Previously, this would cause an `AttributeError` if the patch stopped more than once after this, and would also disrupt the original patched object. --------- (cherry picked from commit 1e40c5b) Co-authored-by: Red4Ru <[email protected]> Co-authored-by: Peter Bierma <[email protected]> Co-authored-by: Bénédikt Tran <[email protected]>

Limit starting a patcher more than once without stopping it

f78011f

Previously, this would cause an `AttributeError` if the patch stopped more than once after this, and would also disrupt the original patched object.

Red4Ru requested a review from cjw296 as a code owner November 10, 2024 16:32

bedevere-app bot added the awaiting review label Nov 10, 2024

bedevere-app bot mentioned this pull request Nov 10, 2024

AttributeError from stopping stopped patch which was started more than once #104745

Closed

ZeroIntensity reviewed Nov 10, 2024

View reviewed changes

Misc/NEWS.d/next/Library/2024-11-10-18-14-51.gh-issue-104745.zAa5Ke.rst Outdated Show resolved Hide resolved

picnixz reviewed Nov 10, 2024

View reviewed changes

Red4Ru and others added 3 commits November 10, 2024 20:52

Update Misc/NEWS.d/next/Library/2024-11-10-18-14-51.gh-issue-104745.z…

1b9c6d4

…Aa5Ke.rst Co-authored-by: Peter Bierma <[email protected]>

Refactor patch context

399bb66

Add property setters (untested)

e28c3fb

picnixz reviewed Nov 10, 2024

View reviewed changes

Lib/unittest/mock.py Outdated Show resolved Hide resolved

Lib/unittest/mock.py Outdated Show resolved Hide resolved

Red4Ru and others added 4 commits November 10, 2024 23:59

Avoid re-call of self.getter

a07d3b5

Co-authored-by: Bénédikt Tran <[email protected]>

Preserve only _patch.temp_original instead of _patch.original

3c4edf2

Change NEWS file

916ac69

I rechecked and get that `unittest.mock.patch.dict` is not involved. Also deleted a trailing comma

Fix of property setters along with a test for them

862d5a0

picnixz reviewed Nov 10, 2024

View reviewed changes

cjw296 reviewed Nov 11, 2024

View reviewed changes

cjw296 requested changes Nov 11, 2024

View reviewed changes

bedevere-app bot added awaiting changes and removed awaiting review labels Nov 11, 2024

Refactor _PatchContext from namedtuple to plain class with __slots__

7719894

Minimize changes in mock.py

1180b63

cjw296 reviewed Nov 12, 2024

View reviewed changes

Leave as little changes in mock.py as possible

7d73933

cjw296 approved these changes Nov 13, 2024

View reviewed changes

bedevere-app bot added awaiting merge and removed awaiting changes labels Nov 13, 2024

cjw296 added needs backport to 3.12 bug and security fixes needs backport to 3.13 bugs and security fixes labels Nov 13, 2024

cjw296 merged commit 1e40c5b into python:main Nov 13, 2024
43 of 44 checks passed

bedevere-app bot removed the awaiting merge label Nov 13, 2024

bedevere-app bot removed the needs backport to 3.13 bugs and security fixes label Nov 13, 2024

bedevere-app bot removed the needs backport to 3.12 bug and security fixes label Nov 13, 2024

Red4Ru deleted the fix-issue-104745 branch November 13, 2024 19:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

gh-104745: Limit starting a patcher more than once without stopping it #126649

gh-104745: Limit starting a patcher more than once without stopping it #126649

Red4Ru commented Nov 10, 2024 •

edited by bedevere-app bot

Loading

cpython-cla-bot bot commented Nov 10, 2024 •

edited

Loading

picnixz Nov 10, 2024

picnixz Nov 10, 2024

picnixz Nov 10, 2024 •

edited

Loading

picnixz Nov 10, 2024

picnixz Nov 10, 2024

picnixz Nov 10, 2024

picnixz Nov 10, 2024

Red4Ru Nov 10, 2024

Red4Ru Nov 10, 2024

picnixz Nov 10, 2024

cjw296 left a comment

Red4Ru commented Nov 11, 2024 •

edited

Loading

cjw296 left a comment •

edited

Loading

bedevere-app bot commented Nov 11, 2024

Red4Ru commented Nov 11, 2024

cjw296 commented Nov 11, 2024 •

edited

Loading

Red4Ru commented Nov 12, 2024

cjw296 Nov 12, 2024

miss-islington-app bot commented Nov 13, 2024

bedevere-app bot commented Nov 13, 2024

bedevere-app bot commented Nov 13, 2024

		@@ -1320,6 +1320,14 @@ def _check_spec_arg_typos(kwargs_to_check):
		)


		class _PatchStartedContext(object):

	arg = self._started_context.exit_stack.enter_context(patching)
	arg = exit_stack.enter_context(patching)

	self._started_context = _PatchStartedContext(
	exit_stack = contextlib.ExitStack()
	self._started_context = _PatchStartedContext(

gh-104745: Limit starting a patcher more than once without stopping it #126649

gh-104745: Limit starting a patcher more than once without stopping it #126649

Conversation

Red4Ru commented Nov 10, 2024 • edited by bedevere-app bot Loading

cpython-cla-bot bot commented Nov 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

picnixz Nov 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjw296 left a comment

Choose a reason for hiding this comment

Red4Ru commented Nov 11, 2024 • edited Loading

cjw296 left a comment • edited Loading

Choose a reason for hiding this comment

bedevere-app bot commented Nov 11, 2024

Red4Ru commented Nov 11, 2024

cjw296 commented Nov 11, 2024 • edited Loading

Red4Ru commented Nov 12, 2024

Choose a reason for hiding this comment

miss-islington-app bot commented Nov 13, 2024

bedevere-app bot commented Nov 13, 2024

bedevere-app bot commented Nov 13, 2024

Red4Ru commented Nov 10, 2024 •

edited by bedevere-app bot

Loading

cpython-cla-bot bot commented Nov 10, 2024 •

edited

Loading

picnixz Nov 10, 2024 •

edited

Loading

Red4Ru commented Nov 11, 2024 •

edited

Loading

cjw296 left a comment •

edited

Loading

cjw296 commented Nov 11, 2024 •

edited

Loading