From 290be0333654502051236d37d0fc0131a1d6f04f Mon Sep 17 00:00:00 2001 From: twisteroid ambassador Date: Fri, 5 Oct 2018 21:34:48 +0800 Subject: [PATCH 1/8] Use call_soon_threadsafe() for _asyncgen_finalizer_hook(). Should fix bpo-34769 in debug mode. --- Lib/asyncio/base_events.py | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/Lib/asyncio/base_events.py b/Lib/asyncio/base_events.py index 780a06192dcf8c..3726c556d4f09d 100644 --- a/Lib/asyncio/base_events.py +++ b/Lib/asyncio/base_events.py @@ -477,10 +477,7 @@ def _check_closed(self): def _asyncgen_finalizer_hook(self, agen): self._asyncgens.discard(agen) if not self.is_closed(): - self.create_task(agen.aclose()) - # Wake up the loop if the finalizer was called from - # a different thread. - self._write_to_self() + self.call_soon_threadsafe(self.create_task, agen.aclose()) def _asyncgen_firstiter_hook(self, agen): if self._asyncgens_shutdown_called: From 0e2e33ebc090b7f31bdbde86669a94921859781c Mon Sep 17 00:00:00 2001 From: twisteroid ambassador Date: Mon, 8 Oct 2018 14:20:32 +0800 Subject: [PATCH 2/8] Add test for _asyncgen_finalizer_hook in normal conditions. Tests whether async generators are finalized correctly when garbage collected in the same thread. --- Lib/test/test_asyncio/test_base_events.py | 48 +++++++++++++++++++++++ 1 file changed, 48 insertions(+) diff --git a/Lib/test/test_asyncio/test_base_events.py b/Lib/test/test_asyncio/test_base_events.py index d15a9c6a81399a..ec13bf1feb7de1 100644 --- a/Lib/test/test_asyncio/test_base_events.py +++ b/Lib/test/test_asyncio/test_base_events.py @@ -926,6 +926,54 @@ def test_run_forever_pre_stopped(self): self.loop.run_forever() self.loop._selector.select.assert_called_once_with(0) + async def leave_unfinalized_asyncgen(self): + # The following should create an async generator, iterate + # it partially, and leave it to be garbage collected + # in the future. + status = {'started': False, + 'stopped': False, + 'finalized': False} + + async def agen(): + status['started'] = True + try: + for item in ['ZERO', 'ONE', 'TWO', 'THREE', 'FOUR']: + yield item + finally: + status['finalized'] = True + + ag = agen() + ai = ag.__aiter__() + + async def iter_one(): + try: + item = await ai.__anext__() + except StopAsyncIteration: + return + if item == 'THREE': + status['stopped'] = True + return + asyncio.create_task(iter_one()) + + asyncio.create_task(iter_one()) + return status + + def test__asyncgen_finalizer_hook_by_gc(self): + # Test for activation of _asyncgen_finalizer_hook when an + # async generator is garbage collected. + self.loop._process_events = mock.Mock() + self.loop._write_to_self = mock.Mock() + with support.disable_gc(): + status = self.loop.run_until_complete(self.leave_unfinalized_asyncgen()) + while not status['stopped']: + test_utils.run_briefly(self.loop) + self.assertTrue(status['started']) + self.assertTrue(status['stopped']) + self.assertFalse(status['finalized']) + support.gc_collect() + test_utils.run_briefly(self.loop) + self.assertTrue(status['finalized']) + class MyProto(asyncio.Protocol): done = None From 6d4e729f6bc6cf236f00aced811920fdba936299 Mon Sep 17 00:00:00 2001 From: twisteroid ambassador Date: Mon, 8 Oct 2018 15:41:08 +0800 Subject: [PATCH 3/8] Fix whitespace. --- Lib/test/test_asyncio/test_base_events.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/Lib/test/test_asyncio/test_base_events.py b/Lib/test/test_asyncio/test_base_events.py index ec13bf1feb7de1..1e82cec5035395 100644 --- a/Lib/test/test_asyncio/test_base_events.py +++ b/Lib/test/test_asyncio/test_base_events.py @@ -928,12 +928,12 @@ def test_run_forever_pre_stopped(self): async def leave_unfinalized_asyncgen(self): # The following should create an async generator, iterate - # it partially, and leave it to be garbage collected + # it partially, and leave it to be garbage collected # in the future. - status = {'started': False, + status = {'started': False, 'stopped': False, 'finalized': False} - + async def agen(): status['started'] = True try: From 0c47d4b36224aaa481bb2e14eea0ed571887e2d4 Mon Sep 17 00:00:00 2001 From: twisteroid ambassador Date: Tue, 9 Oct 2018 10:50:41 +0800 Subject: [PATCH 4/8] Add comments and change test name. --- Lib/test/test_asyncio/test_base_events.py | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/Lib/test/test_asyncio/test_base_events.py b/Lib/test/test_asyncio/test_base_events.py index 1e82cec5035395..91506494caad3b 100644 --- a/Lib/test/test_asyncio/test_base_events.py +++ b/Lib/test/test_asyncio/test_base_events.py @@ -928,8 +928,12 @@ def test_run_forever_pre_stopped(self): async def leave_unfinalized_asyncgen(self): # The following should create an async generator, iterate - # it partially, and leave it to be garbage collected - # in the future. + # it partially, and leave it to be garbage collected. + # This depends on implementation details of the garbage + # collector, and may stop working in future versions of + # cpython or in other implementations. In that case, + # the tests below on async generator finalization may + # break as well. status = {'started': False, 'stopped': False, 'finalized': False} @@ -958,9 +962,8 @@ async def iter_one(): asyncio.create_task(iter_one()) return status - def test__asyncgen_finalizer_hook_by_gc(self): - # Test for activation of _asyncgen_finalizer_hook when an - # async generator is garbage collected. + def test_asyncgen_finalization_by_gc(self): + # Async generators should be finalized when garbage collected. self.loop._process_events = mock.Mock() self.loop._write_to_self = mock.Mock() with support.disable_gc(): From 7c1a268d409e0d789238bcfc6ebfe14b0fa6bb67 Mon Sep 17 00:00:00 2001 From: twisteroid ambassador Date: Tue, 9 Oct 2018 10:51:17 +0800 Subject: [PATCH 5/8] Add test for buggy case: running gc in other thread. --- Lib/test/test_asyncio/test_base_events.py | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/Lib/test/test_asyncio/test_base_events.py b/Lib/test/test_asyncio/test_base_events.py index 91506494caad3b..93f39bfba6ea7c 100644 --- a/Lib/test/test_asyncio/test_base_events.py +++ b/Lib/test/test_asyncio/test_base_events.py @@ -977,6 +977,24 @@ def test_asyncgen_finalization_by_gc(self): test_utils.run_briefly(self.loop) self.assertTrue(status['finalized']) + def test_asyncgen_finalization_by_gc_in_other_thread(self): + # Python issue 34769: If garbage collector runs in another + # thread, async generators will not finalize in debug + # mode. + self.loop._process_events = mock.Mock() + self.loop._write_to_self = mock.Mock() + self.loop.set_debug(True) + with support.disable_gc(): + status = self.loop.run_until_complete(self.leave_unfinalized_asyncgen()) + while not status['stopped']: + test_utils.run_briefly(self.loop) + self.assertTrue(status['started']) + self.assertTrue(status['stopped']) + self.assertFalse(status['finalized']) + self.loop.run_in_executor(None, support.gc_collect) + test_utils.run_briefly(self.loop) + self.assertTrue(status['finalized']) + class MyProto(asyncio.Protocol): done = None From 1390ea232e6ecd878ed11be40f1f30ae98429d70 Mon Sep 17 00:00:00 2001 From: twisteroid ambassador Date: Tue, 9 Oct 2018 11:01:42 +0800 Subject: [PATCH 6/8] Add NEWS entry. --- .../next/Library/2018-10-09-11-01-16.bpo-34769.cSkkZt.rst | 2 ++ 1 file changed, 2 insertions(+) create mode 100644 Misc/NEWS.d/next/Library/2018-10-09-11-01-16.bpo-34769.cSkkZt.rst diff --git a/Misc/NEWS.d/next/Library/2018-10-09-11-01-16.bpo-34769.cSkkZt.rst b/Misc/NEWS.d/next/Library/2018-10-09-11-01-16.bpo-34769.cSkkZt.rst new file mode 100644 index 00000000000000..53d390354a0b26 --- /dev/null +++ b/Misc/NEWS.d/next/Library/2018-10-09-11-01-16.bpo-34769.cSkkZt.rst @@ -0,0 +1,2 @@ +Fix for async generators not finalizing when event loop in debug mode and +garbage collector runs in another thread. From b2795e3d744585984e504d140adc8058ff77ccf9 Mon Sep 17 00:00:00 2001 From: twisteroid ambassador Date: Tue, 9 Oct 2018 11:35:42 +0800 Subject: [PATCH 7/8] Fix a forgotten await. --- Lib/test/test_asyncio/test_base_events.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/Lib/test/test_asyncio/test_base_events.py b/Lib/test/test_asyncio/test_base_events.py index 93f39bfba6ea7c..db5c0c816970eb 100644 --- a/Lib/test/test_asyncio/test_base_events.py +++ b/Lib/test/test_asyncio/test_base_events.py @@ -991,7 +991,8 @@ def test_asyncgen_finalization_by_gc_in_other_thread(self): self.assertTrue(status['started']) self.assertTrue(status['stopped']) self.assertFalse(status['finalized']) - self.loop.run_in_executor(None, support.gc_collect) + self.loop.run_until_complete( + self.loop.run_in_executor(None, support.gc_collect)) test_utils.run_briefly(self.loop) self.assertTrue(status['finalized']) From 4bdd520b4255812ca499c7e55a46777e4d5d0fab Mon Sep 17 00:00:00 2001 From: twisteroid ambassador Date: Tue, 9 Oct 2018 14:17:19 +0800 Subject: [PATCH 8/8] Fix typo and condense comments. --- Lib/test/test_asyncio/test_base_events.py | 12 +++++------- .../Library/2018-10-09-11-01-16.bpo-34769.cSkkZt.rst | 2 +- 2 files changed, 6 insertions(+), 8 deletions(-) diff --git a/Lib/test/test_asyncio/test_base_events.py b/Lib/test/test_asyncio/test_base_events.py index db5c0c816970eb..6d544d1eda8635 100644 --- a/Lib/test/test_asyncio/test_base_events.py +++ b/Lib/test/test_asyncio/test_base_events.py @@ -927,13 +927,11 @@ def test_run_forever_pre_stopped(self): self.loop._selector.select.assert_called_once_with(0) async def leave_unfinalized_asyncgen(self): - # The following should create an async generator, iterate - # it partially, and leave it to be garbage collected. - # This depends on implementation details of the garbage - # collector, and may stop working in future versions of - # cpython or in other implementations. In that case, - # the tests below on async generator finalization may - # break as well. + # Create an async generator, iterate it partially, and leave it + # to be garbage collected. + # Used in async generator finalization tests. + # Depends on implementation details of garbage collector. Changes + # in gc may break this function. status = {'started': False, 'stopped': False, 'finalized': False} diff --git a/Misc/NEWS.d/next/Library/2018-10-09-11-01-16.bpo-34769.cSkkZt.rst b/Misc/NEWS.d/next/Library/2018-10-09-11-01-16.bpo-34769.cSkkZt.rst index 53d390354a0b26..fc034c962ea2e1 100644 --- a/Misc/NEWS.d/next/Library/2018-10-09-11-01-16.bpo-34769.cSkkZt.rst +++ b/Misc/NEWS.d/next/Library/2018-10-09-11-01-16.bpo-34769.cSkkZt.rst @@ -1,2 +1,2 @@ -Fix for async generators not finalizing when event loop in debug mode and +Fix for async generators not finalizing when event loop is in debug mode and garbage collector runs in another thread.