-
-
Notifications
You must be signed in to change notification settings - Fork 32.3k
bpo-32363: Disable Task.set_exception() and Task.set_result() #4923
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
Lib/asyncio/tasks.py
Outdated
raise NotImplementedError | ||
|
||
def set_exception(self, exception): | ||
raise NotImplementedError |
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.
It's backward incompatible change.
I know people use task.set_exception()
in production code, heard about it on conference couple months ago.
I propose raising a deprecation warning in 3.7 and explicit exception (I thought aboutRuntimeError
but NotImplementedeError
is also good maybe) in 3.8.
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.
But it's impossible to use it, i've shown that in the issue. That code is almost certainly broken. Maybe it's OK to force people to fix nonsense code?
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.
Let's pause for day or two: I'm going to wrote email to that guy for clarification.
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.
Maybe @kozzztik can comment his use case.
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.
Hi! Thats correct, we used it on prod for monitoring purposes. However as said above it was broken, thats what I said on conference. When you try to inherit from Task on Py>3.6 (on 3.5 it is ok) that is not obvious, but you inherit CTask which not uses this method.
However, I guess why method raises "Not implemented" and called with magic super just bellow declaration. It looks strange. I see two solutions that looks better:
- Just remove it (may be also make C implementation be similar to python variant and respect method overriding)
- Add comment to "raise" statement why it looks so. Sure, someone will spend hours to understand what that mean.
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.
@kozzztik Thanks for the explanation!
When you try to inherit from Task on Py>3.6 (on 3.5 it is ok) that is not obvious, but you inherit CTask which not uses this method.
@asvetlov As I expected. It's not possible to use either method since 3.6.
Just remove it (may be also make C implementation be similar to python variant and respect method overriding)
Can't do that, Task inherits from Future. It's possible to remove them from C Task, but not from Python version.
Add comment to "raise" statement why it looks so. Sure, someone will spend hours to understand what that mean.
OK.
I'll change NotImplementedError
to some other exception.
Lib/asyncio/tasks.py
Outdated
|
||
def _repr_info(self): | ||
return base_tasks._task_repr_info(self) | ||
|
||
def set_result(self, result): | ||
raise NotImplementedError |
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.
Backward incompatible.
Lib/asyncio/tasks.py
Outdated
|
||
def _repr_info(self): | ||
return base_tasks._task_repr_info(self) | ||
|
||
def set_result(self, result): | ||
raise RuntimeError('Tasks do not support set_result operation') |
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.
Task does not support ...
sounds slightly better to me but I don't insist.
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.
done
ping |
f645e9d
to
e11cf73
Compare
https://bugs.python.org/issue32363