Skip to content
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

Deprecate task.set_result() and task.set_exception() #76544

Closed
asvetlov opened this issue Dec 18, 2017 · 17 comments
Closed

Deprecate task.set_result() and task.set_exception() #76544

asvetlov opened this issue Dec 18, 2017 · 17 comments
Labels
3.7 (EOL) end of life stdlib Python modules in the Lib dir topic-asyncio type-bug An unexpected behavior, bug, or error

Comments

@asvetlov
Copy link
Contributor

BPO 32363
Nosy @asvetlov, @cjerdonek, @1st1, @Kentzo
PRs
  • bpo-32363: Disable Task.set_exception() and Task.set_result() #4923
  • 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:

    assignee = None
    closed_at = <Date 2017-12-25.15:48:39.676>
    created_at = <Date 2017-12-18.10:59:23.929>
    labels = ['3.7', 'type-bug', 'library', 'expert-asyncio']
    title = 'Deprecate task.set_result() and task.set_exception()'
    updated_at = <Date 2019-02-19.20:43:46.189>
    user = 'https://github.com/asvetlov'

    bugs.python.org fields:

    activity = <Date 2019-02-19.20:43:46.189>
    actor = 'chris.jerdonek'
    assignee = 'none'
    closed = True
    closed_date = <Date 2017-12-25.15:48:39.676>
    closer = 'yselivanov'
    components = ['Library (Lib)', 'asyncio']
    creation = <Date 2017-12-18.10:59:23.929>
    creator = 'asvetlov'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 32363
    keywords = ['patch']
    message_count = 17.0
    messages = ['308542', '308579', '308609', '308613', '309037', '309198', '309199', '309201', '336000', '336001', '336002', '336003', '336005', '336009', '336013', '336014', '336019']
    nosy_count = 4.0
    nosy_names = ['asvetlov', 'chris.jerdonek', 'yselivanov', 'Ilya.Kulakov']
    pr_nums = ['4923']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue32363'
    versions = ['Python 3.7']

    @asvetlov
    Copy link
    Contributor Author

    Task result is given from coroutine execution, explicit modification of the value should be deprecated and eventually removed.

    @asvetlov asvetlov added 3.7 (EOL) end of life stdlib Python modules in the Lib dir topic-asyncio type-bug An unexpected behavior, bug, or error labels Dec 18, 2017
    @1st1
    Copy link
    Member

    1st1 commented Dec 18, 2017

    +1.

    @1st1
    Copy link
    Member

    1st1 commented Dec 19, 2017

    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
    1. Let's try to use Task.set_result():
      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.

    1. Let's try set_result + a sleep:
      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.

    1. Same happens with set_exception().

    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.

    @1st1
    Copy link
    Member

    1st1 commented Dec 19, 2017

    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.

    @1st1
    Copy link
    Member

    1st1 commented Dec 25, 2017

    New changeset 0cf16f9 by Yury Selivanov in branch 'master':
    bpo-32363: Disable Task.set_exception() and Task.set_result() (bpo-4923)
    0cf16f9

    @1st1 1st1 closed this as completed Dec 25, 2017
    @Kentzo
    Copy link
    Mannequin

    Kentzo mannequin commented Dec 29, 2017

    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?

    @1st1
    Copy link
    Member

    1st1 commented Dec 29, 2017

    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.

    @Kentzo
    Copy link
    Mannequin

    Kentzo mannequin commented Dec 29, 2017

    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.

    @cjerdonek
    Copy link
    Member

    Use Task.cancel() or use a Queue to communicate with the task.

    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").

    @cjerdonek
    Copy link
    Member

    Correction: that should have been bpo-31033.

    @1st1
    Copy link
    Member

    1st1 commented Feb 19, 2019

    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.

    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.

    @cjerdonek
    Copy link
    Member

    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.

    @cjerdonek
    Copy link
    Member

    There's no clear reason to complicate the Task<->coroutine relationship by allowing to inject arbitrary exceptions into running coroutines.

    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.

    @1st1
    Copy link
    Member

    1st1 commented Feb 19, 2019

    > There's no clear reason to complicate the Task<->coroutine relationship by allowing to inject arbitrary exceptions into running coroutines.

    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, [..]

    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?

    [..] which is something the removed API's let you do.

    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.

    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]

    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.

    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.

    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.

    @cjerdonek
    Copy link
    Member

    Well, if you want to unconditionally end tasks you shouldn't write coroutines that ignore CancelledErrors.

    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.

    So what are those real-world cases that can explain why asyncio should support this functionality?

    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.

    @asvetlov
    Copy link
    Contributor Author

    I agree with Yuri.

    Task.set_exception() (let's assume it works) is very dangerous: if cancellation exception doesn't bubble up from coroutine code there is a very high chance to get broken invariants and not-released resources.

    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

    @cjerdonek
    Copy link
    Member

    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

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.7 (EOL) end of life stdlib Python modules in the Lib dir topic-asyncio type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    3 participants