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

@discordjs/ws throttler wait for identify don't handle errors properly #9688

Closed
Deivu opened this issue Jul 9, 2023 · 1 comment · Fixed by #9701
Closed

@discordjs/ws throttler wait for identify don't handle errors properly #9688

Deivu opened this issue Jul 9, 2023 · 1 comment · Fixed by #9701

Comments

@Deivu
Copy link
Contributor

Deivu commented Jul 9, 2023

Which package is this bug report for?

ws

Issue description

The IIdentifyThrottler.ts waitForIdentify method errors is not being handled by the ws package.

While this works in the SimpleIdentifyThrottler, this will introduce issues when you extend the IdentifyThrottler depending on your needs. You can't be sure that only the shard that sends an abort signal is the only thing that will error here, hence if you do nothing, your shard will be just left connected and heartbeating without any actual connection in it.

To reproduce, just spawn 2 shards, let the first identify, then throw an error on the 2nd shard when it will identify, you will be left with a shard that didn't identify and a potential memory leak due to the promise not resolving and not being removed from map. This could be a big issue as this will make the bot offline in shards where it didn't identify

A fix is to let the WebsocketShard class detect if something errored in waitForIdentify, and check if it's already closed. if not closed, destroy and reconnect.

Code sample

No response

Versions

  • ws: 0.8.3

Issue priority

Medium (should be fixed soon)

Which partials do you have configured?

Not applicable

Which gateway intents are you subscribing to?

Not applicable

I have tested this issue on a development release

No response

@didinele
Copy link
Member

didinele commented Jul 9, 2023

This is more of a feature request in actuality, as per this comment, this is intended behavior at this time:

/**
* Resolves once the given shard should be allowed to identify, or rejects if the operation was aborted.
*/

But indeed, it is less than ideal.

@didinele didinele added this to the ws 0.9.0 milestone Jul 9, 2023
Deivu added a commit to shipgirlproject/Indomitable that referenced this issue Jul 11, 2023
@kodiakhq kodiakhq bot closed this as completed in #9701 Jul 12, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 13, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants