-
-
Notifications
You must be signed in to change notification settings - Fork 31.6k
Deprecate task.set_result() and task.set_exception() #76544
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
Comments
Task result is given from coroutine execution, explicit modification of the value should be deprecated and eventually removed. |
+1. |
Actually, I don't think we need a deprecation period. Both methods are completely unusable now. Given: async def foo():
await asyncio.sleep(1)
print('AAAAA')
return 42
async def main():
f = asyncio.create_task(foo())
f.set_result(123)
result = await f
print(result)
asyncio.run(main()) This will print "123", but will give us an "AssertionError: _step(): already done: <Task finished coro=<foo() running at t.py:4> result=123> None" logged.
async def main():
f = asyncio.create_task(foo())
await asyncio.sleep(2)
f.set_result(123)
result = await f
print(result)
asyncio.run(main()) This just crashes with an InvalidStateError.
All in all, deprecation periods are useful when there's some working and useful functionality that has to be removed in the future. In this case there's no working and no useful functionality. So let's just make Task.set_result() and Task.set_exception() raise NotImplementedError. |
About the only use case I can come up with is subclassing asyncio.Task and overriding set_result and set_exception implementations. This use case has been broken (unintentionally) in Python 3.6, when we first implemented Task in _asynciomodule.c. C Task doesn't try to resolve 'self.set_result()' or 'self.set_exception()'. It calls the C "super" implementations instead. |
Andrew, Yury I test my lib against dev versions of Python and recently got an error in one of the tests due to the deprecation. I do not argue the reason behind removing this methods, but Task.set_exception was working for me in tests: https://github.com/Kentzo/async_app/blob/250ee7a05d2af8035806ce1d86f57d0f00283db0/tests/test_utils.py#L73-L91 My use case was outside control of otherwise unconditionally blocking task (for tests only). What replacement (if any) would you suggest? |
Use Task.cancel() or use a Queue to communicate with the task. Your test code was working, but ultimately was creating an unexpected (and not officially documented/supported) situation for the task. |
cancel will work in my case, but it's somewhat limited. In other hand it's probably a job of a testing library to provide an utility function. |
One reason why Task.cancel() is an incomplete replacement for Task.set_exception() is that you don't have an easy way to communicate why the task was ended. With set_exception() and set_result(), you could. Task.cancel(), though, doesn't let you e.g. specify a CancelledError subclass or "reason" string (see the related bpo-35674, "Add argument to .cancel() of Task and Future"). |
Correction: that should have been bpo-31033. |
The result of the Task is bound to the result of the coroutine it wraps. If the coroutine raises an exception -- that's the exception of the Task object. If the coroutine returns a value -- that's the result value of the Task object. If the coroutine is cancelled via the "Task.cancel()" call -- asyncio.CancelledError is likely the result of the Task. Key rule: the wrapped coroutine dictates the result of the Task, not the other way around. The Task can signal cancellation, but even then, the final result is up to the coroutine. There's no clear reason to complicate the Task<->coroutine relationship by allowing to inject arbitrary exceptions into running coroutines. |
A second reason why Task.cancel() seems to be an incomplete replacement: Task.set_exception() and Task.set_result() both give you a way to unconditionally end a task. With cancel() though, the docs say, "Task.cancel() does not guarantee that the Task will be cancelled." 1 The reason you might want to unconditionally end a task is if e.g. you already called Task.cancel() and it is still running after waiting a certain amount of time. |
My comment was more about CancelledError rather than arbitrary exceptions. You didn't reply to the part of my response saying that Task.cancel() doesn't let you communicate why the task was ended, which is something the removed API's let you do. |
I didn't reply because you didn't clearly articulate why the cancellation reason is important :) AFAIK no other async framework allows you to do this. So what are those real-world cases that can explain why asyncio should support this functionality?
Unfortunately they didn't. They never worked properly; IIRC Task.set_exception had never worked at all. The fact they were available at all was a simple oversight.
Yes, because Task can only signal its coroutine that it was requested to be cancelled. The coroutine may choose to ignore that by swallowing CancelledError.
Well, if you want to unconditionally end tasks you shouldn't write coroutines that ignore CancelledErrors. If you do, then you clearly *do not want* the coroutine to be cancelled, and thus there's no use case for set_exception. Finally, asyncio has queues, which can be used to build two-way communication channels between coroutines and implement whatever advanced cancellation mechanism you want. |
Well, of course. But code can have bugs, and maybe you didn't write the coroutine because it's from a library that you don't control. In that case, you should still be able to end the task. Also, maybe the coroutine doesn't have a bug but the cancellation is just taking longer than you have time for, so you want to end it early.
The case I had in mind was the one I referenced above -- being able to distinguish a normal CancelledError from one where you had interrupt the cancellation (i.e. cancellation timing out). I would like the caller to be able to know when the latter are happening. |
I agree with Yuri.
The same situation is possible with classic threads: killing a thread without unwinding a call stack leads to locked mutexes etc. Regarding distinguishing explicit cancellation from timeout exhausting: it can be done with current asyncio design by using a flag. Take a look on async_timeout context manager, __aexit__() implementation: https://github.com/aio-libs/async-timeout/blob/master/async_timeout/__init__.py#L88-L97 |
To stop the discussion from happening in two places (sorry, Yury), I started a broader discussion on Async-sig with thread starting here: https://mail.python.org/pipermail/async-sig/2019-February/000548.html |
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: