[issue31033] Add argument to .cancel() of Task and Future

[Chris Jerdonek]

Chris Jerdonek  added the comment:


New changeset da742ba826721da84140abc785856d4ccc2d787f by Chris Jerdonek in 
branch 'master':
bpo-31033: Improve the traceback for cancelled asyncio tasks (GH-19951)
https://github.com/python/cpython/commit/da742ba826721da84140abc785856d4ccc2d787f


--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue31033] Add argument to .cancel() of Task and Future

[Yury Selivanov]

Yury Selivanov  added the comment:

Elevating to release blocker to make sure it's included. The PR is good.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue31033] Add argument to .cancel() of Task and Future

[Yury Selivanov]

Change by Yury Selivanov :


--
priority: normal -> release blocker

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue31033] Add argument to .cancel() of Task and Future

[Chris Jerdonek]

Chris Jerdonek  added the comment:

I just want to flag one issue after rebasing my traceback PR onto what was 
merged. If task.cancel() is called like this--

task.cancel("POSSIBLY LONG CANCEL MESSAGE")

There is the question of whether the passed message should be repeated each 
time the CancelledError is raised, or only show it in the innermost, 
originating exception. My preference is to do the latter because it is simpler, 
less verbose, and seems more correct from a Python perspective. But I wanted to 
flag this because the message won't be visible in the leading, outermost 
exception.

There is a third alternative which is to mutate the exception each time (delete 
the message from the earlier exception and add it to the new exception). But 
that seems more fraught and what I'd consider surprising behavior.

Lastly, to illustrate, here is the more verbose option (the one I think it 
**shouldn't** look like):

Traceback (most recent call last):
  File "/.../cpython/test-cancel.py", line 4, in nested
await asyncio.sleep(1)
  File "/.../cpython/Lib/asyncio/tasks.py", line 670, in sleep
return await future
asyncio.exceptions.CancelledError: POSSIBLY LONG CANCEL MESSAGE

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/.../cpython/test-cancel.py", line 11, in run
await task
asyncio.exceptions.CancelledError: POSSIBLY LONG CANCEL MESSAGE

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/.../cpython/test-cancel.py", line 15, in 
loop.run_until_complete(run())
  File "/.../cpython/Lib/asyncio/base_events.py", line 642, in 
run_until_complete
return future.result()
asyncio.exceptions.CancelledError: POSSIBLY LONG CANCEL MESSAGE

--
versions: +Python 3.9 -Python 3.5, Python 3.6, Python 3.7

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue31033] Add argument to .cancel() of Task and Future

[Chris Jerdonek]

Chris Jerdonek  added the comment:

The msg argument has now been added (second PR). I'm going to keep this issue 
open until the traceback issue has also been addressed (the other PR), as that 
was one part of the discussions here.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue31033] Add argument to .cancel() of Task and Future

[Chris Jerdonek]

Chris Jerdonek  added the comment:


New changeset 1ce5841eca6d96b1b1e8c213d04f2e92b1619bb5 by Chris Jerdonek in 
branch 'master':
bpo-31033: Add a msg argument to Future.cancel() and Task.cancel() (GH-19979)
https://github.com/python/cpython/commit/1ce5841eca6d96b1b1e8c213d04f2e92b1619bb5


--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue31033] Add argument to .cancel() of Task and Future

[Chris Jerdonek]

Chris Jerdonek  added the comment:

I also posted a second PR (separate from the first) that adds a "msg" argument 
to Future.cancel() and Task.cancel(). That PR is also now ready for review:
https://github.com/python/cpython/pull/19979

The other PR is simpler and more essential though, so I think should be 
reviewed before this.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue31033] Add argument to .cancel() of Task and Future

[Chris Jerdonek]

Change by Chris Jerdonek :


--
pull_requests: +19295
pull_request: https://github.com/python/cpython/pull/19979

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue31033] Add argument to .cancel() of Task and Future

[Chris Jerdonek]

Chris Jerdonek  added the comment:

Okay, I completed my draft PR (both C and pure Python implementations are done).

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue31033] Add argument to .cancel() of Task and Future

[Chris Jerdonek]

Chris Jerdonek  added the comment:

I just posted a draft, proof-of-concept PR for one aspect of this issue: 
https://github.com/python/cpython/pull/19951

Namely, the PR makes it so that cancelled tasks have full tracebacks, all the 
way to where the code first gets interrupted. I did the C implementation, but I 
still need to do the pure Python implementation. (Note that it doesn't show 
where `task.cancel()` was *called*, but rather the first line of code that is 
cancelled. That other aspect can be handled in a separate PR.)

As an example, for this code--

import asyncio

async def nested():
await asyncio.sleep(1)

async def run():
task = asyncio.create_task(nested())
await asyncio.sleep(0)
task.cancel()
await task

loop = asyncio.new_event_loop()
try:
loop.run_until_complete(run())
finally:
loop.close()

