refactor: Update Client.run to have a better async I/O usage#2645
refactor: Update Client.run to have a better async I/O usage#2645DA-344 wants to merge 72 commits intoPycord-Development:masterfrom
Client.run to have a better async I/O usage#2645Conversation
Lulalaby
requested review from
Dorukyum,
NeloBlivion and
plun1331
and removed request for
ChickenDevs
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 |
|
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._loopThis 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 |
|
clashes with recently merged pr's |
Paillat-dev
added
priority: high
|
@Lumabots When able can you run this for like 24h with some bot ? |
| ([#2905](https://github.com/Pycord-Development/pycord/pull/2905)) | ||
| - `view=None` in various methods causing an AttributeError. | ||
| ([#2915](https://github.com/Pycord-Development/pycord/pull/2915)) | ||
| - Fixed Async I/O errors that could be raised when using `Client.run`. |
There was a problem hiding this comment.
This probably should be more extensive.
| ) | ||
|
|
||
|
|
||
| def is_ambiguous(dt: datetime.datetime) -> bool: |
There was a problem hiding this comment.
Not sure what this is doing here. Is it required ? Or is it an additional feature ? In any case it should at least be in the changelog and maybe a separate PR depending on what the others think
There was a problem hiding this comment.
it is used internally on Loop.
There was a problem hiding this comment.
Yeah, but it wasn't present before. Does it change / increase the types of accepted inputs for loops ?
There was a problem hiding this comment.
this checks whether a datetime object falls in a ambiguous local time by DST transition (eg Spanish winter & summer local times)
so there is a moment in which the clock is set back and the same local hour occurs twice (that is why it uses fold)
this checks whether the datetime already has timezone data (dt.tzinfo) or uses a utc or fixed-offset datetime.timezone instance.
There was a problem hiding this comment.
Please remove the unrelated datetime changes and (if you want) move them to a separate pull request, I feel like this PR is already heavy enough
plun1331
left a comment
plun1331
left a comment
There was a problem hiding this comment.
This looks like it refactors a large amount of ext.tasks, much of it unrelated to asyncio usage. That should be in a different pull request.
| loop: asyncio.AbstractEventLoop | None = None, | ||
| name: str | None = MISSING, | ||
| overlap: bool | int = False, | ||
| create_loop: bool = False, |
There was a problem hiding this comment.
Technically makes this breaking I believe. True default probably wouldn't be most ideal though?
| return task | ||
|
|
||
| def dispatch(self, event: str, *args: Any, **kwargs: Any) -> None: | ||
| _log.debug("Dispatching event %s", event) |
| **options: Any, | ||
| ) -> None: | ||
| self.loop: asyncio.AbstractEventLoop = loop | ||
| self.loop: asyncio.AbstractEventLoop = loop or MISSING |
There was a problem hiding this comment.
Is there a difference between using None and MISSING here?
Summary
This PR refactors the
Client.runlogic to fix problems involving asyncio due to how the library used the loop:Needs testing
Exception
Information
examples, ...).
Checklist
type: ignorecomments were used, a comment is also left explaining why.