Check if Task(Future) is canceled.

[fujitatomoya]

closes #1099

[nadavelkabets]

Please backport this bugfix into humble/jazzy.
Cancelling tasks is a crucial feature of any event loop, and for a project of mine I had to come up with complex and incomplete solutions to make that work.

[weber-niklas]

Hey,
any updates here?
I think @fujitatomoya was pretty close to make it work.
Maybe if we go with @nadavelkabets suggestion number 2:

def cancel(self):
	if not self._done and iscoroutine(self._handle):
		  self._handle.close()
	super().cancel()

We can at least fix the bug at hand and concentrate on improving on the functionality after?
I am currently having a pretty hard time using Tasks since a bunch of it is broken. Would love to get a fix out which makes the feature usable. Thanks for the work you guys put in <3

[fujitatomoya]

@nadavelkabets @weber-niklas sorry guys for being late to get back to this, i will try to allocate some time for this in next week.

[nadavelkabets]

@nadavelkabets @weber-niklas sorry guys for being late to get back to this, i will try to allocate some time for this in next week.

I created PR #1392 to fix the unawaited coroutine issue based on my first suggestion. It's not yet complete since we need to figure out how to handle the CancelledException in a way that won't crash the executor.

I believe we should first go with a solution that we can backport into jazzy.
Do you think suggestion 1 is viable for a backport, or should we start with suggestion 2 and add suggestion 1 for the next version?

[fujitatomoya]

@nadavelkabets again thank you very much for helping this one moving forward 👍

i have already #1394 merged in this PR.

I believe we should first go with a solution that we can backport into jazzy.

agree, this is also broken in jazzy and humble.

Do you think suggestion 1 is viable for a backport, or should we start with suggestion 2 and add suggestion 1 for the next version?

IMO, we can go with suggestion 2 (coroutine close, that is currently implemented in this PR) to avoid surprising the application with new exception generated by task cancel. that said it does not break the user space, user application can have the fix and also no user code change is required.

And then probably we can move on to #1392 for rolling branch. i think having the capability for clean-up in the user callback makes sense for canceling tasks.

It's not yet complete since we need to figure out how to handle the CancelledException in a way that won't crash the executor.

i will look into this for sure and leave some comments.

[nadavelkabets]

Missed this line in my PR, not needed anymore since done() now returns true if cancelled.
There are some other duplicate checks for done and cancelled, I'll review the code again to look for them.