Skip to content

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

Merged
merged 3 commits into from
Dec 25, 2017

Conversation

1st1
Copy link
Member

@1st1 1st1 commented Dec 19, 2017

raise NotImplementedError

def set_exception(self, exception):
raise NotImplementedError
Copy link
Contributor

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.

Copy link
Member Author

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?

Copy link
Contributor

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.

Copy link
Contributor

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.

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:

  1. Just remove it (may be also make C implementation be similar to python variant and respect method overriding)
  2. Add comment to "raise" statement why it looks so. Sure, someone will spend hours to understand what that mean.

Copy link
Member Author

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.


def _repr_info(self):
return base_tasks._task_repr_info(self)

def set_result(self, result):
raise NotImplementedError
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Backward incompatible.


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')
Copy link
Contributor

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@asvetlov
Copy link
Contributor

ping

@1st1 1st1 force-pushed the no_task_set_methods branch from f645e9d to e11cf73 Compare December 25, 2017 15:29
@1st1 1st1 merged commit 0cf16f9 into python:master Dec 25, 2017
@1st1 1st1 deleted the no_task_set_methods branch December 25, 2017 15:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants