From d744dc779cd7c3b128cf28830f54a0dc2aecc574 Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Sat, 19 Dec 2020 10:45:33 -0800 Subject: APIClient: simplify session creation Making the class-reusable is not worth the added complexity. --- bot/api.py | 42 ++++++------------------------------------ bot/bot.py | 7 ++++--- 2 files changed, 10 insertions(+), 39 deletions(-) diff --git a/bot/api.py b/bot/api.py index 4b8520582..6436c2b8b 100644 --- a/bot/api.py +++ b/bot/api.py @@ -50,52 +50,22 @@ class APIClient: self.session = None self.loop = loop - self._ready = asyncio.Event(loop=loop) - self._creation_task = None - self._default_session_kwargs = kwargs - - self.recreate() + # It has to be instantiated in a task/coroutine to avoid warnings from aiohttp. + self._creation_task = self.loop.create_task(self._create_session(**kwargs)) @staticmethod def _url_for(endpoint: str) -> str: return f"{URLs.site_schema}{URLs.site_api}/{quote_url(endpoint)}" 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. - 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() + """Create the aiohttp session with `session_kwargs`.""" + self.session = aiohttp.ClientSession(**session_kwargs) async def close(self) -> None: """Close the aiohttp session and unset the ready event.""" if self.session: await self.session.close() - self._ready.clear() - - 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 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 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: """Raise ResponseCodeError for non-OK response if an exception should be raised.""" if should_raise and response.status >= 400: @@ -108,7 +78,7 @@ class APIClient: async def request(self, method: str, endpoint: str, *, raise_for_status: bool = True, **kwargs) -> dict: """Send an HTTP request to the site API and return the JSON response.""" - await self._ready.wait() + await self._creation_task async with self.session.request(method.upper(), self._url_for(endpoint), **kwargs) as resp: await self.maybe_raise_for_status(resp, raise_for_status) @@ -132,7 +102,7 @@ class APIClient: async def delete(self, endpoint: str, *, raise_for_status: bool = True, **kwargs) -> Optional[dict]: """Site API DELETE.""" - await self._ready.wait() + await self._creation_task async with self.session.delete(self._url_for(endpoint), **kwargs) as resp: if resp.status == 204: diff --git a/bot/bot.py b/bot/bot.py index f71f5d1fb..1715f3ca3 100644 --- a/bot/bot.py +++ b/bot/bot.py @@ -32,7 +32,7 @@ class Bot(commands.Bot): self.http_session: Optional[aiohttp.ClientSession] = None self.redis_session = redis_session - self.api_client = api.APIClient(loop=self.loop) + self.api_client = None self.filter_list_cache = defaultdict(dict) self._connector = None @@ -112,7 +112,7 @@ class Bot(commands.Bot): ) self.http_session = aiohttp.ClientSession(connector=self._connector) - self.api_client.recreate(force=True, connector=self._connector) + self.api_client = api.APIClient(loop=self.loop, connector=self._connector) # Build the FilterList cache self.loop.create_task(self.cache_filter_list_data()) @@ -194,7 +194,8 @@ class Bot(commands.Bot): """Close the Discord connection and the aiohttp session, connector, statsd client, and resolver.""" await super().close() - await self.api_client.close() + if self.api_client: + await self.api_client.close() if self.http_session: await self.http_session.close() -- cgit v1.2.3 From 2021c78635de4ae5a9f9835944c87cba699b41c0 Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Sat, 19 Dec 2020 10:46:26 -0800 Subject: APIClient: remove obsolete function --- bot/api.py | 14 -------------- 1 file changed, 14 deletions(-) diff --git a/bot/api.py b/bot/api.py index 6436c2b8b..ce3f2ada3 100644 --- a/bot/api.py +++ b/bot/api.py @@ -110,17 +110,3 @@ class APIClient: await self.maybe_raise_for_status(resp, raise_for_status) return await resp.json() - - -def loop_is_running() -> bool: - """ - Determine if there is a running asyncio event loop. - - This helps enable "call this when event loop is running" logic (see: Twisted's `callWhenRunning`), - which is currently not provided by asyncio. - """ - try: - asyncio.get_running_loop() - except RuntimeError: - return False - return True -- cgit v1.2.3 From 598c4a50c0c4f2e0c2139d6c349ee183010dbf78 Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Sat, 19 Dec 2020 10:59:09 -0800 Subject: Bot: cease support of Bot.clear() Supporting the function means supporting re-use of a closed Bot. However, this functionality is not relied upon by anything nor will it ever be in the foreseeable future. Support of it required scheduling any needed startup coroutines as tasks. This made augmenting the Bot clunky and didn't make it easy to wait for startup coroutines to complete before logging in. --- bot/bot.py | 79 ++++++++++++++++++++++---------------------------------------- 1 file changed, 28 insertions(+), 51 deletions(-) diff --git a/bot/bot.py b/bot/bot.py index 1715f3ca3..b01cbea43 100644 --- a/bot/bot.py +++ b/bot/bot.py @@ -32,7 +32,7 @@ class Bot(commands.Bot): self.http_session: Optional[aiohttp.ClientSession] = None self.redis_session = redis_session - self.api_client = None + self.api_client: Optional[api.APIClient] = None self.filter_list_cache = defaultdict(dict) self._connector = None @@ -77,46 +77,6 @@ class Bot(commands.Bot): for item in full_cache: self.insert_item_into_filter_list_cache(item) - def _recreate(self) -> None: - """Re-create the connector, aiohttp session, the APIClient and the Redis session.""" - # Use asyncio for DNS resolution instead of threads so threads aren't spammed. - # Doesn't seem to have any state with regards to being closed, so no need to worry? - self._resolver = aiohttp.AsyncResolver() - - # Its __del__ does send a warning but it doesn't always show up for some reason. - if self._connector and not self._connector._closed: - log.warning( - "The previous connector was not closed; it will remain open and be overwritten" - ) - - if self.redis_session.closed: - # If the RedisSession was somehow closed, we try to reconnect it - # here. Normally, this shouldn't happen. - self.loop.create_task(self.redis_session.connect()) - - # 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, - ) - - # Client.login() will call HTTPClient.static_login() which will create a session using - # 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: - log.warning( - "The previous session was not closed; it will remain open and be overwritten" - ) - - self.http_session = aiohttp.ClientSession(connector=self._connector) - self.api_client = api.APIClient(loop=self.loop, connector=self._connector) - - # Build the FilterList cache - self.loop.create_task(self.cache_filter_list_data()) - @classmethod def create(cls) -> "Bot": """Create and return an instance of a Bot.""" @@ -180,15 +140,8 @@ class Bot(commands.Bot): return command def clear(self) -> None: - """ - 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 suit and recreate - # our own stuff here too. - self._recreate() - super().clear() + """Not implemented! Re-instantiate the bot instead of attempting to re-use a closed one.""" + raise NotImplementedError("Re-using a Bot object after closing it is not supported.") async def close(self) -> None: """Close the Discord connection and the aiohttp session, connector, statsd client, and resolver.""" @@ -230,7 +183,31 @@ 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() + # Use asyncio for DNS resolution instead of threads so threads aren't spammed. + self._resolver = aiohttp.AsyncResolver() + + # 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, + ) + + # 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 = api.APIClient(loop=self.loop, connector=self._connector) + + if self.redis_session.closed: + # If the RedisSession was somehow closed, we try to reconnect it + # here. Normally, this shouldn't happen. + await self.redis_session.connect() + + # Build the FilterList cache + await self.cache_filter_list_data() + await self.stats.create_socket() await super().login(*args, **kwargs) -- cgit v1.2.3 From c58fbe828781444fc282151c06c2f4fb9057fe59 Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Sat, 19 Dec 2020 11:29:05 -0800 Subject: APIClient: create the session directly in __init__ The client is already instantiated in a coroutine and aiohttp won't complain. Therefore, scheduling a task to create the session is redundant. --- bot/api.py | 29 +++++++++-------------------- bot/bot.py | 2 +- 2 files changed, 10 insertions(+), 21 deletions(-) diff --git a/bot/api.py b/bot/api.py index ce3f2ada3..d93f9f2ba 100644 --- a/bot/api.py +++ b/bot/api.py @@ -37,34 +37,27 @@ class APIClient: session: Optional[aiohttp.ClientSession] = None loop: asyncio.AbstractEventLoop = None - def __init__(self, loop: asyncio.AbstractEventLoop, **kwargs): + def __init__(self, **session_kwargs): auth_headers = { 'Authorization': f"Token {Keys.site_api}" } - if 'headers' in kwargs: - kwargs['headers'].update(auth_headers) + if 'headers' in session_kwargs: + session_kwargs['headers'].update(auth_headers) else: - kwargs['headers'] = auth_headers + session_kwargs['headers'] = auth_headers - self.session = None - self.loop = loop - - # It has to be instantiated in a task/coroutine to avoid warnings from aiohttp. - self._creation_task = self.loop.create_task(self._create_session(**kwargs)) + # aiohttp will complain if APIClient gets instantiated outside a coroutine. Thankfully, we + # don't and shouldn't need to do that, so we can avoid scheduling a task to create it. + self.session = aiohttp.ClientSession(**session_kwargs) @staticmethod def _url_for(endpoint: str) -> str: return f"{URLs.site_schema}{URLs.site_api}/{quote_url(endpoint)}" - async def _create_session(self, **session_kwargs) -> None: - """Create the aiohttp session with `session_kwargs`.""" - self.session = aiohttp.ClientSession(**session_kwargs) - async def close(self) -> None: - """Close the aiohttp session and unset the ready event.""" - if self.session: - await self.session.close() + """Close the aiohttp session.""" + await self.session.close() 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.""" @@ -78,8 +71,6 @@ class APIClient: async def request(self, method: str, endpoint: str, *, raise_for_status: bool = True, **kwargs) -> dict: """Send an HTTP request to the site API and return the JSON response.""" - await self._creation_task - async with self.session.request(method.upper(), self._url_for(endpoint), **kwargs) as resp: await self.maybe_raise_for_status(resp, raise_for_status) return await resp.json() @@ -102,8 +93,6 @@ class APIClient: async def delete(self, endpoint: str, *, raise_for_status: bool = True, **kwargs) -> Optional[dict]: """Site API DELETE.""" - await self._creation_task - async with self.session.delete(self._url_for(endpoint), **kwargs) as resp: if resp.status == 204: return None diff --git a/bot/bot.py b/bot/bot.py index b01cbea43..4ebe0a5c3 100644 --- a/bot/bot.py +++ b/bot/bot.py @@ -198,7 +198,7 @@ class Bot(commands.Bot): self.http.connector = self._connector self.http_session = aiohttp.ClientSession(connector=self._connector) - self.api_client = api.APIClient(loop=self.loop, connector=self._connector) + self.api_client = api.APIClient(connector=self._connector) if self.redis_session.closed: # If the RedisSession was somehow closed, we try to reconnect it -- cgit v1.2.3 From 52ee0dab1e59089a7acc9f08078dee0df3fa40e6 Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Sat, 19 Dec 2020 11:33:37 -0800 Subject: Remove obsolete test cases Forgot to remove these when removing `loop_is_running` in a previous commit. --- tests/bot/test_api.py | 8 -------- 1 file changed, 8 deletions(-) diff --git a/tests/bot/test_api.py b/tests/bot/test_api.py index 99e942813..76bcb481d 100644 --- a/tests/bot/test_api.py +++ b/tests/bot/test_api.py @@ -13,14 +13,6 @@ class APIClientTests(unittest.IsolatedAsyncioTestCase): cls.error_api_response = MagicMock() cls.error_api_response.status = 999 - def test_loop_is_not_running_by_default(self): - """The event loop should not be running by default.""" - self.assertFalse(api.loop_is_running()) - - async def test_loop_is_running_in_async_context(self): - """The event loop should be running in an async context.""" - self.assertTrue(api.loop_is_running()) - def test_response_code_error_default_initialization(self): """Test the default initialization of `ResponseCodeError` without `text` or `json`""" error = api.ResponseCodeError(response=self.error_api_response) -- cgit v1.2.3 From 6115f5a9f9c4c72e3ec7cac02372f10135b836bc Mon Sep 17 00:00:00 2001 From: Matteo Bertucci Date: Sun, 20 Dec 2020 16:56:27 +0100 Subject: Add the clear alias to the clean command --- bot/exts/utils/clean.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bot/exts/utils/clean.py b/bot/exts/utils/clean.py index bf25cb4c2..8acaf9131 100644 --- a/bot/exts/utils/clean.py +++ b/bot/exts/utils/clean.py @@ -191,7 +191,7 @@ class Clean(Cog): channel_id=Channels.mod_log, ) - @group(invoke_without_command=True, name="clean", aliases=["purge"]) + @group(invoke_without_command=True, name="clean", aliases=["clear", "purge"]) @has_any_role(*MODERATION_ROLES) async def clean_group(self, ctx: Context) -> None: """Commands for cleaning messages in channels.""" -- cgit v1.2.3