bpo-40607: Reraise exception during task cancelation in asyncio.wait_for() by romasku · Pull Request #20054 · python/cpython

romasku · 2020-05-12T13:10:50Z

https://bugs.python.org/issue40607

the-knights-who-say-ni · 2020-05-12T13:10:53Z

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept this contribution by verifying everyone involved has signed the PSF contributor agreement (CLA).

Recognized GitHub username

We couldn't find a bugs.python.org (b.p.o) account corresponding to the following GitHub usernames:

@romasku

This might be simply due to a missing "GitHub Name" entry in one's b.p.o account settings. This is necessary for legal reasons before we can look at this contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

You can check yourself to see if the CLA has been received.

Thanks again for the contribution, we look forward to reviewing it!

belkka · 2020-05-12T19:20:44Z

Lib/asyncio/tasks.py

Could it be

if not fut.cancelled(): return fut.result()

?

Building on this, how about--

try: return fut.result() except CancelledError as exc: raise TimeoutError() from exc

What's nice about this is that you'll see the CancelledError that caused the TimeoutError in the traceback. This will be even better when this PR is merged: #19951
In the traceback, you'll get to see exactly what line of code was interrupted by the cancellation.

OK, thanks, I find this even more appropriate way to handle such situations.

belkka · 2020-05-12T19:24:01Z

Lib/asyncio/tasks.py

What about the case when task cancellation failed with some result (not exception)? Is it OK to ignore it? I mean something like

async def inner(): try: await asyncio.sleep(0.2) except asyncio.CancellationError: return 123 # instead of `raise FooException`

romasku · 2020-05-13T12:43:57Z

I signed CLA yesterday, but for some reason checker returns 500 for my username. Is it OK and I should wait some more time?

cjerdonek · 2020-05-13T12:59:52Z

@Mariatta, are you able to answer @romasku's question above about the checker returning 500? Thanks a lot if you're able to help.

cjerdonek

A couple higher-level comments before a more detailed review:

In the TimeoutError case, it would be good to check that the CancelledError is getting chained. You can see an example of how to check for that in a test here: #19858
Maybe this can be added to another existing test. I'm not sure as I haven't looked.
It looks like your tests take 0.1 seconds to run, which adds up a lot given the number of tests. It would be better to design them so they run without having to sleep for non-negligible amounts of time but still be tests that are (1) simple and (2) not flaky. This may take a little more creativity, or it might be easy. I'm not sure, but it should be doable.

bedevere-bot · 2020-05-13T13:11:14Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

Mariatta · 2020-05-13T16:55:20Z

Seems to work fine when I entered romasku in the CLA checker app.
It says that there is no b.p.o user associated with the has romasku GitHub user. Basically you need to add romasku to your b.p.o account.

1st1 · 2020-05-13T18:32:36Z

Lib/asyncio/tasks.py

I'd put also an else: raise TimeoutError

Sorry, but I think that else branch is unreachable: if there isn't any exception in try block, then return works normally and we leave this method. Please explain if I am wrong.

Yes, but your new code is simply not equivalent to the previous code that raised TimeoutError unconditionally. Now I see that there might be a condition for clear return and I don't want that.

The linked issue talks about the correct exception propagation. Basically, wait_for timeouts with an exception and we want that original exception to be propagated. That's fine and I'm +1 for that.

The code here is slightly different though and a bit hard to trace so it would be easier to read if it was replaced with:

try: fut.result() except exceptions.CancelledError as exc: raise exceptions.TimeoutError() from exc else: raise exceptions.TimeoutError()

I think the issue here is that there is a race condition where the task can complete after the timeout has elapsed. So the question is whether the caller should get to learn that the task completed in this case.

Yes, the current implementation discards the result, as the probability of actually having it is low. I'd like to keep it that way. If we want to change the current behavior that should be done in a separate issue, with a new PR and new discussion.

romasku · 2020-05-13T19:24:20Z

I entered "@romasku" (with '@' symbol) as my github username in b.p.o. account, and was checking against it. Now everything works, thank you!

romasku · 2020-05-13T21:55:03Z

Thank you for your review!
To overcome the second issue (about sleeping in tests) I've added PositiveZero class: it passes greater than zero check, so asyncio.wait_for main case logic runs, but it makes sleep time effectively zero.
To be honest, I am not sure about this solution: just using really small sleep time (for 0.000001 seconds) works the same way.
Also, I noticed that there are many other tests in test_tasks.py that sleep for 0.1-0.3 seconds - maybe it's better to make another PR to optimize those tests?

romasku · 2020-05-13T21:56:34Z

I have made the requested changes; please review again

bedevere-bot · 2020-05-13T21:56:37Z

Thanks for making the requested changes!

@cjerdonek: please review the changes made to this pull request.

1st1 · 2020-05-13T22:45:49Z

Lib/asyncio/tasks.py

Yes, but your new code is simply not equivalent to the previous code that raised TimeoutError unconditionally. Now I see that there might be a condition for clear return and I don't want that.

bedevere-bot · 2020-05-13T22:45:57Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

1st1 · 2020-05-14T20:08:07Z

This looks great now. @cjerdonek please review again.

Lib/test/test_asyncio/test_tasks.py

cjerdonek

There are a couple more tiny things I noticed, but otherwise looks great. Thanks a lot for your good work on this, @romasku!

Lib/test/test_asyncio/test_tasks.py

bedevere-bot · 2020-05-14T23:30:53Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

