GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | |||||||||||||||||||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | |||||||||||||||||||||||||||||||
GeezerVison is showing github.com — gh-105836: Fix asyncio.run_coroutine_threadsafe leaving underlying cancelled asyncio task running by iaalm · Pull Request #141696 · python/cpython Conversation
|
|
Hi @gvanrossum,
Regarding the asyncio.sleep(0), the example in gh-105836 uses Thread.run() which actually run the function in main thread (maybe by typo?). In such cases, because future.cancel() is a sync function running in loop. There's no good way in my opinion to wait for the cancellation in future.cancel(). So one asyncio.sleep(0) is always needed to execute the cancellation work. I think that is also the reason all test in test_asyncio.test_tasks.RunCoroutineThreadsafeTests needs a asyncio.sleep(0) after cancel. Does that make sense? |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Nearly there! Thanks for persevering.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
|
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
|
will investigate why the case fail on windows tomorrow (need to find a windows device😝) |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| import random | ||
| import re | ||
| import sys | ||
| from threading import Event |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
I'd move this import into the test function -- otherwise it could be confused with asyncio.Event.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| # self.loop.run_in_executor(None, _in_another_thread) | ||
| thread_future = asyncio.run_coroutine_threadsafe(self.add(1, 2), self.loop) | ||
| await asyncio.sleep(0) | ||
| target_started = Event() |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
I'd use import threading followed by target_started = threading.Event() to be super clear about what kind of event this is.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Now the tests pass, I'm approving this.
I just want @kumaraditya303 to have a look as well.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
I have improved the test to not rely on Running Debug|x64 interpreter... == CPython 3.15.0a1+ (heads/cancel_in_loop:787c9fc5de0, Dec 6 2025, 20:45:00) [MSC v.1944 64 bit (AMD64)] == Windows-11-10.0.26200-SP0 little-endian == Python build: debug == cwd: D:\cpython\build\test_python_worker_24356æ == CPU count: 12 == encodings: locale=cp1252 FS=utf-8 == resources: all test resources are disabled, use -u option to unskip tests Using random seed: 1952231759 0:00:00 Run 1 test sequentially in a single process 0:00:00 [1/1] test_asyncio.test_tasks test_run_coroutine_threadsafe_and_cancel (test.test_asyncio.test_tasks.RunCoroutineThreadsafeTests.test_run_coroutine_threadsafe_and_cancel) ... FAIL ====================================================================== FAIL: test_run_coroutine_threadsafe_and_cancel (test.test_asyncio.test_tasks.RunCoroutineThreadsafeTests.test_run_coroutine_threadsafe_and_cancel) ---------------------------------------------------------------------- Traceback (most recent call last): File "D:\cpython\Lib\test\test_asyncio\test_tasks.py", line 3704, in test_run_coroutine_threadsafe_and_cancel self.assertTrue(task.cancelled()) ~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^ AssertionError: False is not true ---------------------------------------------------------------------- Ran 1 test in 0.014s FAILED (failures=1) Task was destroyed but it is pending! task: <Task cancelling name='Task-2' coro=<sleep() done, defined at D:\cpython\Lib\asyncio\tasks.py:687> wait_for=<Future cancelled> cb=[_chain_future.<locals>._call_set_state() at D:\cpython\Lib\asyncio\futures.py:397]> test test_asyncio.test_tasks failed 0:00:00 [1/1/1] test_asyncio.test_tasks failed (1 failure) == Tests result: FAILURE == 1 test failed: test_asyncio.test_tasks Total duration: 542 ms Total tests: run=1 (filtered) failures=1 Total test files: run=1/1 (filtered) failed=1 Result: FAILURE |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Thanks @kumaraditya303, that looks much better!
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
|
Thanks @iaalm for the PR, and @kumaraditya303 for merging it 🌮🎉.. I'm working now to backport this PR to: 3.13, 3.14. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
…lying cancelled asyncio task running (pythonGH-141696) (cherry picked from commit 14715e3) Co-authored-by: Kaisheng Xu <[email protected]> Co-authored-by: Kumar Aditya <[email protected]>
…lying cancelled asyncio task running (pythonGH-141696) (cherry picked from commit 14715e3) Co-authored-by: Kaisheng Xu <[email protected]> Co-authored-by: Kumar Aditya <[email protected]>
|
GH-142358 is a backport of this pull request to the 3.14 branch. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
GH-142359 is a backport of this pull request to the 3.13 branch. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
…rlying cancelled asyncio task running (GH-141696) (#142359) gh-105836: Fix `asyncio.run_coroutine_threadsafe` leaving underlying cancelled asyncio task running (GH-141696) (cherry picked from commit 14715e3) Co-authored-by: Kaisheng Xu <[email protected]> Co-authored-by: Kumar Aditya <[email protected]>
…rlying cancelled asyncio task running (GH-141696) (#142358) gh-105836: Fix `asyncio.run_coroutine_threadsafe` leaving underlying cancelled asyncio task running (GH-141696) (cherry picked from commit 14715e3) Co-authored-by: Kaisheng Xu <[email protected]> Co-authored-by: Kumar Aditya <[email protected]>
There was an error while loading. Please reload this page.
Successfully merging this pull request may close these issues.
There was an error while loading. Please reload this page.