Clean improvements - #3535
Conversation
mbaruh
added
a: moderation
| self._running_tasks = set() | ||
|
|
||
| def submit[T](self, coro: Awaitable[T]) -> asyncio.Task[T]: | ||
| """Wraps the coroutine with _semaphore logic, schedules it on the event loop, and ensures cleanup.""" |
There was a problem hiding this comment.
Nit: Use imperative mood for docs "Wrap the coroutine" rather than "Wraps the coroutine".
Also, _semaphore is an internal detail so it shouldn't be directly referenced in the docstring.
There was a problem hiding this comment.
hah the underscore is because of a refactor, but yeah don't have to mention how it's implemented.
|
|
||
| async def gather(self, return_exceptions: bool = False) -> list[Any]: | ||
| """Waits for all submitted coroutines to finish execution.""" | ||
| if self._running_tasks: |
There was a problem hiding this comment.
Nit: I think this check is redundant
There was a problem hiding this comment.
oh I thought an empty asyncio.gather raised an error, I guess not
| await self._event.wait() | ||
|
|
||
|
|
||
| class AsyncExecutor: |
There was a problem hiding this comment.
Should there be a public interface for cancelling the tasks? Might be needed if the bot is shutting down.
| """Return the datetime of the earliest message cached, or now if the cache is empty.""" | ||
| return self.bot.cached_messages[0].created_at if self.bot.cached_messages else arrow.utcnow().datetime | ||
|
|
||
| def _use_cache(self, most_recent_limit: datetime | None) -> bool: |
There was a problem hiding this comment.
Nit: Inconsistent terminology in this file. There's earliest/latest, lower/upper, first/second, and least/most recent.
| continue | ||
|
|
||
| messages_to_delete.append(message) | ||
| message_ids.append(message.id) |
There was a problem hiding this comment.
Nit: message IDs can be derived from messages_to_delete. I think it'd be cleaner to not maintain parallel lists, and that it wouldn't be a significant change in performance.
| self._running_tasks.clear() | ||
| return result | ||
|
|
||
| def cancel_all(self) -> None: |
There was a problem hiding this comment.
Should we wait for the tasks to be cancelled or is it fine to fire and forget?
There was a problem hiding this comment.
Do we have anything to do if we wait? Tasks that haven't started yet are cleared, and tasks that are already ongoing get cancelled. But if the task e.g. made an API call to Discord, we have no choice but let it play out. Is there something beyond cancelling it that we can realistically do? In the case of the clean cog, I think it's ok even if we let tasks that already started finish.
|
|
||
| def cog_unload(self) -> None: | ||
| """Stop any ongoing clean.""" | ||
| self.cleaning = False |
There was a problem hiding this comment.
Why not cancel the tasks in here? Is setting cleaning to False guaranteed to cancel the tasks if any exist?
There was a problem hiding this comment.
If there's a running _clean_messages coro, it will stop the cleaning process, hit the check in line 489 and cancel the tasks. I can also cancel it here for good measure, but that function is the central point for handling this flag. What do you think?
There was a problem hiding this comment.
I also don't have the tasks here, because I've found holding in the cog tasks specific to a command call awkward
| if not self.cleaning: | ||
| return None | ||
| if old_messages: | ||
| log.trace("Some of the found messages are older than 14d, and will deleted individually.") |
There was a problem hiding this comment.
Typo: "will deleted" -> "will be deleted"
2 participants
No description provided.