-
-
Notifications
You must be signed in to change notification settings - Fork 33.6k
gh-105836: Fix asyncio.run_coroutine_threadsafe leaving underlying cancelled asyncio task running #141696
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
gvanrossum
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
I tried this with the example from gh-105836. This indeed fixes it, but still requires the await asyncio.sleep(0) after the shutdown() call. (Also note that the example passes if there are two sleep(0) calls.)
Is there a way to do better?
Also, probably that example should be added as a unit test (preferably without long sleeps).
|
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? |
gvanrossum
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nearly there! Thanks for persevering.
|
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 |
|
will investigate why the case fail on windows tomorrow (need to find a windows device😝) |
Lib/test/test_asyncio/test_tasks.py
Outdated
| import random | ||
| import re | ||
| import sys | ||
| from threading import Event |
There was a problem hiding this comment.
Choose a reason for 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.
Lib/test/test_asyncio/test_tasks.py
Outdated
| # 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.
Choose a reason for 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.
Thank you! Things have been hectic on my end. Come back to the PR now. |
|
@iaalm Alas, the Windows tests still fail. Can you look into that? I suspect the timing is different because of IOCP (proactor_loop). |
You're right. I tried it on a Windows VM. The best fix I can see is filter out BaseProactorEventLoop._loop_self_reading which seems always there in _ready while counting tasks. |
gvanrossum
left a comment
There was a problem hiding this comment.
Choose a reason for 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.
|
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
|
gvanrossum
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @kumaraditya303, that looks much better!
Misc/NEWS.d/next/Library/2025-11-18-15-48-13.gh-issue-105836.sbUw24.rst
Outdated
Show resolved
Hide resolved
|
Thanks @iaalm for the PR, and @kumaraditya303 for merging it 🌮🎉.. I'm working now to backport this PR to: 3.13, 3.14. |
…lying cancelled asyncio task running (pythonGH-141696) (cherry picked from commit 14715e3a64a674629c781d4a3dd11143ba010990) Co-authored-by: Kaisheng Xu <iaalmsimon@gmail.com> Co-authored-by: Kumar Aditya <kumaraditya@python.org>
…lying cancelled asyncio task running (pythonGH-141696) (cherry picked from commit 14715e3) Co-authored-by: Kaisheng Xu <iaalmsimon@gmail.com> Co-authored-by: Kumar Aditya <kumaraditya@python.org>
|
GH-142358 is a backport of this pull request to the 3.14 branch. |
|
GH-142359 is a backport of this pull request to the 3.13 branch. |
_chain_futurewhich supportsconcurrent.futures.Futureandasycio.Future. Whileasyncio.run_coroutine_threadsafegivesdestinationaconcurrent.futures.Future, thedest_loopis None. Thendest_loop is source_loopis not good enough for the check. We should always the current running loop for the future operating.concurrent.futures.Futurecreated byasyncio.run_coroutine_threadsafe, we need to wait for the cancellation before return. That's what I done in_cancel_future_in_loop.