-
-
Notifications
You must be signed in to change notification settings - Fork 483
refactor: Update Client.run
to have a better async I/O usage
#2645
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
base: master
Are you sure you want to change the base?
Conversation
Co-authored-by: Dorukyum <53639936+Dorukyum@users.noreply.github.com> Signed-off-by: DA344 <108473820+DA-344@users.noreply.github.com>
Co-authored-by: Dorukyum <53639936+Dorukyum@users.noreply.github.com> Signed-off-by: DA344 <108473820+DA-344@users.noreply.github.com>
Applied all the changes Dorukyum requested. Before merging, I would like some feedback on this discussion message |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR refactors the Client.run
logic to improve async I/O usage and fix asyncio-related issues that could occur when using the library's event loop handling.
- Replaces
asyncio.iscoroutinefunction
withinspect.iscoroutinefunction
throughout the codebase for better reliability - Refactors
Client.run
to use modernasyncio.run
instead of manual loop management - Updates loop handling to be more flexible with
None
values and better async context management
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
File | Description |
---|---|
discord/utils.py |
Added inspect import and replaced asyncio.iscoroutinefunction |
discord/state.py |
Updated loop handling to accept None and added dispatch wrapper |
discord/http.py |
Simplified loop initialization to accept None values |
discord/ext/tasks/__init__.py |
Major refactor of task loop handling and scheduling logic |
discord/ext/commands/core.py |
Replaced asyncio.iscoroutinefunction with inspect equivalent |
discord/commands/core.py |
Replaced asyncio.iscoroutinefunction with inspect equivalent |
discord/client.py |
Major refactor of Client.run and loop management |
discord/bot.py |
Replaced asyncio.iscoroutinefunction with inspect equivalent |
CHANGELOG.md |
Added entry documenting the async I/O fixes |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Signed-off-by: Paillat <paillat@pycord.dev>
Can't seem to manually call login + connect: ErrorTraceback (most recent call last):
File "C:\xxx\pr2645.py", line 19, in <module>
asyncio.run(main())
~~~~~~~~~~~^^^^^^^^
File "C:xxx\uv\python\cpython-3.14.0-windows-x86_64-none\Lib\asyncio\runners.py", line 204, in run
return runner.run(main)
~~~~~~~~~~^^^^^^
File "C:\xxx\uv\python\cpython-3.14.0-windows-x86_64-none\Lib\asyncio\runners.py", line 127, in run
return self._loop.run_until_complete(task)
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^^^^^^
File "C:\xxx\uv\python\cpython-3.14.0-windows-x86_64-none\Lib\asyncio\base_events.py", line 719, in run_until_complete
return future.result()
~~~~~~~~~~~~~^^
File "C:\xxx\pr2645.py", line 16, in main
await client.connect()
File "xxx\.venv\Lib\site-packages\discord\client.py", line 761, in connect
self.ws = await asyncio.wait_for(coro, timeout=60.0)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "C:\xxx\uv\python\cpython-3.14.0-windows-x86_64-none\Lib\asyncio\tasks.py", line 488, in wait_for
return await fut
^^^^^^^^^
File "C:\xxx\.venv\Lib\site-packages\discord\gateway.py", line 348, in from_client
ws = cls(socket, loop=client.loop)
^^^^^^^^^^^
File "C:xxx\.venv\Lib\site-packages\discord\client.py", line 369, in loop
raise RuntimeError("loop is not set")
RuntimeError: loop is not set
Unclosed client session Codeimport asyncio
import discord
client = discord.Client()
@client.event
async def on_ready():
print(f"We have logged in as {client.user}")
async def main():
await client.login("xxx")
await client.connect()
asyncio.run(main()) |
ext.tasks seems to be broken and the changes to it aren't in the changelog?
@tasks.loop(seconds=1)
# @tasks.loop(seconds=1, create_loop=False) # <- default
async def my_background_task():
print("Background task is running...")
# my_background_task.start() <- does nothing
client.run(...)
@tasks.loop(seconds=1)
# @tasks.loop(seconds=1, create_loop=False) # <- default
async def my_background_task():
print("Background task is running...")
# optional: my_background_task.start()
@tasks.loop(seconds=1, create_loop=True)
async def my_background_task():
print("Background task is running...")
client.run(...)
@tasks.loop(seconds=1, create_loop=True)
async def my_background_task():
print("Background task is running...")
my_background_task.start()
# console:
# Task was destroyed but it is pending!
# task: <Task pending name='pycord-ext-task (0x1f4fb9d3380): my_background_task' coro=<Loop._loop() running at C:\Users\Sohea\OneDrive\Documents\test\pycord\.venv\Lib\site-packages\discord\ext\tasks\__init__.py:213>>
# <sys>:0: RuntimeWarning: coroutine 'Loop._loop' was never awaited This is all super confusing and so is the doc of the |
Isn't that the same code as task number one ? |
Yes but without running the bot, which is what I assumed |
Added medium prio, this fixes multiple issues and is a much needed cleanup, will check everything out to try and find out more about those loop issues. |
Also got that same issue. @DA-344 What do you think about this ? Is there any reason why we couldn't do that: @property
def loop(self) -> asyncio.AbstractEventLoop:
"""The event loop that the client uses for asynchronous operations."""
if self._loop is None:
try:
self._loop = asyncio.get_running_loop()
except RuntimeError as e:
raise RuntimeError("loop is not set") from e
return self._loop This should also normally allow you to remove the other places where it try: excepts asyncio.get_running_loop since it would be in the property itself |
Note, Needs testing w/ async autocompletes and typing context manager as well as ext.loop |
Summary
This PR refactors the
Client.run
logic to fix problems involving asyncio due to how the library used the loop:Needs testing
Exception
Information
examples, ...).
Checklist
type: ignore
comments were used, a comment is also left explaining why.