romasku · 2020-05-15T10:22:18Z

I have made the requested changes; please review again

bedevere-bot · 2020-05-15T10:22:21Z

Thanks for making the requested changes!

@cjerdonek, @1st1: please review the changes made to this pull request.

cjerdonek

Thanks, @romasku.

cjerdonek · 2020-05-15T11:28:31Z

@romasku, I forgot there are a couple more administrative things. Can you add your name to Misc/ACKS and also add "Patch by <your name>." at the end of your NEWS entry? More details can be found here: https://devguide.python.org/committing/#handling-others-code

…or() Currently, if asyncio.wait_for() timeout expires, it cancels inner future and then always raises TimeoutError. In case those future is task, it can handle cancelation mannually, and those process can lead to some other exception. Current implementation silently loses thoses exception. To resolve this, wait_for will check was the cancelation successfull or not. In case there was exception, wait_for will reraise it.

romasku · 2020-05-15T13:47:58Z

@cjerdonek, done. I am happy to contribute!

1st1 · 2020-05-15T20:12:23Z

Thanks so much, Roman!

…for() (pythonGH-20054) Currently, if asyncio.wait_for() timeout expires, it cancels inner future and then always raises TimeoutError. In case those future is task, it can handle cancelation mannually, and those process can lead to some other exception. Current implementation silently loses thoses exception. To resolve this, wait_for will check was the cancelation successfull or not. In case there was exception, wait_for will reraise it. Co-authored-by: Roman Skurikhin <roman.skurikhin@cruxlab.com>

romasku requested review from 1st1 and asvetlov as code owners May 12, 2020 13:10

the-knights-who-say-ni added the CLA not signed label May 12, 2020

bedevere-bot added the awaiting review label May 12, 2020

belkka reviewed May 12, 2020

View reviewed changes

romasku force-pushed the fix-issue-40607 branch from f31c497 to 7d4af0e Compare May 13, 2020 12:36

cjerdonek requested changes May 13, 2020

View reviewed changes

bedevere-bot removed the awaiting review label May 13, 2020

bedevere-bot added the awaiting changes label May 13, 2020

1st1 reviewed May 13, 2020

View reviewed changes

the-knights-who-say-ni added CLA signed and removed CLA not signed labels May 13, 2020

romasku force-pushed the fix-issue-40607 branch from 7d4af0e to c4bfb96 Compare May 13, 2020 20:06

romasku force-pushed the fix-issue-40607 branch from c4bfb96 to a46c582 Compare May 13, 2020 21:55

bedevere-bot added awaiting change review and removed awaiting changes labels May 13, 2020

bedevere-bot requested a review from cjerdonek May 13, 2020 21:56

1st1 requested changes May 13, 2020

View reviewed changes

bedevere-bot removed the awaiting change review label May 13, 2020

cjerdonek reviewed May 14, 2020

View reviewed changes

Lib/test/test_asyncio/test_tasks.py Outdated Show resolved Hide resolved

cjerdonek requested changes May 14, 2020

View reviewed changes

Lib/test/test_asyncio/test_tasks.py Outdated Show resolved Hide resolved

bedevere-bot removed the awaiting merge label May 14, 2020

bedevere-bot added the awaiting changes label May 14, 2020

romasku force-pushed the fix-issue-40607 branch from 9234d92 to 7393a28 Compare May 15, 2020 10:20

bedevere-bot added awaiting change review and removed awaiting changes labels May 15, 2020

bedevere-bot requested review from 1st1 and cjerdonek May 15, 2020 10:22

cjerdonek approved these changes May 15, 2020

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting change review labels May 15, 2020

romasku force-pushed the fix-issue-40607 branch from 7393a28 to c2f4f3c Compare May 15, 2020 13:46

1st1 approved these changes May 15, 2020

View reviewed changes

1st1 merged commit 382a563 into python:master May 15, 2020

bedevere-bot removed the awaiting merge label May 15, 2020

graingert mentioned this pull request May 31, 2022

wait_for(gather(...)) logs weird error message #73618

Closed

Uh oh!

Conversation

romasku commented May 12, 2020 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

the-knights-who-say-ni commented May 12, 2020

Recognized GitHub username

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

romasku commented May 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cjerdonek commented May 13, 2020

Uh oh!

cjerdonek left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented May 13, 2020

Uh oh!

Mariatta commented May 13, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

1st1 May 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

romasku commented May 13, 2020

Uh oh!

romasku commented May 13, 2020

Uh oh!

romasku commented May 13, 2020

Uh oh!

bedevere-bot commented May 13, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented May 13, 2020

Uh oh!

1st1 commented May 14, 2020

Uh oh!

Uh oh!

cjerdonek left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bedevere-bot commented May 14, 2020

Uh oh!

romasku commented May 15, 2020

Uh oh!

bedevere-bot commented May 15, 2020

Uh oh!

cjerdonek left a comment

Choose a reason for hiding this comment

Uh oh!

cjerdonek commented May 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

romasku commented May 15, 2020

Uh oh!

1st1 commented May 15, 2020

Uh oh!

Reviewers

Assignees

romasku commented May 12, 2020 •

edited by bedevere-bot

Loading

romasku commented May 13, 2020 •

edited

Loading

cjerdonek left a comment •

edited

Loading

1st1 May 14, 2020 •

edited

Loading

cjerdonek commented May 15, 2020 •

edited

Loading