Python currently (before the PR) does this:

Traceback (most recent call last):
  File "/.../cpython/test-cancel.py", line 15, in 
loop.run_until_complete(run())
  File "/.../cpython/Lib/asyncio/base_events.py", line 642, in 
run_until_complete
return future.result()
asyncio.exceptions.CancelledError

With the PR, it looks like this. In particular, you can see that it includes 
"await asyncio.sleep(1)", as well as the intermediate await:

Traceback (most recent call last):
  File "/.../cpython/test-cancel.py", line 5, in nested
await asyncio.sleep(1)
  File "/.../cpython/Lib/asyncio/tasks.py", line 657, in sleep
return await future
asyncio.exceptions.CancelledError

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/.../cpython/test-cancel.py", line 11, in run
await task
asyncio.exceptions.CancelledError

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/.../cpython/test-cancel.py", line 15, in 
loop.run_until_complete(run())
  File "/.../cpython/Lib/asyncio/base_events.py", line 642, in 
run_until_complete
return future.result()
asyncio.exceptions.CancelledError

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue31033] Add argument to .cancel() of Task and Future

[Chris Jerdonek]

Change by Chris Jerdonek :


--
keywords: +patch
pull_requests: +19267
stage:  -> patch review
pull_request: https://github.com/python/cpython/pull/19951

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue31033] Add argument to .cancel() of Task and Future

[Alexander Mohr]

Alexander Mohr  added the comment:

@yselivanov even better :)

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue31033] Add argument to .cancel() of Task and Future

[Carl Meyer]

Change by Carl Meyer :


--
nosy: +carljm

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue31033] Add argument to .cancel() of Task and Future

[Yury Selivanov]

Yury Selivanov  added the comment:

> I like the idea of having an argument to construct the CancelledError with, 
> but I like even more the ability to tell the exception that will be raised to 
> have the traceback of the point where the task was originally cancelled.

Why don't we make CancelledErrors having proper traceback a default behaviour?

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue31033] Add argument to .cancel() of Task and Future

[Alexander Mohr]

Alexander Mohr  added the comment:

I was about to open a new bug, but I think my idea overlaps with this one.  
From what I understand there are two ways to cancel tasks:

1) calling task.cancel()
2) explicitly raising a CancelledError

with #2 you can get a traceback by catching the exception which yields why the 
task was cancelled (per callstack, and you create the CancelledError with a 
custom message).

#1 I think is what this is talking about.

The most useful information IMHO is the callstack, and via #1 this callstack is 
completely lost.

I like the idea of having an argument to construct the CancelledError with, but 
I like even more the ability to tell the exception that will be raised to have 
the traceback of the point where the task was originally cancelled.

Right now I have to use forbidden fruit to override the Task.cancel method and 
store the callstack in a custom attribute.

I was talking with asvetlov and and he thought perhaps there could be an 
environment variable to enable this by default and then when the cancellation 
error is raised it can have the caller's callstack...I think ideally this would 
be the default if it could be done cheaply (given if a regular exception throw 
contains the callstack, then a cancellation exception should always as well.

Ideally I think when a task is cancelled it instantiates the exception with the 
callstack at that point, and then does something like a "with e" to raise the 
exception from the future so you get it when it's raised.

--
nosy: +thehesiod

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue31033] Add argument to .cancel() of Task and Future

[Марк Коренберг]

Марк Коренберг added the comment:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue31033] Add argument to .cancel() of Task and Future

[Chris Jerdonek]

Chris Jerdonek added the comment:

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(100)

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 
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(100) 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.)

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue31033] Add argument to .cancel() of Task and Future

[Chris Jerdonek]

Changes by Chris Jerdonek :


--
nosy: +chris.jerdonek

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue31033] Add argument to .cancel() of Task and Future

[Yury Selivanov]

Yury Selivanov added the comment:

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue31033] Add argument to .cancel() of Task and Future

[Марк Коренберг]

Марк Коренберг added the comment:

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 ?

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue31033] Add argument to .cancel() of Task and Future

[Yury Selivanov]

Yury Selivanov added the comment:

> 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'))

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue31033] Add argument to .cancel() of Task and Future

[Марк Коренберг]

Марк Коренберг added the comment:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue31033] Add argument to .cancel() of Task and Future

[Stéphane Wirtel]

Stéphane Wirtel added the comment:

you could use gdb or pdb for the debugging.

--
nosy: +matrixise

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue31033] Add argument to .cancel() of Task and Future

[Yury Selivanov]

Yury Selivanov added the comment:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue31033] Add argument to .cancel() of Task and Future

[Марк Коренберг]

New submission from Марк Коренберг:

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.

--
components: asyncio
messages: 299079
nosy: socketpair, yselivanov
priority: normal
severity: normal
status: open
title: Add argument to .cancel() of Task and Future
type: enhancement
versions: Python 3.5, Python 3.6, Python 3.7

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com