From ba3d71a688e583b953d7bf0b735bda894e0e3132 Mon Sep 17 00:00:00 2001 From: Mario Corchero Date: Mon, 6 May 2019 13:45:45 -0400 Subject: [PATCH 1/5] Fix cycle generated in `socket.create_connection` on error The cycle happens when an exception is raised in `socket.create_connection` as it saves the exception in a variable and then re-raise it. By removing the name when the exception is raised we remove this cycle. We cannot just remove the name before as in the happy path since we need the exception to be raised. --- Lib/socket.py | 6 +++++- Lib/test/test_socket.py | 23 +++++++++++++++++++++-- 2 files changed, 26 insertions(+), 3 deletions(-) diff --git a/Lib/socket.py b/Lib/socket.py index 84a5dcb0daf29a..374f1124bf7e83 100755 --- a/Lib/socket.py +++ b/Lib/socket.py @@ -839,7 +839,11 @@ def create_connection(address, timeout=_GLOBAL_DEFAULT_TIMEOUT, sock.close() if err is not None: - raise err + try: + raise err + finally: + # Break explicitly a reference cycle + err = None else: raise error("getaddrinfo returns an empty list") diff --git a/Lib/test/test_socket.py b/Lib/test/test_socket.py index 184c67c56ebd08..fe52030c89d855 100755 --- a/Lib/test/test_socket.py +++ b/Lib/test/test_socket.py @@ -15,7 +15,7 @@ import platform import array import contextlib -from weakref import proxy +import weakref import signal import math import pickle @@ -850,7 +850,7 @@ def test_csocket_repr(self): def test_weakref(self): with socket.socket(socket.AF_INET, socket.SOCK_STREAM) as s: - p = proxy(s) + p = weakref.proxy(s) self.assertEqual(p.fileno(), s.fileno()) s = None try: @@ -1882,6 +1882,25 @@ def test_socket_fileno_requires_valid_fd(self): fileno=support.make_bad_fd()) self.assertIn(cm.exception.errno, (errno.EBADF, WSAENOTSOCK)) + def testCreateConnectionDoesntCreateCycles(self): + # `socket.create_connection` saves the exception in a + # variable to then re-raise, which caused a cycle + # and kept objects in upper frames alive unnecesarily. + port = support.find_unused_port() + class A: pass + obj_wr = None + + def x(): + nonlocal obj_wr + obj = A() + obj_wr = weakref.ref(obj) + try: + socket.create_connection((HOST, port)) + except OSError: + pass + x() + assert obj_wr() is None + def test_socket_fileno_requires_socket_fd(self): with tempfile.NamedTemporaryFile() as afile: with self.assertRaises(OSError): From 4fee725a33f734a1d01b4ae616bcdd3b7dc6bac6 Mon Sep 17 00:00:00 2001 From: Mario Corchero Date: Mon, 6 May 2019 15:24:54 -0400 Subject: [PATCH 2/5] Remove exceptions saved in variables in codeop.py As the exceptions were being saved in a different name it was generating a cycle since it was own through the `__traceback__`. By unsetting them we can collect the object earlier without the need of waiting for the gc. --- Lib/codeop.py | 11 +++++++---- Lib/test/test_codeop.py | 17 +++++++++++++++++ 2 files changed, 24 insertions(+), 4 deletions(-) diff --git a/Lib/codeop.py b/Lib/codeop.py index fc7e1e70ceafe4..082285f94fe847 100644 --- a/Lib/codeop.py +++ b/Lib/codeop.py @@ -93,10 +93,13 @@ def _maybe_compile(compiler, source, filename, symbol): except SyntaxError as e: err2 = e - if code: - return code - if not code1 and repr(err1) == repr(err2): - raise err1 + try: + if code: + return code + if not code1 and repr(err1) == repr(err2): + raise err1 + finally: + err1 = err2 = None def _compile(source, filename, symbol): return compile(source, filename, symbol, PyCF_DONT_IMPLY_DEDENT) diff --git a/Lib/test/test_codeop.py b/Lib/test/test_codeop.py index 98da26fa5dab13..6f8d71a71276ae 100644 --- a/Lib/test/test_codeop.py +++ b/Lib/test/test_codeop.py @@ -7,6 +7,7 @@ from codeop import compile_command, PyCF_DONT_IMPLY_DEDENT import io +import weakref if is_jython: import sys @@ -294,6 +295,22 @@ def test_filename(self): self.assertNotEqual(compile_command("a = 1\n", "abc").co_filename, compile("a = 1\n", "def", 'single').co_filename) + def test_invalid_doesnt_keep_user_object_alive(self): + '''succeed iff str is the start of an invalid piece of code''' + class A: pass + obj_wr = None + + def x(): + nonlocal obj_wr + obj = A() + obj_wr = weakref.ref(obj) + try: + compile_command("invalid syntax =") + except SyntaxError: + pass + x() + assert obj_wr() is None + if __name__ == "__main__": unittest.main() From 2fc95b9a69bead9a15bd1a0c69476ba86bfc25ad Mon Sep 17 00:00:00 2001 From: Mario Corchero Date: Mon, 6 May 2019 15:29:30 -0400 Subject: [PATCH 3/5] Remove exceptions saved in variables in dyld.py As the exceptions were being saved in a different name it was generating a cycle since it was own through the `__traceback__`. By unsetting them we can collect the object earlier without the need of waiting for the gc. --- Lib/ctypes/macholib/dyld.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/Lib/ctypes/macholib/dyld.py b/Lib/ctypes/macholib/dyld.py index c158e672f0511a..9d86b058765a3e 100644 --- a/Lib/ctypes/macholib/dyld.py +++ b/Lib/ctypes/macholib/dyld.py @@ -149,6 +149,8 @@ def framework_find(fn, executable_path=None, env=None): return dyld_find(fn, executable_path=executable_path, env=env) except ValueError: raise error + finally: + error = None def test_dyld_find(): env = {} From fb86cbee439a7d22c281092acc2c6c2a7597d4b4 Mon Sep 17 00:00:00 2001 From: Mario Corchero Date: Mon, 6 May 2019 15:34:26 -0400 Subject: [PATCH 4/5] Add news entry --- .../next/Library/2019-05-06-15-34-17.bpo-36820.Eh5mIB.rst | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 Misc/NEWS.d/next/Library/2019-05-06-15-34-17.bpo-36820.Eh5mIB.rst diff --git a/Misc/NEWS.d/next/Library/2019-05-06-15-34-17.bpo-36820.Eh5mIB.rst b/Misc/NEWS.d/next/Library/2019-05-06-15-34-17.bpo-36820.Eh5mIB.rst new file mode 100644 index 00000000000000..82f6635c81582d --- /dev/null +++ b/Misc/NEWS.d/next/Library/2019-05-06-15-34-17.bpo-36820.Eh5mIB.rst @@ -0,0 +1,3 @@ +Break cycle generated when saving an exception in socket.py, codeop.py and +dyld.py as they keep alive not only the exception but user objects through +the ``__traceback__`` attribute. Patch by Mario Corchero. From be7ebe04245d5b87641a13ba679da94a049b1db7 Mon Sep 17 00:00:00 2001 From: Mario Corchero Date: Fri, 6 Dec 2019 14:07:22 +0000 Subject: [PATCH 5/5] tests: Remove tests as required on review --- Lib/test/test_codeop.py | 17 ----------------- Lib/test/test_socket.py | 23 ++--------------------- 2 files changed, 2 insertions(+), 38 deletions(-) diff --git a/Lib/test/test_codeop.py b/Lib/test/test_codeop.py index 6f8d71a71276ae..98da26fa5dab13 100644 --- a/Lib/test/test_codeop.py +++ b/Lib/test/test_codeop.py @@ -7,7 +7,6 @@ from codeop import compile_command, PyCF_DONT_IMPLY_DEDENT import io -import weakref if is_jython: import sys @@ -295,22 +294,6 @@ def test_filename(self): self.assertNotEqual(compile_command("a = 1\n", "abc").co_filename, compile("a = 1\n", "def", 'single').co_filename) - def test_invalid_doesnt_keep_user_object_alive(self): - '''succeed iff str is the start of an invalid piece of code''' - class A: pass - obj_wr = None - - def x(): - nonlocal obj_wr - obj = A() - obj_wr = weakref.ref(obj) - try: - compile_command("invalid syntax =") - except SyntaxError: - pass - x() - assert obj_wr() is None - if __name__ == "__main__": unittest.main() diff --git a/Lib/test/test_socket.py b/Lib/test/test_socket.py index fe52030c89d855..184c67c56ebd08 100755 --- a/Lib/test/test_socket.py +++ b/Lib/test/test_socket.py @@ -15,7 +15,7 @@ import platform import array import contextlib -import weakref +from weakref import proxy import signal import math import pickle @@ -850,7 +850,7 @@ def test_csocket_repr(self): def test_weakref(self): with socket.socket(socket.AF_INET, socket.SOCK_STREAM) as s: - p = weakref.proxy(s) + p = proxy(s) self.assertEqual(p.fileno(), s.fileno()) s = None try: @@ -1882,25 +1882,6 @@ def test_socket_fileno_requires_valid_fd(self): fileno=support.make_bad_fd()) self.assertIn(cm.exception.errno, (errno.EBADF, WSAENOTSOCK)) - def testCreateConnectionDoesntCreateCycles(self): - # `socket.create_connection` saves the exception in a - # variable to then re-raise, which caused a cycle - # and kept objects in upper frames alive unnecesarily. - port = support.find_unused_port() - class A: pass - obj_wr = None - - def x(): - nonlocal obj_wr - obj = A() - obj_wr = weakref.ref(obj) - try: - socket.create_connection((HOST, port)) - except OSError: - pass - x() - assert obj_wr() is None - def test_socket_fileno_requires_socket_fd(self): with tempfile.NamedTemporaryFile() as afile: with self.assertRaises(OSError):