Skip to content

asyncio.base_events.BaseEventLoop._add_callback references TimerHandle but never uses them #101143

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.

Sign up for GitHub

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

Jump to bottom
Closed
bdraco opened this issue Jan 19, 2023 · 3 comments · Fixed by #101197
Closed

asyncio.base_events.BaseEventLoop._add_callback references TimerHandle but never uses them #101143

bdraco opened this issue Jan 19, 2023 · 3 comments · Fixed by #101197
Labels
topic-asyncio type-bug An unexpected behavior, bug, or error

Comments

@bdraco
Copy link
Contributor

bdraco commented Jan 19, 2023 •
edited by bedevere-bot
Loading

Bug report

After 592ada9 it appears this function not longer cares about self._scheduled or TimerHandle but the docstring seems to imply otherwise

Your environment

  • CPython versions tested on: 3.12.0a4
  • Operating system and architecture: MacOS

I only noticed because it was unexpectedly more expensive

unexpectedly_more_expensive

I think its dead code and can be simplified to:

diff --git a/Lib/asyncio/base_events.py b/Lib/asyncio/base_events.py
index cbabb43ae0..070ea1044d 100644
--- a/Lib/asyncio/base_events.py
+++ b/Lib/asyncio/base_events.py
@@ -1857,11 +1857,9 @@ def call_exception_handler(self, context):
                                  exc_info=True)
 
     def _add_callback(self, handle):
-        """Add a Handle to _scheduled (TimerHandle) or _ready."""
+        """Add a Handle to _ready."""
         assert isinstance(handle, events.Handle), 'A Handle is required here'
-        if handle._cancelled:
-            return
-        assert not isinstance(handle, events.TimerHandle)
+        if not handle._cancelled:
             self._ready.append(handle)
 
     def _add_callback_signalsafe(self, handle):

Linked PRs
@bdraco bdraco added the type-bug An unexpected behavior, bug, or error label Jan 19, 2023
@github-project-automation github-project-automation bot moved this to Todo in asyncio Jan 19, 2023
@gvanrossum
Copy link
Member

You might as well submit a PR, we can review then.

@bdraco
Copy link
Contributor Author

bdraco commented Jan 20, 2023

Pushed that suggested change to a few more production machines to be sure. Still kinda surprising how much run time it has but I guess thats what happens when you call it ~10000 time a minute.

mod

Will send a PR soon

@bdraco bdraco mentioned this issue Jan 20, 2023
Merged
@bdraco
Copy link
Contributor Author

bdraco commented Jan 20, 2023 •
edited
Loading

Updated callgrind after #101197 (comment) which brings it down to the 21st most run time from 6th on my test Home Assistant instance.

post_review

@github-project-automation github-project-automation bot moved this from Todo to Done in asyncio Jan 21, 2023
kumaraditya303 added a commit that referenced this issue Jan 21, 2023
@bdraco @kumaraditya303
gh-101143: Remove references to TimerHandle from `asyncio.base_even…
9e94767 
…ts.BaseEventLoop._add_callback` (#101197)

Co-authored-by: Kumar Aditya <59607654+kumaraditya303@users.noreply.github.com>
miss-islington pushed a commit to miss-islington/cpython that referenced this issue Jan 21, 2023
@bdraco @kumaraditya303 @miss-islington
pythongh-101143: Remove references to TimerHandle from `asyncio.bas…
c43548f 
…e_events.BaseEventLoop._add_callback` (pythonGH-101197)

(cherry picked from commit 9e94767)

Co-authored-by: J. Nick Koston <nick@koston.org>
Co-authored-by: Kumar Aditya <59607654+kumaraditya303@users.noreply.github.com>
@bedevere-bot bedevere-bot mentioned this issue Jan 21, 2023
Merged
miss-islington pushed a commit to miss-islington/cpython that referenced this issue Jan 21, 2023
@bdraco @kumaraditya303 @miss-islington
pythongh-101143: Remove references to TimerHandle from `asyncio.bas…
4484f7d 
…e_events.BaseEventLoop._add_callback` (pythonGH-101197)

(cherry picked from commit 9e94767)

Co-authored-by: J. Nick Koston <nick@koston.org>
Co-authored-by: Kumar Aditya <59607654+kumaraditya303@users.noreply.github.com>
@bedevere-bot bedevere-bot mentioned this issue Jan 21, 2023
Merged
miss-islington added a commit that referenced this issue Jan 21, 2023
@miss-islington @bdraco @kumaraditya303
gh-101143: Remove references to TimerHandle from `asyncio.base_even…
99da75e 
…ts.BaseEventLoop._add_callback` (GH-101197)

(cherry picked from commit 9e94767)

Co-authored-by: J. Nick Koston <nick@koston.org>
Co-authored-by: Kumar Aditya <59607654+kumaraditya303@users.noreply.github.com>
miss-islington added a commit that referenced this issue Jan 21, 2023
@miss-islington @bdraco @kumaraditya303
gh-101143: Remove references to TimerHandle from `asyncio.base_even…
9532c04 
…ts.BaseEventLoop._add_callback` (GH-101197)

(cherry picked from commit 9e94767)

Co-authored-by: J. Nick Koston <nick@koston.org>
Co-authored-by: Kumar Aditya <59607654+kumaraditya303@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
topic-asyncio type-bug An unexpected behavior, bug, or error
Projects
asyncio
Status: Done
Milestone
No milestone
3 participants
@bdraco @gvanrossum @sobolevn