From c1649a91552528228c95e95953aa7299c283de1c Mon Sep 17 00:00:00 2001 From: twisteroid ambassador Date: Tue, 25 Oct 2022 01:06:00 +0800 Subject: [PATCH 1/6] Add tests for wait_for() behavior when things happen simultaneously. --- Lib/test/test_asyncio/test_waitfor.py | 64 +++++++++++++++++++++++++++ 1 file changed, 64 insertions(+) diff --git a/Lib/test/test_asyncio/test_waitfor.py b/Lib/test/test_asyncio/test_waitfor.py index 45498fa097f6bc..6371ffcde55c76 100644 --- a/Lib/test/test_asyncio/test_waitfor.py +++ b/Lib/test/test_asyncio/test_waitfor.py @@ -289,6 +289,70 @@ async def test_cancel_blocking_wait_for(self): async def test_cancel_wait_for(self): await self._test_cancel_wait_for(60.0) + async def simultaneous_self_cancel_and_inner_result( + self, + do_cancel, + waitfor_timeout, + inner_action, + ): + """Construct scenario where external cancellation, exceeding timeout and + awaitable becoming done all happen simultaneously. + inner_acion is one of 'cancel', 'exception' or 'result'. + if do_cancel, the wait_for() call is cancelled at the same time when + inner_action is executed. + If waitfor_timeout == 0.1, then the timeout should happen at the same + time as well.""" + loop = asyncio.get_running_loop() + inner = loop.create_future() + waitfor_task = asyncio.create_task( + asyncio.wait_for(inner, timeout=waitfor_timeout)) + await asyncio.sleep(0.1) + if inner_action == 'cancel': + inner.cancel() + elif inner_action == 'exception': + inner.set_exception(RuntimeError('inner exception')) + else: + assert inner_action == 'result' + inner.set_result('inner result') + waitfor_task.cancel() + return await waitfor_task + + async def test_simultaneous_self_cancel_and_inner_result(self): + for timeout in (0.1, 10, None): + with self.subTest(waitfor_timeout=timeout): + with self.assertRaises(asyncio.CancelledError): + await self.simultaneous_self_cancel_and_inner_result( + True, timeout, 'result') + + async def test_simultaneous_self_cancel_and_inner_exc(self): + for timeout in (0.1, 10, None): + with self.subTest(waitfor_timeout=timeout): + with self.assertRaises(RuntimeError): + await self.simultaneous_self_cancel_and_inner_result( + True, timeout, 'exception') + + async def test_simultaneous_self_cancel_and_inner_cancel(self): + for timeout in (0.1, 10, None): + with self.subTest(waitfor_timeout=timeout): + with self.assertRaises(asyncio.CancelledError): + await self.simultaneous_self_cancel_and_inner_result( + True, timeout, 'cancel') + + async def test_simultaneous_inner_result_and_timeout(self): + result = await self.simultaneous_self_cancel_and_inner_result( + False, 0.1, 'result') + self.assertEqual(result, 'inner result') + + async def test_simultaneous_inner_exc_and_timeout(self): + with self.assertRaises(RuntimeError): + await self.simultaneous_self_cancel_and_inner_result( + False, 0.1, 'exception') + + async def test_simultaneous_inner_cancel_and_timeout(self): + with self.assertRaises(asyncio.CancelledError): + await self.simultaneous_self_cancel_and_inner_result( + False, 0.1, 'cancel') + if __name__ == '__main__': unittest.main() From 992d843a1470b357d9eeb736c8adc1b5a97d5590 Mon Sep 17 00:00:00 2001 From: twisteroid ambassador Date: Tue, 25 Oct 2022 01:25:11 +0800 Subject: [PATCH 2/6] Actually use do_cancel argument. --- Lib/test/test_asyncio/test_waitfor.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/Lib/test/test_asyncio/test_waitfor.py b/Lib/test/test_asyncio/test_waitfor.py index 6371ffcde55c76..924fba7dc1e428 100644 --- a/Lib/test/test_asyncio/test_waitfor.py +++ b/Lib/test/test_asyncio/test_waitfor.py @@ -314,7 +314,8 @@ async def simultaneous_self_cancel_and_inner_result( else: assert inner_action == 'result' inner.set_result('inner result') - waitfor_task.cancel() + if do_cancel: + waitfor_task.cancel() return await waitfor_task async def test_simultaneous_self_cancel_and_inner_result(self): From 55f5547c704651423739449afb6ff8527d33b5d4 Mon Sep 17 00:00:00 2001 From: twisteroid ambassador Date: Tue, 25 Oct 2022 14:08:57 +0800 Subject: [PATCH 3/6] Ensure wait_for() does not swallow cancellation. --- Lib/asyncio/tasks.py | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/Lib/asyncio/tasks.py b/Lib/asyncio/tasks.py index 571013745aa03a..13ddd2c8047326 100644 --- a/Lib/asyncio/tasks.py +++ b/Lib/asyncio/tasks.py @@ -468,15 +468,16 @@ async def wait_for(fut, timeout): try: await waiter except exceptions.CancelledError: - if fut.done(): - return fut.result() - else: - fut.remove_done_callback(cb) + if not fut.done(): # We must ensure that the task is not running # after wait_for() returns. # See https://bugs.python.org/issue32751 + fut.remove_done_callback(cb) await _cancel_and_wait(fut, loop=loop) - raise + exc = fut.exception() + if exc: # If fut.cancelled(), this will raise CancelledError + raise exc + raise if fut.done(): return fut.result() From c834122db8f08928b2e555801314f877c7bfa5d2 Mon Sep 17 00:00:00 2001 From: twisteroid ambassador Date: Wed, 26 Oct 2022 13:42:01 +0800 Subject: [PATCH 4/6] Remove tests for simultaneous *timeout* and *inner becoming done*. The existing method of setting identical timeout / sleep lengths is not reliable. --- Lib/test/test_asyncio/test_waitfor.py | 35 ++++++++++----------------- 1 file changed, 13 insertions(+), 22 deletions(-) diff --git a/Lib/test/test_asyncio/test_waitfor.py b/Lib/test/test_asyncio/test_waitfor.py index 924fba7dc1e428..bb48223f4dc751 100644 --- a/Lib/test/test_asyncio/test_waitfor.py +++ b/Lib/test/test_asyncio/test_waitfor.py @@ -295,18 +295,24 @@ async def simultaneous_self_cancel_and_inner_result( waitfor_timeout, inner_action, ): - """Construct scenario where external cancellation, exceeding timeout and - awaitable becoming done all happen simultaneously. + """Construct scenario where external cancellation and + awaitable becoming done happen simultaneously. inner_acion is one of 'cancel', 'exception' or 'result'. if do_cancel, the wait_for() call is cancelled at the same time when inner_action is executed. - If waitfor_timeout == 0.1, then the timeout should happen at the same - time as well.""" + Make sure waitfor_timeout > 0.1. Trying to make it == 0.1 is not + a reliable way to make timeout happen at the same time as the above. + """ loop = asyncio.get_running_loop() inner = loop.create_future() waitfor_task = asyncio.create_task( asyncio.wait_for(inner, timeout=waitfor_timeout)) await asyncio.sleep(0.1) + # Even if waitfor_timeout == 0.1, there's still no guarantee whether the + # timer handler (or similar) in wait_for() or code below in this + # coroutine executes first. If the timer handler executes first, then + # inner will be cancelled(), and code below will raise + # InvalidStateError. if inner_action == 'cancel': inner.cancel() elif inner_action == 'exception': @@ -319,41 +325,26 @@ async def simultaneous_self_cancel_and_inner_result( return await waitfor_task async def test_simultaneous_self_cancel_and_inner_result(self): - for timeout in (0.1, 10, None): + for timeout in (10, None): with self.subTest(waitfor_timeout=timeout): with self.assertRaises(asyncio.CancelledError): await self.simultaneous_self_cancel_and_inner_result( True, timeout, 'result') async def test_simultaneous_self_cancel_and_inner_exc(self): - for timeout in (0.1, 10, None): + for timeout in (10, None): with self.subTest(waitfor_timeout=timeout): with self.assertRaises(RuntimeError): await self.simultaneous_self_cancel_and_inner_result( True, timeout, 'exception') async def test_simultaneous_self_cancel_and_inner_cancel(self): - for timeout in (0.1, 10, None): + for timeout in (10, None): with self.subTest(waitfor_timeout=timeout): with self.assertRaises(asyncio.CancelledError): await self.simultaneous_self_cancel_and_inner_result( True, timeout, 'cancel') - async def test_simultaneous_inner_result_and_timeout(self): - result = await self.simultaneous_self_cancel_and_inner_result( - False, 0.1, 'result') - self.assertEqual(result, 'inner result') - - async def test_simultaneous_inner_exc_and_timeout(self): - with self.assertRaises(RuntimeError): - await self.simultaneous_self_cancel_and_inner_result( - False, 0.1, 'exception') - - async def test_simultaneous_inner_cancel_and_timeout(self): - with self.assertRaises(asyncio.CancelledError): - await self.simultaneous_self_cancel_and_inner_result( - False, 0.1, 'cancel') - if __name__ == '__main__': unittest.main() From 7660179b100494d47efb316ca575c0af4ad04f1b Mon Sep 17 00:00:00 2001 From: twisteroid ambassador Date: Wed, 26 Oct 2022 14:08:13 +0800 Subject: [PATCH 5/6] Spec change: when cancelled, always raise CancelledError. This means swallowing the inner future's result / exception, if they are set simultaneously with the external cancellation. --- Lib/asyncio/tasks.py | 3 --- Lib/test/test_asyncio/test_waitfor.py | 28 +++++++++++++-------------- 2 files changed, 13 insertions(+), 18 deletions(-) diff --git a/Lib/asyncio/tasks.py b/Lib/asyncio/tasks.py index 13ddd2c8047326..e322f81d692039 100644 --- a/Lib/asyncio/tasks.py +++ b/Lib/asyncio/tasks.py @@ -474,9 +474,6 @@ async def wait_for(fut, timeout): # See https://bugs.python.org/issue32751 fut.remove_done_callback(cb) await _cancel_and_wait(fut, loop=loop) - exc = fut.exception() - if exc: # If fut.cancelled(), this will raise CancelledError - raise exc raise if fut.done(): diff --git a/Lib/test/test_asyncio/test_waitfor.py b/Lib/test/test_asyncio/test_waitfor.py index bb48223f4dc751..a428ec2d9a0fd6 100644 --- a/Lib/test/test_asyncio/test_waitfor.py +++ b/Lib/test/test_asyncio/test_waitfor.py @@ -291,15 +291,12 @@ async def test_cancel_wait_for(self): async def simultaneous_self_cancel_and_inner_result( self, - do_cancel, waitfor_timeout, inner_action, ): """Construct scenario where external cancellation and awaitable becoming done happen simultaneously. inner_acion is one of 'cancel', 'exception' or 'result'. - if do_cancel, the wait_for() call is cancelled at the same time when - inner_action is executed. Make sure waitfor_timeout > 0.1. Trying to make it == 0.1 is not a reliable way to make timeout happen at the same time as the above. """ @@ -320,30 +317,31 @@ async def simultaneous_self_cancel_and_inner_result( else: assert inner_action == 'result' inner.set_result('inner result') - if do_cancel: - waitfor_task.cancel() - return await waitfor_task + waitfor_task.cancel() + with self.assertRaises(asyncio.CancelledError): + return await waitfor_task + # Consume inner's exception, to avoid "Future exception was never + # retrieved" messages + if inner_action == 'exception': + self.assertIsInstance(inner.exception(), RuntimeError) async def test_simultaneous_self_cancel_and_inner_result(self): for timeout in (10, None): with self.subTest(waitfor_timeout=timeout): - with self.assertRaises(asyncio.CancelledError): - await self.simultaneous_self_cancel_and_inner_result( - True, timeout, 'result') + await self.simultaneous_self_cancel_and_inner_result( + timeout, 'result') async def test_simultaneous_self_cancel_and_inner_exc(self): for timeout in (10, None): with self.subTest(waitfor_timeout=timeout): - with self.assertRaises(RuntimeError): - await self.simultaneous_self_cancel_and_inner_result( - True, timeout, 'exception') + await self.simultaneous_self_cancel_and_inner_result( + timeout, 'exception') async def test_simultaneous_self_cancel_and_inner_cancel(self): for timeout in (10, None): with self.subTest(waitfor_timeout=timeout): - with self.assertRaises(asyncio.CancelledError): - await self.simultaneous_self_cancel_and_inner_result( - True, timeout, 'cancel') + await self.simultaneous_self_cancel_and_inner_result( + timeout, 'cancel') if __name__ == '__main__': From 02a624c18db2b8dd249e7dd051992b75b8bc81f0 Mon Sep 17 00:00:00 2001 From: "blurb-it[bot]" <43283697+blurb-it[bot]@users.noreply.github.com> Date: Wed, 26 Oct 2022 06:52:00 +0000 Subject: [PATCH 6/6] =?UTF-8?q?=F0=9F=93=9C=F0=9F=A4=96=20Added=20by=20blu?= =?UTF-8?q?rb=5Fit.?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../next/Library/2022-10-26-06-51-59.gh-issue-86296.l1MdXh.rst | 1 + 1 file changed, 1 insertion(+) create mode 100644 Misc/NEWS.d/next/Library/2022-10-26-06-51-59.gh-issue-86296.l1MdXh.rst diff --git a/Misc/NEWS.d/next/Library/2022-10-26-06-51-59.gh-issue-86296.l1MdXh.rst b/Misc/NEWS.d/next/Library/2022-10-26-06-51-59.gh-issue-86296.l1MdXh.rst new file mode 100644 index 00000000000000..b37e5ec9dca050 --- /dev/null +++ b/Misc/NEWS.d/next/Library/2022-10-26-06-51-59.gh-issue-86296.l1MdXh.rst @@ -0,0 +1 @@ +Make :func:`asyncio.wait_for` not ignore external cancellation if the inner awaitable becomes done at the same time. Patch by twisteroid ambassador.