Skip to content

Hopefully fix the API session being closed when populating the filter cache on startup#1333

Merged
ks129 merged 5 commits into
masterfrom
bug/backend/bot-j8/api-session-closed
Dec 20, 2020
Merged

Hopefully fix the API session being closed when populating the filter cache on startup#1333
ks129 merged 5 commits into
masterfrom
bug/backend/bot-j8/api-session-closed

Conversation

@MarkKoz
Copy link
Copy Markdown
Contributor

@MarkKoz MarkKoz commented Dec 19, 2020

Fixes BOT-J8.

I've simplified the code for creation sessions upon startup. This was facilitated by dropping support for Bot.clear(), which we never relied on. I go into a bit more detail in the commit messages. I've ran the bot locally and don't have any issues with closed sessions upon startup.

Making the class-reusable is not worth the added complexity.
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.
@MarkKoz MarkKoz added t: bug Something isn't working a: backend Related to internal functionality and utilities (error_handler, logging, security, utils and core) p: 1 - high High Priority labels Dec 19, 2020
@ghost ghost added the needs 2 approvals label Dec 19, 2020
Copy link
Copy Markdown
Member

@jb3 jb3 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good. Works locally.

The client is already instantiated in a coroutine and aiohttp won't
complain. Therefore, scheduling a task to create the session is
redundant.
Forgot to remove these when removing `loop_is_running` in a previous
commit.
@MarkKoz MarkKoz requested a review from Akarys42 as a code owner December 19, 2020 19:34
@coveralls
Copy link
Copy Markdown

Coverage Status

Coverage decreased (-0.03%) to 56.541% when pulling 52ee0da on bug/backend/bot-j8/api-session-closed into 9799020 on master.

Copy link
Copy Markdown
Contributor

@ks129 ks129 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks good, tested locally.

@ghost ghost removed the needs 1 approval label Dec 20, 2020
@ks129 ks129 merged commit 3fbb4db into master Dec 20, 2020
@ks129 ks129 deleted the bug/backend/bot-j8/api-session-closed branch December 20, 2020 07:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

a: backend Related to internal functionality and utilities (error_handler, logging, security, utils and core) p: 1 - high High Priority t: bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants