classification
Title: Add argument to .cancel() of Task and Future
Type: enhancement Stage:
Components: asyncio Versions: Python 3.7, Python 3.6, Python 3.5
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: Nosy List: chris.jerdonek, matrixise, socketpair, yselivanov
Priority: normal Keywords:

Created on 2017-07-25 13:21 by socketpair, last changed 2017-08-11 05:34 by socketpair.

Messages (9)
msg299079 - (view) Author: Марк Коренберг (socketpair) * Date: 2017-07-25 13:21
History:

First, I tried to debug code around asyncio.Task() cancelling. So I wrote:
=====
try:
   ...
except Exception as e:
   print(e)
=====

When task was cancelled, an empty string printed. I wondered why. So I change the code to

====
print(repr(e))
====

and it printed 'CancelledError' as expected.

Next, I tried:

====
print(Exception())
====

It prints empty string too!

So I came up to propose API change. I propose to add argument to the .cancel() methods (for Task and for Future). This argument should be passed to the CancelledError constructor. This will greatly improves debugging -- it allows to easily know why Future/Task was cancelled.

Also, this change does not break current code. Argument must be optional.
msg299083 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2017-07-25 13:48
Well, don't use "print(e)" when you are printing errors in in Python. This is pretty standard behaviour for all Python code, not just asyncio. A lot of code in stdlib raises exceptions, almost none allows to customize them.
msg299084 - (view) Author: Stéphane Wirtel (matrixise) * Date: 2017-07-25 13:50
you could use gdb or pdb for the debugging.
msg299101 - (view) Author: Марк Коренберг (socketpair) * Date: 2017-07-25 15:18
Yes, I agree with you about my weird way of debugging. But anyway, changing API with adding ability to pass actual cause would be welcome.
msg299103 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2017-07-25 15:21
> Yes, I agree with you about my weird way of debugging. But anyway, changing API with adding ability to pass actual cause would be welcome.

I'm not opposed to the idea, btw. If we do decide to add an argument to 'cancel', we probably should do the same for concurrent.futures.

Another possibility would be to allow cancellation via Future.set_exception:

  task.set_exception(asyncio.CancelledError('message'))
msg299107 - (view) Author: Марк Коренберг (socketpair) * Date: 2017-07-25 15:29
Hmmm....

task.set_exception(Exception('xxx'))

Will it cancel whole chain of depending futures in a RIGHT way ?
Or we must require exception passed here to be subclassed from CancelledError ?
msg299112 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2017-07-25 15:34
> Will it cancel whole chain of depending futures in a RIGHT way ?

I was thinking about this:

  def Future.set_exception(exc):
      if isinstance(exc, asyncio.CancelledError):
          return self._cancel(exc)
      # here goes old code

  def Future.cancel():
      return self._cancel(asyncio.CancelledError())

Although now, that I'm looking at it, I don't like this idea, because setting as exception is a different operation from cancelling a task/future, and we shouldn't mix them.  It's OK to set a CancelledError without cancelling.  Also this would be a backwards incompatible change.

So back to square one -- we can consider passing extra args to .cancel() methods.
msg299891 - (view) Author: Chris Jerdonek (chris.jerdonek) * (Python committer) Date: 2017-08-08 02:15
A couple thoughts on this issue:

First, I think the OP's original issue could perhaps largely be addressed without having to change cancel()'s signature. Namely, simply passing a hard-coded string to CancelledError in the couple spots that CancelledError is raised would cause the exception to display:

https://github.com/python/cpython/blob/733d0f63c562090a2b840859b96028d6ec0a4803/Lib/asyncio/futures.py#L153
https://github.com/python/cpython/blob/733d0f63c562090a2b840859b96028d6ec0a4803/Lib/asyncio/futures.py#L173

The "raise CancelledError" could be changed to "raise CancelledError('future is cancelled')", a bit like how InvalidStateError is handled a couple lines later:

    if self._state == _CANCELLED:
        raise CancelledError
    if self._state != _FINISHED:
        raise InvalidStateError('Result is not ready.')

Second, in terms of making cancellations easier to debug, is it a deliberate design decision that the CancelledError traceback "swallows" / doesn't show the point at which the coroutine was cancelled?

For example, running the following code--

    async def run():
        await asyncio.sleep(1000000)

    loop = asyncio.new_event_loop()
    task = asyncio.ensure_future(run(), loop=loop)
    loop.call_later(2, task.cancel)
    loop.run_until_complete(task)

Results in the following output:

    Traceback (most recent call last):
      File "test-cancel.py", line 46, in <module>
        loop.run_until_complete(task)
      File "/Users/.../python3.6/asyncio/base_events.py", line 466,
          in run_until_complete
        return future.result()
    concurrent.futures._base.CancelledError

In particular, it doesn't show that the task was waiting on asyncio.sleep(1000000) when the task was cancelled. It would be very useful to see full tracebacks in these cases. (Sorry in advance if this second point is off-topic for this issue.)
msg300152 - (view) Author: Марк Коренберг (socketpair) * Date: 2017-08-11 05:34
1. Yes, specifying argument to cancel in `raise CancelledError` would be perfect
2. specifying argument to `.cancel()` is still actual.
3. Yes, important part of exceptions should not be swallowed (please open separate issue). Also, it will be nice to show which code calls `.cancel()`, not only `sleep()` you menitioned.
History
Date User Action Args
2017-08-11 05:34:50socketpairsetmessages: + msg300152
2017-08-08 02:15:28chris.jerdoneksetmessages: + msg299891
2017-08-08 01:12:27chris.jerdoneksetnosy: + chris.jerdonek
2017-07-25 15:34:11yselivanovsetmessages: + msg299112
2017-07-25 15:29:04socketpairsetmessages: + msg299107
2017-07-25 15:21:42yselivanovsetmessages: + msg299103
2017-07-25 15:18:40socketpairsetmessages: + msg299101
2017-07-25 13:50:07matrixisesetnosy: + matrixise
messages: + msg299084
2017-07-25 13:48:27yselivanovsetmessages: + msg299083
2017-07-25 13:21:40socketpaircreate