Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

reorder some startup to prevent heartbeat issues #3073

Merged
merged 14 commits into from Nov 9, 2019
Merged

reorder some startup to prevent heartbeat issues #3073

merged 14 commits into from Nov 9, 2019

Conversation

mikeshardmind
Copy link
Contributor

@mikeshardmind mikeshardmind commented Oct 19, 2019

Type

  • Bugfix
  • Enhancement
  • New feature

Description of the changes

resolves #3045

@mikeshardmind mikeshardmind added the Type: Enhancement label Oct 19, 2019
@mikeshardmind mikeshardmind added this to the 3.2.0 milestone Oct 19, 2019
@mikeshardmind
Copy link
Contributor Author

@mikeshardmind mikeshardmind commented Oct 20, 2019

Context on the neccessity of this change:

It brought a larger Red's time to responsiveness down from a 15-16 minute window, all the way down to under 2 minutes, most of which is spent syncing with the gateway.

A note:

While it wasn't safe before due to ordering concerns, it is now completely unsafe to use bot.wait_until_ready in cog startup. If a cog absolutely needs to bot to be ready before doing anything, it should use a background task which waits until ready and doesnt allow the cog to do anything until that is finished.

@mikeshardmind mikeshardmind added the Breaking Change label Oct 20, 2019
@DevilXD
Copy link
Contributor

@DevilXD DevilXD commented Oct 22, 2019

Does this mean that using bot.wait_until_ready() inside the extension's async setup function, to wait until the cache is ready before adding the cog, will just make everything hang on startup?

@mikeshardmind
Copy link
Contributor Author

@mikeshardmind mikeshardmind commented Oct 22, 2019

Yes, that's why this is marked as breaking

@mikeshardmind
Copy link
Contributor Author

@mikeshardmind mikeshardmind commented Oct 22, 2019

I've gone ahead and added a timeout on loading extensions. The timeout is only in use during the bot's initial load not part of the normal method of adding extensions, and should prevent infinite hangs should a cog be doing this. I've not put this at a lower level because this is only added to prevent infinite hangs during startup.

It wont prevent a cog from doing time.sleep(2**1000 * 60), but nothing was preventing that already. Timeout is handled using asyncio.wait_for, and is relatively generous at 30 seconds per cog.

@mikeshardmind
Copy link
Contributor Author

@mikeshardmind mikeshardmind commented Nov 4, 2019

Last 4 commits address additional preexisting issues, the effects of which were amplified and made more noticeable by the startup changes.

Kowlin
Kowlin approved these changes Nov 9, 2019
Copy link
Member

@Kowlin Kowlin left a comment

Can confirm that this PR works, cannot confirm a performance increase due the fact that I don't have a bot with the needed capacity. maybe @PredaaA can share some of his thoughts on this.

@mikeshardmind mikeshardmind merged commit b3363ac into Cog-Creators:V3/develop Nov 9, 2019
1 check passed
@mikeshardmind mikeshardmind deleted the reorder-startup branch Nov 9, 2019
@PredaaA
Copy link
Member

@PredaaA PredaaA commented Nov 9, 2019

Can confirm that this PR works, cannot confirm a performance increase due the fact that I don't have a bot with the needed capacity. maybe @PredaaA can share some of his thoughts on this.

Yes, in my case the startup takes now ~1min 30, instead of over 15 minutes of hell before. And all that in peace without getting any heartbeat blocked and any disconnects.

@Drapersniper
Copy link
Contributor

@Drapersniper Drapersniper commented Nov 9, 2019

@PredaaA did you not mention that you saw an issue with this where all commands in a cog stop working after reloading the cog?

@PredaaA
Copy link
Member

@PredaaA PredaaA commented Nov 9, 2019

Huhh no I forgot about this one. But well, yeah, but we wasn't sure if it was related to this PR. Basically after a restart audio commands wasn't working.

@kennnyshiwa
Copy link
Contributor

@kennnyshiwa kennnyshiwa commented Nov 9, 2019

As draper mentioned above, with this PR both @PredaaA and I have seen an odd issue where specifically with audio none of the commands will work, you can do the help for them, like [p]help play will work but not [p]play test or any other sub commands, reloading audio doesn't do anything and only way to fix is to restart the bot again in which audio commands start working

@mikeshardmind
Copy link
Contributor Author

@mikeshardmind mikeshardmind commented Nov 9, 2019

PR included a handful of changes to audio to address those issues after they were brought up in discord.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Breaking Change Type: Enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants