From edffd9b59dcf275848076291ea22aae5e71326dc Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Thu, 13 Feb 2020 09:03:44 -0800 Subject: API: accept additional session kwargs for recreate() These kwargs are merged with the kwargs given when the APIClient was created. This is useful for facilitating changing the session's connector with a new instance when the session needs to be recreated. * Rename _session_args attribute to _default_session_kwargs --- bot/api.py | 23 ++++++++++++++++------- 1 file changed, 16 insertions(+), 7 deletions(-) diff --git a/bot/api.py b/bot/api.py index 56db99828..c168a869d 100644 --- a/bot/api.py +++ b/bot/api.py @@ -47,7 +47,7 @@ class APIClient: self._ready = asyncio.Event(loop=loop) self._creation_task = None - self._session_args = kwargs + self._default_session_kwargs = kwargs self.recreate() @@ -55,9 +55,13 @@ class APIClient: def _url_for(endpoint: str) -> str: return f"{URLs.site_schema}{URLs.site_api}/{quote_url(endpoint)}" - async def _create_session(self) -> None: - """Create the aiohttp session and set the ready event.""" - self.session = aiohttp.ClientSession(**self._session_args) + async def _create_session(self, **session_kwargs) -> None: + """ + Create the aiohttp session with `session_kwargs` and set the ready event. + + `session_kwargs` is merged with `_default_session_kwargs` and overwrites its values. + """ + self.session = aiohttp.ClientSession(**{**self._default_session_kwargs, **session_kwargs}) self._ready.set() async def close(self) -> None: @@ -68,12 +72,17 @@ class APIClient: await self.session.close() self._ready.clear() - def recreate(self) -> None: - """Schedule the aiohttp session to be created if it's been closed.""" + def recreate(self, **session_kwargs) -> None: + """ + Schedule the aiohttp session to be created with `session_kwargs` if it's been closed. + + `session_kwargs` is merged with the kwargs given when the `APIClient` was created and + overwrites those default kwargs. + """ if self.session is None or self.session.closed: # Don't schedule a task if one is already in progress. if self._creation_task is None or self._creation_task.done(): - self._creation_task = self.loop.create_task(self._create_session()) + self._creation_task = self.loop.create_task(self._create_session(**session_kwargs)) async def maybe_raise_for_status(self, response: aiohttp.ClientResponse, should_raise: bool) -> None: """Raise ResponseCodeError for non-OK response if an exception should be raised.""" -- cgit v1.2.3 From b19e6aaabdbcfefc5d22d1f72e670421fe8a8d97 Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Thu, 13 Feb 2020 10:18:09 -0800 Subject: Bot: avoid DeprecationWarning for aiohttp.AsyncResolver (fix #748) AsyncResolver has to be created inside a coroutine so it's moved inside start(). Consequently, the APIClient session is also recreated inside start() now. When using clear(), the default connector is used for the session it recreates because clear() is not a coroutine. This should only affect requests made to the Discord API via the Client when not using it to run a bot; starting the bot will re-create the session with the custom connector. * Close connector and resolver when bot closes --- bot/bot.py | 45 ++++++++++++++++++++++++++++++--------------- 1 file changed, 30 insertions(+), 15 deletions(-) diff --git a/bot/bot.py b/bot/bot.py index 8f808272f..95fbae17f 100644 --- a/bot/bot.py +++ b/bot/bot.py @@ -14,18 +14,13 @@ class Bot(commands.Bot): """A subclass of `discord.ext.commands.Bot` with an aiohttp session and an API client.""" def __init__(self, *args, **kwargs): - # Use asyncio for DNS resolution instead of threads so threads aren't spammed. - # Use AF_INET as its socket family to prevent HTTPS related problems both locally - # and in production. - self.connector = aiohttp.TCPConnector( - resolver=aiohttp.AsyncResolver(), - family=socket.AF_INET, - ) - - super().__init__(*args, connector=self.connector, **kwargs) + super().__init__(*args, **kwargs) self.http_session: Optional[aiohttp.ClientSession] = None - self.api_client = api.APIClient(loop=self.loop, connector=self.connector) + self.api_client = api.APIClient(loop=self.loop) + + self._connector = None + self._resolver = None log.addHandler(api.APILoggingHandler(self.api_client)) @@ -35,19 +30,39 @@ class Bot(commands.Bot): log.info(f"Cog loaded: {cog.qualified_name}") def clear(self) -> None: - """Clears the internal state of the bot and resets the API client.""" + """Clears the internal state of the bot and sets the HTTPClient connector to None.""" + self.http.connector = None # Use the default connector. super().clear() - self.api_client.recreate() async def close(self) -> None: - """Close the aiohttp session after closing the Discord connection.""" + """Close the Discord connection and the aiohttp session, connector, and resolver.""" await super().close() await self.http_session.close() await self.api_client.close() + if self._connector: + await self._connector.close() + + if self._resolver: + await self._resolver.close() + async def start(self, *args, **kwargs) -> None: - """Open an aiohttp session before logging in and connecting to Discord.""" - self.http_session = aiohttp.ClientSession(connector=self.connector) + """Set up aiohttp sessions before logging in and connecting to Discord.""" + # Use asyncio for DNS resolution instead of threads so threads aren't spammed. + # Use AF_INET as its socket family to prevent HTTPS related problems both locally + # and in production. + self._resolver = aiohttp.AsyncResolver() + self._connector = aiohttp.TCPConnector( + resolver=self._resolver, + family=socket.AF_INET, + ) + + # Client.login() will call HTTPClient.static_login() which will create a session using + # this connector attribute. + self.http.connector = self._connector + + self.http_session = aiohttp.ClientSession(connector=self._connector) + self.api_client.recreate(connector=self._connector) await super().start(*args, **kwargs) -- cgit v1.2.3 From 253073ad059fc3a8eac890b4f3fe006454aae4b0 Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Thu, 13 Feb 2020 10:20:45 -0800 Subject: Bot: add warning for when connector is a specified kwarg --- bot/bot.py | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/bot/bot.py b/bot/bot.py index 95fbae17f..762d316bf 100644 --- a/bot/bot.py +++ b/bot/bot.py @@ -1,5 +1,6 @@ import logging import socket +import warnings from typing import Optional import aiohttp @@ -14,6 +15,11 @@ class Bot(commands.Bot): """A subclass of `discord.ext.commands.Bot` with an aiohttp session and an API client.""" def __init__(self, *args, **kwargs): + if "connector" in kwargs: + warnings.warn( + "If the bot is started, the connector will be overwritten with an internal one" + ) + super().__init__(*args, **kwargs) self.http_session: Optional[aiohttp.ClientSession] = None -- cgit v1.2.3 From 9b6c9e8692313bd5ed70ce00c31e5b24e25635b1 Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Fri, 14 Feb 2020 12:23:59 -0800 Subject: Bot: fix error trying to close a None session --- bot/bot.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/bot/bot.py b/bot/bot.py index 762d316bf..67a15faba 100644 --- a/bot/bot.py +++ b/bot/bot.py @@ -44,9 +44,11 @@ class Bot(commands.Bot): """Close the Discord connection and the aiohttp session, connector, and resolver.""" await super().close() - await self.http_session.close() await self.api_client.close() + if self.http_session: + await self.http_session.close() + if self._connector: await self._connector.close() -- cgit v1.2.3 From a21f4e63680e55149c33ee0bdde938281a8eb020 Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Sat, 15 Feb 2020 11:05:42 -0800 Subject: Bot: override login() instead of start() The client can be used without running a bot so it makes more sense for the connector to be created when logging in, which is done in both cases, rather than in start(), which is only used when running a bot. --- bot/bot.py | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/bot/bot.py b/bot/bot.py index 67a15faba..1d187f031 100644 --- a/bot/bot.py +++ b/bot/bot.py @@ -17,7 +17,8 @@ class Bot(commands.Bot): def __init__(self, *args, **kwargs): if "connector" in kwargs: warnings.warn( - "If the bot is started, the connector will be overwritten with an internal one" + "If login() is called (or the bot is started), the connector will be overwritten " + "with an internal one" ) super().__init__(*args, **kwargs) @@ -55,8 +56,8 @@ class Bot(commands.Bot): if self._resolver: await self._resolver.close() - async def start(self, *args, **kwargs) -> None: - """Set up aiohttp sessions before logging in and connecting to Discord.""" + async def login(self, *args, **kwargs) -> None: + """Re-create the connector and set up sessions before logging into Discord.""" # Use asyncio for DNS resolution instead of threads so threads aren't spammed. # Use AF_INET as its socket family to prevent HTTPS related problems both locally # and in production. @@ -73,4 +74,4 @@ class Bot(commands.Bot): self.http_session = aiohttp.ClientSession(connector=self._connector) self.api_client.recreate(connector=self._connector) - await super().start(*args, **kwargs) + await super().login(*args, **kwargs) -- cgit v1.2.3 From 5cc4e360aee28832ace207d8df2fb17b487fbfe7 Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Sat, 15 Feb 2020 11:21:09 -0800 Subject: Bot: move connector/session recreation to a separate function The function itself doesn't need to be a coroutine. It just has to be called in a coroutine (or, more indirectly, in an async context?). --- bot/bot.py | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/bot/bot.py b/bot/bot.py index 1d187f031..e1b1d81dc 100644 --- a/bot/bot.py +++ b/bot/bot.py @@ -58,6 +58,11 @@ class Bot(commands.Bot): async def login(self, *args, **kwargs) -> None: """Re-create the connector and set up sessions before logging into Discord.""" + self._recreate() + await super().login(*args, **kwargs) + + def _recreate(self) -> None: + """Re-create the connector, aiohttp session, and the APIClient.""" # Use asyncio for DNS resolution instead of threads so threads aren't spammed. # Use AF_INET as its socket family to prevent HTTPS related problems both locally # and in production. @@ -73,5 +78,3 @@ class Bot(commands.Bot): self.http_session = aiohttp.ClientSession(connector=self._connector) self.api_client.recreate(connector=self._connector) - - await super().login(*args, **kwargs) -- cgit v1.2.3 From 6b689a15be69120a775789892f155c736926ef07 Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Sat, 15 Feb 2020 11:22:29 -0800 Subject: Bot: call _recreate() in clear() Because discord.py recreates the HTTPClient session, may as well follow suite and recreate our own stuff here too. --- bot/bot.py | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/bot/bot.py b/bot/bot.py index e1b1d81dc..9f48c980c 100644 --- a/bot/bot.py +++ b/bot/bot.py @@ -37,8 +37,14 @@ class Bot(commands.Bot): log.info(f"Cog loaded: {cog.qualified_name}") def clear(self) -> None: - """Clears the internal state of the bot and sets the HTTPClient connector to None.""" - self.http.connector = None # Use the default connector. + """ + Clears the internal state of the bot and recreates the connector and sessions. + + Will cause a DeprecationWarning if called outside a coroutine. + """ + # Because discord.py recreates the HTTPClient session, may as well follow suite and recreate + # our own stuff here too. + self._recreate() super().clear() async def close(self) -> None: -- cgit v1.2.3 From a417c318e6a0e57fa53b9b68572a524e0aa0f729 Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Sat, 15 Feb 2020 11:38:27 -0800 Subject: Bot: warn when connector/session not closed when recreating aiohttp does warn too, but these warnings will provide more immediate feedback. --- bot/bot.py | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/bot/bot.py b/bot/bot.py index 9f48c980c..0287ec925 100644 --- a/bot/bot.py +++ b/bot/bot.py @@ -72,7 +72,18 @@ class Bot(commands.Bot): # Use asyncio for DNS resolution instead of threads so threads aren't spammed. # Use AF_INET as its socket family to prevent HTTPS related problems both locally # and in production. + + # Doesn't seem to have any state with regards to being closed, so no need to worry? self._resolver = aiohttp.AsyncResolver() + + # Does have a closed state. Its __del__ will warn about this, but let's do it immediately. + if self._connector and not self._connector._closed: + warnings.warn( + "The previous connector was not closed; it will remain open and be overwritten", + ResourceWarning, + stacklevel=2 + ) + self._connector = aiohttp.TCPConnector( resolver=self._resolver, family=socket.AF_INET, @@ -82,5 +93,12 @@ class Bot(commands.Bot): # this connector attribute. self.http.connector = self._connector + if self.http_session and not self.http_session.closed: + warnings.warn( + "The previous ClientSession was not closed; it will remain open and be overwritten", + ResourceWarning, + stacklevel=2 + ) + self.http_session = aiohttp.ClientSession(connector=self._connector) self.api_client.recreate(connector=self._connector) -- cgit v1.2.3 From f4e569b56d52f4e29024be4fdbae796e4d85aea6 Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Sat, 15 Feb 2020 11:51:29 -0800 Subject: Bot: send not-closed warnings as log messages "Real" warnings weren't showing up for some reason. --- bot/bot.py | 20 ++++++++------------ 1 file changed, 8 insertions(+), 12 deletions(-) diff --git a/bot/bot.py b/bot/bot.py index 0287ec925..088b94a1f 100644 --- a/bot/bot.py +++ b/bot/bot.py @@ -70,20 +70,17 @@ class Bot(commands.Bot): def _recreate(self) -> None: """Re-create the connector, aiohttp session, and the APIClient.""" # Use asyncio for DNS resolution instead of threads so threads aren't spammed. - # Use AF_INET as its socket family to prevent HTTPS related problems both locally - # and in production. - # Doesn't seem to have any state with regards to being closed, so no need to worry? self._resolver = aiohttp.AsyncResolver() - # Does have a closed state. Its __del__ will warn about this, but let's do it immediately. + # Its __del__ does send a warning but it doesn't always show up for some reason. if self._connector and not self._connector._closed: - warnings.warn( - "The previous connector was not closed; it will remain open and be overwritten", - ResourceWarning, - stacklevel=2 + log.warning( + "The previous connector was not closed; it will remain open and be overwritten" ) + # Use AF_INET as its socket family to prevent HTTPS related problems both locally + # and in production. self._connector = aiohttp.TCPConnector( resolver=self._resolver, family=socket.AF_INET, @@ -93,11 +90,10 @@ class Bot(commands.Bot): # this connector attribute. self.http.connector = self._connector + # Its __del__ does send a warning but it doesn't always show up for some reason. if self.http_session and not self.http_session.closed: - warnings.warn( - "The previous ClientSession was not closed; it will remain open and be overwritten", - ResourceWarning, - stacklevel=2 + log.warning( + "The previous session was not closed; it will remain open and be overwritten" ) self.http_session = aiohttp.ClientSession(connector=self._connector) -- cgit v1.2.3 From fd313d2b1dac8b627d3d731e0898fbdff6642616 Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Sat, 15 Feb 2020 12:47:42 -0800 Subject: API: add argument to force recreation of the session --- bot/api.py | 12 +++++++++--- bot/bot.py | 2 +- 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/bot/api.py b/bot/api.py index c168a869d..37c1497fc 100644 --- a/bot/api.py +++ b/bot/api.py @@ -72,16 +72,22 @@ class APIClient: await self.session.close() self._ready.clear() - def recreate(self, **session_kwargs) -> None: + def recreate(self, force: bool = False, **session_kwargs) -> None: """ Schedule the aiohttp session to be created with `session_kwargs` if it's been closed. + If `force` is True, the session will be recreated even if an open one exists. If a task to + create the session is pending, it will be cancelled. + `session_kwargs` is merged with the kwargs given when the `APIClient` was created and overwrites those default kwargs. """ - if self.session is None or self.session.closed: + if force or self.session is None or self.session.closed: + if force and self._creation_task: + self._creation_task.cancel() + # Don't schedule a task if one is already in progress. - if self._creation_task is None or self._creation_task.done(): + if force or self._creation_task is None or self._creation_task.done(): self._creation_task = self.loop.create_task(self._create_session(**session_kwargs)) async def maybe_raise_for_status(self, response: aiohttp.ClientResponse, should_raise: bool) -> None: diff --git a/bot/bot.py b/bot/bot.py index 088b94a1f..3094a27c5 100644 --- a/bot/bot.py +++ b/bot/bot.py @@ -97,4 +97,4 @@ class Bot(commands.Bot): ) self.http_session = aiohttp.ClientSession(connector=self._connector) - self.api_client.recreate(connector=self._connector) + self.api_client.recreate(force=True, connector=self._connector) -- cgit v1.2.3 From 02f5e2fe019f332cb6b9e79b63fee54e41b66732 Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Sat, 15 Feb 2020 12:51:37 -0800 Subject: API: close existing open session before recreating it --- bot/api.py | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/bot/api.py b/bot/api.py index 37c1497fc..8cb429cd5 100644 --- a/bot/api.py +++ b/bot/api.py @@ -60,16 +60,17 @@ class APIClient: Create the aiohttp session with `session_kwargs` and set the ready event. `session_kwargs` is merged with `_default_session_kwargs` and overwrites its values. + If an open session already exists, it will first be closed. """ + await self.close() self.session = aiohttp.ClientSession(**{**self._default_session_kwargs, **session_kwargs}) self._ready.set() async def close(self) -> None: """Close the aiohttp session and unset the ready event.""" - if not self._ready.is_set(): - return + if self.session: + await self.session.close() - await self.session.close() self._ready.clear() def recreate(self, force: bool = False, **session_kwargs) -> None: -- cgit v1.2.3 From cc322adb7486560481cee1e2e5bf511114631d8c Mon Sep 17 00:00:00 2001 From: Mark Date: Tue, 3 Mar 2020 08:50:21 -0800 Subject: Fix typo in comment MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-Authored-By: Leon Sandøy --- bot/bot.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bot/bot.py b/bot/bot.py index 54b092193..950ac6751 100644 --- a/bot/bot.py +++ b/bot/bot.py @@ -44,7 +44,7 @@ class Bot(commands.Bot): Will cause a DeprecationWarning if called outside a coroutine. """ - # Because discord.py recreates the HTTPClient session, may as well follow suite and recreate + # Because discord.py recreates the HTTPClient session, may as well follow suit and recreate # our own stuff here too. self._recreate() super().clear() -- cgit v1.2.3 From eddd515d864666b45f243ddc516981f10cedcda3 Mon Sep 17 00:00:00 2001 From: "S. Co1" Date: Tue, 3 Mar 2020 21:18:40 -0500 Subject: Prevent exception if a watched user sends a DM to the bot The previous embed assumed that the messages would be sent on the server, where the channel would have a name and the message would have a jump URL. For a DM, neither of these are present and an exception will be raised when attempting to construct the embed for the webhook to send. --- bot/cogs/watchchannels/watchchannel.py | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/bot/cogs/watchchannels/watchchannel.py b/bot/cogs/watchchannels/watchchannel.py index 3667a80e8..479820444 100644 --- a/bot/cogs/watchchannels/watchchannel.py +++ b/bot/cogs/watchchannels/watchchannel.py @@ -9,7 +9,7 @@ from typing import Optional import dateutil.parser import discord -from discord import Color, Embed, HTTPException, Message, errors +from discord import Color, DMChannel, Embed, HTTPException, Message, errors from discord.ext.commands import Cog, Context from bot.api import ResponseCodeError @@ -273,7 +273,14 @@ class WatchChannel(metaclass=CogABCMeta): reason = self.watched_users[user_id]['reason'] - embed = Embed(description=f"{msg.author.mention} in [#{msg.channel.name}]({msg.jump_url})") + if isinstance(msg.channel, DMChannel): + # If a watched user DMs the bot there won't be a channel name or jump URL + # This could technically include a GroupChannel but bot's can't be in those + message_jump = "via DM" + else: + message_jump = f"in [#{msg.channel.name}]({msg.jump_url})" + + embed = Embed(description=f"{msg.author.mention} {message_jump}") embed.set_footer(text=f"Added {time_delta} by {actor} | Reason: {reason}") await self.webhook_send(embed=embed, username=msg.author.display_name, avatar_url=msg.author.avatar_url) -- cgit v1.2.3