From f165e5694dee9de2759f359994d991260ab04250 Mon Sep 17 00:00:00 2001 From: Shantanu <12621235+hauntsaninja@users.noreply.github.com> Date: Fri, 22 Sep 2023 23:57:29 -0700 Subject: [PATCH 1/9] Avoid creating ref cycles By storing previously raised exceptions inside a local, this code created ref cycles that kept all locals in all calling stack frames alive. This is because exceptions hold references to their tracebacks, which hold references to the relevant frames, which holds a reference to the local errors dict that holds references to the exceptions. See https://peps.python.org/pep-0344/#open-issue-garbage-collection and https://peps.python.org/pep-3110/#rationale This breaks the cycle by deleting the local when we raise, so frames are destroyed by the normal reference counting mechanism. This fixes some resource exhaustion issues I encountered at work. --- src/typeguard/_checkers.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/typeguard/_checkers.py b/src/typeguard/_checkers.py index 34bff23..145312f 100644 --- a/src/typeguard/_checkers.py +++ b/src/typeguard/_checkers.py @@ -405,7 +405,10 @@ def check_union( formatted_errors = indent( "\n".join(f"{key}: {error}" for key, error in errors.items()), " " ) - raise TypeCheckError(f"did not match any element in the union:\n{formatted_errors}") + try: + raise TypeCheckError(f"did not match any element in the union:\n{formatted_errors}") + finally: + del errors def check_uniontype( From 8b5d2c6bab6824a5f6ebce793e5ab5dfc44b7bc1 Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Sat, 23 Sep 2023 06:57:47 +0000 Subject: [PATCH 2/9] [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --- src/typeguard/_checkers.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/typeguard/_checkers.py b/src/typeguard/_checkers.py index 145312f..2ee23ee 100644 --- a/src/typeguard/_checkers.py +++ b/src/typeguard/_checkers.py @@ -406,7 +406,9 @@ def check_union( "\n".join(f"{key}: {error}" for key, error in errors.items()), " " ) try: - raise TypeCheckError(f"did not match any element in the union:\n{formatted_errors}") + raise TypeCheckError( + f"did not match any element in the union:\n{formatted_errors}" + ) finally: del errors From 7577df51e2412c977f4bd2022edcb157cf5097f8 Mon Sep 17 00:00:00 2001 From: hauntsaninja Date: Sat, 13 Jan 2024 19:44:31 -0800 Subject: [PATCH 3/9] code review --- src/typeguard/_checkers.py | 10 ++++------ tests/test_checkers.py | 15 +++++++++++++++ 2 files changed, 19 insertions(+), 6 deletions(-) diff --git a/src/typeguard/_checkers.py b/src/typeguard/_checkers.py index 2ee23ee..fea3d26 100644 --- a/src/typeguard/_checkers.py +++ b/src/typeguard/_checkers.py @@ -405,12 +405,10 @@ def check_union( formatted_errors = indent( "\n".join(f"{key}: {error}" for key, error in errors.items()), " " ) - try: - raise TypeCheckError( - f"did not match any element in the union:\n{formatted_errors}" - ) - finally: - del errors + del errors + raise TypeCheckError( + f"did not match any element in the union:\n{formatted_errors}" + ) def check_uniontype( diff --git a/tests/test_checkers.py b/tests/test_checkers.py index 1a87f2c..62482c4 100644 --- a/tests/test_checkers.py +++ b/tests/test_checkers.py @@ -768,6 +768,21 @@ def test_union_fail(self, annotation, value): f" int: is not an instance of int" ) + def test_union_reference_leak(self): + leaked = True + class Leak: + def __del__(self): + nonlocal leaked + leaked = False + + def inner(): + leak = Leak() + with pytest.raises(TypeCheckError, match="any element in the union:"): + check_type(1, Union[str, bytes]) + + inner() + assert not leaked + class TestTypevar: def test_bound(self): From 3e31b88499544c7edca68da737202a91c8dd904b Mon Sep 17 00:00:00 2001 From: hauntsaninja Date: Sat, 13 Jan 2024 19:46:30 -0800 Subject: [PATCH 4/9] . --- src/typeguard/_checkers.py | 4 +--- tests/test_checkers.py | 3 ++- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/src/typeguard/_checkers.py b/src/typeguard/_checkers.py index fea3d26..9d299b6 100644 --- a/src/typeguard/_checkers.py +++ b/src/typeguard/_checkers.py @@ -406,9 +406,7 @@ def check_union( "\n".join(f"{key}: {error}" for key, error in errors.items()), " " ) del errors - raise TypeCheckError( - f"did not match any element in the union:\n{formatted_errors}" - ) + raise TypeCheckError(f"did not match any element in the union:\n{formatted_errors}") def check_uniontype( diff --git a/tests/test_checkers.py b/tests/test_checkers.py index 62482c4..5c28e69 100644 --- a/tests/test_checkers.py +++ b/tests/test_checkers.py @@ -770,13 +770,14 @@ def test_union_fail(self, annotation, value): def test_union_reference_leak(self): leaked = True + class Leak: def __del__(self): nonlocal leaked leaked = False def inner(): - leak = Leak() + leak = Leak() # noqa: F841 with pytest.raises(TypeCheckError, match="any element in the union:"): check_type(1, Union[str, bytes]) From fdb173e5e6188df35bd8264470dd7faa9d4969f8 Mon Sep 17 00:00:00 2001 From: hauntsaninja Date: Sat, 13 Jan 2024 19:48:08 -0800 Subject: [PATCH 5/9] comment --- src/typeguard/_checkers.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/typeguard/_checkers.py b/src/typeguard/_checkers.py index 9d299b6..a9ba7fa 100644 --- a/src/typeguard/_checkers.py +++ b/src/typeguard/_checkers.py @@ -405,7 +405,7 @@ def check_union( formatted_errors = indent( "\n".join(f"{key}: {error}" for key, error in errors.items()), " " ) - del errors + del errors # avoid creating ref cycle raise TypeCheckError(f"did not match any element in the union:\n{formatted_errors}") From cf38460cfe5d26d26d350ae3ef62dc8cdcc940c5 Mon Sep 17 00:00:00 2001 From: hauntsaninja Date: Sat, 13 Jan 2024 19:52:07 -0800 Subject: [PATCH 6/9] pypy --- tests/test_checkers.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/tests/test_checkers.py b/tests/test_checkers.py index 5c28e69..7f191ec 100644 --- a/tests/test_checkers.py +++ b/tests/test_checkers.py @@ -768,6 +768,10 @@ def test_union_fail(self, annotation, value): f" int: is not an instance of int" ) + @pytest.mark.skipif( + sys.implementation.name != "cpython", + reason="Test relies on CPython's reference counting behavior", + ) def test_union_reference_leak(self): leaked = True From 4802b3ad9c4573d6aee14dfacfb6a294770085ea Mon Sep 17 00:00:00 2001 From: hauntsaninja Date: Mon, 15 Jan 2024 02:22:31 -0800 Subject: [PATCH 7/9] changelog and fix --- docs/versionhistory.rst | 5 +++++ src/typeguard/_checkers.py | 22 ++++++++++++---------- tests/test_checkers.py | 13 +++++++++++-- 3 files changed, 28 insertions(+), 12 deletions(-) diff --git a/docs/versionhistory.rst b/docs/versionhistory.rst index 868b446..2bdfb73 100644 --- a/docs/versionhistory.rst +++ b/docs/versionhistory.rst @@ -4,6 +4,11 @@ Version history This library adheres to `Semantic Versioning 2.0 `_. +**Unreleased** + +- Avoid creating reference cycles when type checking unions + (`#408 `_) + **4.1.5** (2023-09-11) - Fixed ``Callable`` erroneously rejecting a callable that has the requested amount of diff --git a/src/typeguard/_checkers.py b/src/typeguard/_checkers.py index a9ba7fa..f5dcc7b 100644 --- a/src/typeguard/_checkers.py +++ b/src/typeguard/_checkers.py @@ -395,17 +395,19 @@ def check_union( memo: TypeCheckMemo, ) -> None: errors: dict[str, TypeCheckError] = {} - for type_ in args: - try: - check_type_internal(value, type_, memo) - return - except TypeCheckError as exc: - errors[get_type_name(type_)] = exc + try: + for type_ in args: + try: + check_type_internal(value, type_, memo) + return + except TypeCheckError as exc: + errors[get_type_name(type_)] = exc - formatted_errors = indent( - "\n".join(f"{key}: {error}" for key, error in errors.items()), " " - ) - del errors # avoid creating ref cycle + formatted_errors = indent( + "\n".join(f"{key}: {error}" for key, error in errors.items()), " " + ) + finally: + del errors # avoid creating ref cycle raise TypeCheckError(f"did not match any element in the union:\n{formatted_errors}") diff --git a/tests/test_checkers.py b/tests/test_checkers.py index 7f191ec..55623c9 100644 --- a/tests/test_checkers.py +++ b/tests/test_checkers.py @@ -780,12 +780,21 @@ def __del__(self): nonlocal leaked leaked = False - def inner(): + def inner1(): + leak = Leak() # noqa: F841 + check_type(b"asdf", Union[str, bytes]) + + inner1() + assert not leaked + + leaked = True + + def inner2(): leak = Leak() # noqa: F841 with pytest.raises(TypeCheckError, match="any element in the union:"): check_type(1, Union[str, bytes]) - inner() + inner2() assert not leaked From 8afdea878c757e4ffd2ae2b88916442885493928 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alex=20Gr=C3=B6nholm?= Date: Mon, 15 Jan 2024 12:42:03 +0200 Subject: [PATCH 8/9] Update versionhistory.rst --- docs/versionhistory.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/versionhistory.rst b/docs/versionhistory.rst index 2bdfb73..c2896a9 100644 --- a/docs/versionhistory.rst +++ b/docs/versionhistory.rst @@ -4,7 +4,7 @@ Version history This library adheres to `Semantic Versioning 2.0 `_. -**Unreleased** +**UNRELEASED** - Avoid creating reference cycles when type checking unions (`#408 `_) From 17ffe9caefb58ed4c5fd321008cae9202103e3a4 Mon Sep 17 00:00:00 2001 From: hauntsaninja Date: Mon, 15 Jan 2024 03:01:11 -0800 Subject: [PATCH 9/9] remove link --- docs/versionhistory.rst | 1 - 1 file changed, 1 deletion(-) diff --git a/docs/versionhistory.rst b/docs/versionhistory.rst index c2896a9..b03fcc4 100644 --- a/docs/versionhistory.rst +++ b/docs/versionhistory.rst @@ -7,7 +7,6 @@ This library adheres to **UNRELEASED** - Avoid creating reference cycles when type checking unions - (`#408 `_) **4.1.5** (2023-09-11)