Skip to content

bpo-31943: Add asyncio.Handle.cancelled() method #2388

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

Merged
merged 1 commit into from
Nov 7, 2017

Conversation

decaz
Copy link
Contributor

@decaz decaz commented Jun 25, 2017

It would be handy to access the loop's time the Handle's callback will be called at if you are using delayed calls with "delay" (not "when") argument.
Also it is useful to know whether the call was cancelled through the special attribute.

https://bugs.python.org/issue31943

@mention-bot
Copy link

@decaz, thanks for your PR! By analyzing the history of the files in this pull request, we identified @1st1, @methane and @serhiy-storchaka to be potential reviewers.

@decaz decaz changed the title Renamed asyncio.Handle._when attribute to public asyncio.Handle.when trivial: Renamed asyncio.Handle._when attribute to public asyncio.Handle.when Jun 25, 2017
@decaz decaz changed the title trivial: Renamed asyncio.Handle._when attribute to public asyncio.Handle.when trivial: Added asyncio.TimerHandle.when and asyncio.Handle.cancelled properties Jun 27, 2017
@decaz decaz force-pushed the asyncio-handle-when branch 2 times, most recently from bb9c589 to 769d3d2 Compare June 28, 2017 08:22
@asvetlov
Copy link
Contributor

asvetlov commented Nov 1, 2017

@1st1 should we support it?
The change makes no harm but add version dependent behavior: user code should check if when attribute is present before accessing it.
On other side call_later() and family are low level API methods, most likely user should not call them from async functions.

@1st1
Copy link
Member

1st1 commented Nov 1, 2017

I'm OK with .cancelled, but I've no idea why we need .when. @decaz do you have a real use-case for it?

@1st1
Copy link
Member

1st1 commented Nov 1, 2017

BTW, .cancelled should be a method, not property, similar to Future.cancelled() method.

@decaz decaz force-pushed the asyncio-handle-when branch from 769d3d2 to 911c75c Compare November 4, 2017 14:49
@decaz decaz requested a review from 1st1 as a code owner November 4, 2017 14:49
@decaz decaz changed the title trivial: Added asyncio.TimerHandle.when and asyncio.Handle.cancelled properties trivial: Add asyncio.Handle.cancelled() and asyncio.TimerHandle.when Nov 4, 2017
@decaz decaz force-pushed the asyncio-handle-when branch 2 times, most recently from f2facf7 to 575a7eb Compare November 4, 2017 15:55
@decaz decaz changed the title trivial: Add asyncio.Handle.cancelled() and asyncio.TimerHandle.when bpo-31943: Add asyncio.Handle.cancelled() and asyncio.TimerHandle.when Nov 4, 2017
@decaz decaz force-pushed the asyncio-handle-when branch from 575a7eb to 0f8ac80 Compare November 4, 2017 16:31
@decaz
Copy link
Contributor Author

decaz commented Nov 4, 2017

@1st1 yes, .when can be used to determine how much time is left before the delayed call is executed. For instance:

seconds_left = handler.when - loop.time()

Now it's impossible as the time the handler will be called at is calculated internally.

@1st1
Copy link
Member

1st1 commented Nov 5, 2017

I specifically asked you about a real use case for ‘.when’. That means you should have told us about a real problem you encountered implementing a protocol or a server using asyncio, which was unsolvable without the ‘.when’ attribute.

We won’t add something just because there are hypothetical use cases for it.

@decaz decaz force-pushed the asyncio-handle-when branch from 0f8ac80 to 7510b17 Compare November 6, 2017 17:00
@decaz decaz changed the title bpo-31943: Add asyncio.Handle.cancelled() and asyncio.TimerHandle.when bpo-31943: Add asyncio.Handle.cancelled() method Nov 6, 2017
@decaz
Copy link
Contributor Author

decaz commented Nov 6, 2017

@1st1 agree with you, just wanted to add such an attribute on the case if it will be convenient. I have rebased the branch.

.. method:: cancelled()

Return ``True`` if the call was cancelled.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add .. versionadded:: 3.6

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

3.7 :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, added. I hope it will be backported to 3.6?)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No. Asyncio is no longer provisional and this is not a bug fix.

@bedevere-bot
Copy link

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.

@asvetlov
Copy link
Contributor

asvetlov commented Nov 6, 2017

BTW Handler and TimeHandler are heavy used by asyncio core.
Maybe adding C implementation makes sense?
Even maybe supporting free lists for these classes might affect asyncio performance?

@1st1
Copy link
Member

1st1 commented Nov 6, 2017

Maybe adding C implementation makes sense?

I can see it improving the performance somewhat. But please hold off with the implementation before PEP 550 (or its successor) lands.

@decaz decaz force-pushed the asyncio-handle-when branch from 7510b17 to 7c21485 Compare November 6, 2017 17:31
@decaz
Copy link
Contributor Author

decaz commented Nov 6, 2017

I have made the requested changes; please review again.

@bedevere-bot
Copy link

Thanks for making the requested changes!

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

@asvetlov
Copy link
Contributor

asvetlov commented Nov 7, 2017

Thanks for contribution!

embray pushed a commit to embray/cpython that referenced this pull request Nov 9, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants