Re-think approach for creating sessions in the Bot

[MarkKoz]

Background

How sessions are currently created

There are things the Bot needs to create which rely on a running event loop like aiohttp ClientSessions. This is done in Bot._recreate():

bot/bot/bot.py

Lines 87 to 117 in b256195

	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.
	# 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"
	)
	# 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.recreate(force=True, connector=self._connector)

Because discord.py manages the event loop, we currently hook into Bot.login() and create everything in there. This is done indirectly by calling _recreate() within login():

bot/bot/bot.py

Lines 81 to 85 in b256195

	async def login(self, *args, **kwargs) -> None:
	"""Re-create the connector and set up sessions before logging into Discord."""
	self._recreate()
	await self.stats.create_socket()
	await super().login(*args, **kwargs)

Resetting the bot with clear()

Closing

A discord.Client instance is designed to be re-usable. The client can be closed with close(), which also closes the internal ClientSession and sets the client's internal state to "closed".

Logging in

One can still login() again in this closed state. When logging in, discord.py will re-create its internal ClientSession. Keep in mind that not all of discord.py functionality requires a running bot; for some API interactions, merely logging in is sufficient.

Running the bot & using clear()

While logging in will work in a closed state, running the bot will not. Hence, to run again after closing, clear() needs to be called to reset to an open state. This function also re-creates the internal ClientSession. Note that this function is not a coroutine, so aiohttp will complain that a session is being created outside a coroutine if clear() is called outside a coroutine.

If the Bot subclass is to maintain support for clear(), it has to re-create its own sessions when clear() is called. This is why it calls _recreate(). Since clear() is not a coroutine, _recreate() also cannot be a coroutine.

Scheduled tasks in cogs

Some cogs schedule tasks on the event loop in their __init__s. The scheduling happens before the bot is started, but the tasks won't actually be executed until discord.py starts the event loop (which will happen when we call bot.run()).

Problem

There is a race condition between scheduled tasks and the creation of the sessions upon which they depend. When a task is scheduled like this, the time or order in which it completes is not guaranteed. It's possible that a scheduled task could try to make a request via a ClientSession before the Bot has managed to create that session!

Current solution: events

To solve this, the APIClient has an asyncio.Event which is set when the ClientSession is created:

bot/bot/api.py

Lines 63 to 72 in b256195

	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()

All methods of APIClient wait for this event to be set before sending any requests to ensure the session is available. This pattern works but is cumbersome.

bot/bot/api.py

Lines 109 to 111 in b256195

	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()

Only APIClient uses an event. Neither Bot.http_session nor Bot.stats have events associated with them, and therefore they are open to potential race conditions.

A better solution?

In #947 (comment), @aeros suggested using loop.run_until_complete to create sessions and @SebastiaanZ had a similar idea. This is better because it blocks the event loop and guarantees the sessions will be created before the loop is unblocked. We would have to do this sometime before discord.py starts the loop here. @SebastiaanZ had concerns of whether the object created would be fine with the fact that the loop would temporarily stop running after they're created. There'd be a gap in which the loop is stopped between when the objects are finished being created and when discord.py starts the loop back up to run the bot.

This suggestion should be investigated further.

Merits of supporting clear()

While it's sort of nice to say that our subclass still maintains the API discord.py provides, we never actually use the bot like this. We never try to re-use the client after it's been closed. If it's proving complicated to maintain support for it while still properly creating our sessions, then we should probably just drop support for clear().

[aeros]

@SebastiaanZ had concerns of whether the object created would be fine with the fact that the loop would temporarily stop running after they're created. There'd be a gap in which the loop is stopped between when the objects are finished being created and when discord.py starts the loop back up to run the bot.

Although I have not specifically tried this w/ aiohttp, I don't believe this will be an issue as long as the event loop is not closed during the "gap". It's definitely worth some experimenting and testing it though to verify it works as intended.

[MarkKoz]

This was somewhat addressed in #1333. Basically, I dropped support for clear(), which made it possible to await things instead of scheduling background tasks. There shouldn't be any more race conditions of that kind.

However, there could still be issues where cog __init__s have create_tasks that rely on one of the clients. The client will be None until the bot logs in but tasks get scheduled before client.run() is even called. The solution would be to have those tasks wait for the ready/guild available events. The idea of using loop.run_until_complete for these stills stands.

[wookie184]

I don't think this is an issue any more, as all the clients used are guaranteed to be created before they are used:

• For the Discord one: Extensions are loaded from discord.Client.setup_hook which is called after the client is made
• For the main bot one: It's passed in before the bot instance is even made

  bot/bot/__main__.py

  Lines 55 to 58 in e5bab98

  	async with aiohttp.ClientSession() as session:
  	bot.instance = Bot(
  	guild_id=constants.Guild.id,
  	http_session=session,

• For the site API one: It's created in the __init__ so can't be used before that https://github.com/python-discord/bot-core/blob/07f517225fc59157233f74edb136ee39046fda00/pydis_core/site_api.py#L69
• For the stats one: It's also created in the setup_hook, https://github.com/python-discord/bot-core/blob/07f517225fc59157233f74edb136ee39046fda00/pydis_core/_bot.py#L272, and this is before the extensions are loaded

  bot/bot/bot.py

  Lines 50 to 51 in e5bab98

  	await super().setup_hook()
  	await self.load_extensions(exts)