From 5773ef56b51de3145915bafcf9fc2a4ba14c6035 Mon Sep 17 00:00:00 2001 From: pteromys Date: Sun, 9 Jul 2023 00:05:06 -0400 Subject: [PATCH 1/8] Fix multiprocessing manager exception ref cycle. If `kind == '#ERROR'`, then `result` is an exception whose attached traceback includes the frame that calls `convert_to_error()` and has `result` in scope. So delete `result` from the scope as we raise it, to save the consumer of these exceptions from having to cope with a buildup of cyclic references. --- Lib/multiprocessing/managers.py | 10 ++++++++-- .../2023-07-09-00-36-33.gh-issue-106558.Zqsj6F.rst | 3 +++ 2 files changed, 11 insertions(+), 2 deletions(-) create mode 100644 Misc/NEWS.d/next/Library/2023-07-09-00-36-33.gh-issue-106558.Zqsj6F.rst diff --git a/Lib/multiprocessing/managers.py b/Lib/multiprocessing/managers.py index b6534939b4d98b..b185804ab1163e 100644 --- a/Lib/multiprocessing/managers.py +++ b/Lib/multiprocessing/managers.py @@ -90,7 +90,10 @@ def dispatch(c, id, methodname, args=(), kwds={}): kind, result = c.recv() if kind == '#RETURN': return result - raise convert_to_error(kind, result) + try: + raise convert_to_error(kind, result) + finally: + del result def convert_to_error(kind, result): if kind == '#ERROR': @@ -833,7 +836,10 @@ def _callmethod(self, methodname, args=(), kwds={}): conn = self._Client(token.address, authkey=self._authkey) dispatch(conn, None, 'decref', (token.id,)) return proxy - raise convert_to_error(kind, result) + try: + raise convert_to_error(kind, result) + finally: + del result def _getvalue(self): ''' diff --git a/Misc/NEWS.d/next/Library/2023-07-09-00-36-33.gh-issue-106558.Zqsj6F.rst b/Misc/NEWS.d/next/Library/2023-07-09-00-36-33.gh-issue-106558.Zqsj6F.rst new file mode 100644 index 00000000000000..408342af9853f2 --- /dev/null +++ b/Misc/NEWS.d/next/Library/2023-07-09-00-36-33.gh-issue-106558.Zqsj6F.rst @@ -0,0 +1,3 @@ +Fix ref cycle in callers of +:func:`~multiprocessing.managers.convert_to_error` by deleting ``result`` +from scope in a ``finally`` block if it's an exception. From d65ce646c6cdf93ee283c4523292232786383149 Mon Sep 17 00:00:00 2001 From: Andrew Geng Date: Thu, 3 Aug 2023 02:46:40 -0400 Subject: [PATCH 2/8] Apply NEWS.d suggestions from code review. Co-authored-by: Irit Katriel <1055913+iritkatriel@users.noreply.github.com> --- .../Library/2023-07-09-00-36-33.gh-issue-106558.Zqsj6F.rst | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Misc/NEWS.d/next/Library/2023-07-09-00-36-33.gh-issue-106558.Zqsj6F.rst b/Misc/NEWS.d/next/Library/2023-07-09-00-36-33.gh-issue-106558.Zqsj6F.rst index 408342af9853f2..8fe677f5d84b5f 100644 --- a/Misc/NEWS.d/next/Library/2023-07-09-00-36-33.gh-issue-106558.Zqsj6F.rst +++ b/Misc/NEWS.d/next/Library/2023-07-09-00-36-33.gh-issue-106558.Zqsj6F.rst @@ -1,3 +1,3 @@ -Fix ref cycle in callers of +Remove ref cycle in callers of :func:`~multiprocessing.managers.convert_to_error` by deleting ``result`` -from scope in a ``finally`` block if it's an exception. +from scope in a ``finally`` block. From db020166ca1781012969ca281106c17724368625 Mon Sep 17 00:00:00 2001 From: pteromys Date: Mon, 7 Aug 2023 00:40:08 -0400 Subject: [PATCH 3/8] Add check for ref cycles to _TestQueue.test_get. --- Lib/test/_test_multiprocessing.py | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/Lib/test/_test_multiprocessing.py b/Lib/test/_test_multiprocessing.py index c1f9487ae80511..130041ba5a6dbe 100644 --- a/Lib/test/_test_multiprocessing.py +++ b/Lib/test/_test_multiprocessing.py @@ -1139,6 +1139,15 @@ def test_get(self): self.assertRaises(pyqueue.Empty, get, timeout=TIMEOUT3) self.assertTimingAlmostEqual(get.elapsed, TIMEOUT3) + if gc.isenabled(): + gc.disable() + self.addCleanup(gc.enable) + try: + queue.get_nowait() + except pyqueue.Empty as e: + w = weakref.ref(e) + self.assertEqual(w(), None) + proc.join() close_queue(queue) From a11b780ca39bcdfd55e6e7516921f7d092d7b9ac Mon Sep 17 00:00:00 2001 From: pteromys Date: Mon, 7 Aug 2023 01:03:35 -0400 Subject: [PATCH 4/8] Maybe it's neater to move test to its own class. --- Lib/test/_test_multiprocessing.py | 32 ++++++++++++++++++++++--------- 1 file changed, 23 insertions(+), 9 deletions(-) diff --git a/Lib/test/_test_multiprocessing.py b/Lib/test/_test_multiprocessing.py index 130041ba5a6dbe..061bc7aa3df8a0 100644 --- a/Lib/test/_test_multiprocessing.py +++ b/Lib/test/_test_multiprocessing.py @@ -1139,15 +1139,6 @@ def test_get(self): self.assertRaises(pyqueue.Empty, get, timeout=TIMEOUT3) self.assertTimingAlmostEqual(get.elapsed, TIMEOUT3) - if gc.isenabled(): - gc.disable() - self.addCleanup(gc.enable) - try: - queue.get_nowait() - except pyqueue.Empty as e: - w = weakref.ref(e) - self.assertEqual(w(), None) - proc.join() close_queue(queue) @@ -3158,6 +3149,29 @@ def test_rapid_restart(self): if hasattr(manager, "shutdown"): self.addCleanup(manager.shutdown) +# +# Issue 106558: Manager exceptions should not create cyclic references. +# + +class TestManagerExceptions(unittest.TestCase): + def setUp(self): + self.mgr = multiprocessing.Manager() + + def tearDown(self): + self.mgr.shutdown() + self.mgr.join() + + def test_queue_get(self): + queue = self.mgr.Queue() + if gc.isenabled(): + gc.disable() + self.addCleanup(gc.enable) + try: + queue.get_nowait() + except pyqueue.Empty as e: + wr = weakref.ref(e) + self.assertEqual(wr(), None) + # # # From 5265cf4933946f523614dfd7202f3d8073920cbc Mon Sep 17 00:00:00 2001 From: Andrew Geng Date: Thu, 10 Aug 2023 23:07:59 -0400 Subject: [PATCH 5/8] Apply suggestions from code review. Co-authored-by: Irit Katriel <1055913+iritkatriel@users.noreply.github.com> --- Lib/multiprocessing/managers.py | 4 ++-- Lib/test/_test_multiprocessing.py | 4 +--- 2 files changed, 3 insertions(+), 5 deletions(-) diff --git a/Lib/multiprocessing/managers.py b/Lib/multiprocessing/managers.py index b185804ab1163e..273c22a7654f05 100644 --- a/Lib/multiprocessing/managers.py +++ b/Lib/multiprocessing/managers.py @@ -93,7 +93,7 @@ def dispatch(c, id, methodname, args=(), kwds={}): try: raise convert_to_error(kind, result) finally: - del result + del result # break reference cycle def convert_to_error(kind, result): if kind == '#ERROR': @@ -839,7 +839,7 @@ def _callmethod(self, methodname, args=(), kwds={}): try: raise convert_to_error(kind, result) finally: - del result + del result # break reference cycle def _getvalue(self): ''' diff --git a/Lib/test/_test_multiprocessing.py b/Lib/test/_test_multiprocessing.py index 061bc7aa3df8a0..9e74f0f6a6b02b 100644 --- a/Lib/test/_test_multiprocessing.py +++ b/Lib/test/_test_multiprocessing.py @@ -3149,11 +3149,9 @@ def test_rapid_restart(self): if hasattr(manager, "shutdown"): self.addCleanup(manager.shutdown) -# -# Issue 106558: Manager exceptions should not create cyclic references. -# class TestManagerExceptions(unittest.TestCase): + # Issue 106558: Manager exceptions avoids creating cyclic references. def setUp(self): self.mgr = multiprocessing.Manager() From 6b5b17b6bb83801aafae3f1023ac116d9ccdb002 Mon Sep 17 00:00:00 2001 From: pteromys Date: Fri, 11 Aug 2023 00:54:47 -0400 Subject: [PATCH 6/8] Add test for the convert_to_error() in dispatch(). --- Lib/test/_test_multiprocessing.py | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/Lib/test/_test_multiprocessing.py b/Lib/test/_test_multiprocessing.py index 9e74f0f6a6b02b..df879f865be09e 100644 --- a/Lib/test/_test_multiprocessing.py +++ b/Lib/test/_test_multiprocessing.py @@ -3170,6 +3170,19 @@ def test_queue_get(self): wr = weakref.ref(e) self.assertEqual(wr(), None) + def test_dispatch(self): + queue = self.mgr.Queue() + queue._connect() + if gc.isenabled(): + gc.disable() + self.addCleanup(gc.enable) + try: + multiprocessing.managers.dispatch( + queue._tls.connection, queue._id, 'get_nowait') + except pyqueue.Empty as e: + wr = weakref.ref(e) + self.assertEqual(wr(), None) + # # # From 0e13423b9a0f33482346e16c710854e148caf7e5 Mon Sep 17 00:00:00 2001 From: pteromys Date: Fri, 11 Aug 2023 01:58:25 -0400 Subject: [PATCH 7/8] Don't use private members in dispatch test. --- Lib/test/_test_multiprocessing.py | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/Lib/test/_test_multiprocessing.py b/Lib/test/_test_multiprocessing.py index df879f865be09e..aa5c95f34d8366 100644 --- a/Lib/test/_test_multiprocessing.py +++ b/Lib/test/_test_multiprocessing.py @@ -3150,6 +3150,12 @@ def test_rapid_restart(self): self.addCleanup(manager.shutdown) +class FakeConnection: + def send(self, payload): + pass + def recv(self): + return '#ERROR', pyqueue.Empty() + class TestManagerExceptions(unittest.TestCase): # Issue 106558: Manager exceptions avoids creating cyclic references. def setUp(self): @@ -3171,14 +3177,11 @@ def test_queue_get(self): self.assertEqual(wr(), None) def test_dispatch(self): - queue = self.mgr.Queue() - queue._connect() if gc.isenabled(): gc.disable() self.addCleanup(gc.enable) try: - multiprocessing.managers.dispatch( - queue._tls.connection, queue._id, 'get_nowait') + multiprocessing.managers.dispatch(FakeConnection(), None, None) except pyqueue.Empty as e: wr = weakref.ref(e) self.assertEqual(wr(), None) From 40aafe251833171074ae17b2c77031ddc9df0eb5 Mon Sep 17 00:00:00 2001 From: Irit Katriel <1055913+iritkatriel@users.noreply.github.com> Date: Fri, 11 Aug 2023 17:42:44 +0100 Subject: [PATCH 8/8] whitespace --- Lib/test/_test_multiprocessing.py | 1 + 1 file changed, 1 insertion(+) diff --git a/Lib/test/_test_multiprocessing.py b/Lib/test/_test_multiprocessing.py index aa5c95f34d8366..f881a5d4674699 100644 --- a/Lib/test/_test_multiprocessing.py +++ b/Lib/test/_test_multiprocessing.py @@ -3153,6 +3153,7 @@ def test_rapid_restart(self): class FakeConnection: def send(self, payload): pass + def recv(self): return '#ERROR', pyqueue.Empty()