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

feat(Rest): switch queue to AsyncQueue #4835

Merged
merged 4 commits into from
Sep 25, 2020

Conversation

kyranet
Copy link
Member

@kyranet kyranet commented Sep 20, 2020

Please describe the changes this PR makes and why it should be merged:

Better alternative to #2744.

When running the following code as an example message.channel.send();, REST gives us this stack trace:

DiscordAPIError: Cannot send an empty message
    at RequestHandler.execute (***/node_modules/discord.js/src/rest/RequestHandler.js:170:25)
    at runMicrotasks (<anonymous>)
    at processTicksAndRejections (internal/process/task_queues.js:93:5)

After this patch, it returns this one:

DiscordAPIError: Cannot send an empty message
    at RequestHandler.execute (***/node_modules/discord.js/src/rest/RequestHandler.js:157:13)
    at processTicksAndRejections (internal/process/task_queues.js:93:5)
    at async RequestHandler.push (***/node_modules/discord.js/src/rest/RequestHandler.js:39:14)
    at async Object.eval (***/dist/commands/System/Admin/eval.js:73:26)
    at async Object.run (***/dist/commands/System/Admin/eval.js:18:49)
    at async default_1.runCommand (***/dist/monitors/commandHandler.js:57:38)
    ...

Which correctly points to where I ran my eval command (same code before and after this patch).

Typings are unpatched until further decision from the team on whether we should use discord.js v13's core package (once released, will come with its typings), or stick with this implementation in the meantime.

There are a few modifications needed for this to work, but since the modified properties were inside the library's internals (which are private), I believe this falls under semver: patch, and we can ship this in v12.4.

What is better about this compared to the previous system?

  • Can be easily tweaked to support retries on AbortError, which can fix 2 issues in one go.
  • Complete async stack traces (async queue lock implementation, needs Node.js v12, which the library already needs, V8's async stack traces are very new).
  • Impossible to dead-lock by design, if an error happened during the handler, it'd hang as it needs to call reject(), with this patch, it'd bubble up, and a try/finally takes care of resuming the queue.

Status

  • Code changes have been tested against the Discord API, or there are no code changes
  • I know how to update typings and have done so, or typings don't need updating

Semantic versioning classification:

  • This PR changes the library's interface (methods or parameters added)
    • This PR includes breaking changes (methods removed or renamed, parameters moved or removed)
  • This PR only includes non-code changes, like changes to documentation, README, etc.

@kyranet kyranet mentioned this pull request Sep 20, 2020
1 task
src/rest/AsyncQueue.js Outdated Show resolved Hide resolved
src/rest/AsyncQueue.js Outdated Show resolved Hide resolved
src/rest/AsyncQueue.js Outdated Show resolved Hide resolved
Co-authored-by: Sugden <28943913+NotSugden@users.noreply.github.com>
Copy link
Contributor

@papaia papaia left a comment

Choose a reason for hiding this comment

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

Shouldn't AsyncQueue be a private class?

Copy link
Member

@vladfrangu vladfrangu left a comment

Choose a reason for hiding this comment

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

Unless we want to wait a bit more for v13's rest, LGTM

@iCrawl iCrawl merged commit 32fe72f into discordjs:master Sep 25, 2020
@kyranet kyranet deleted the feat/async-queue branch September 27, 2020 12:50
GiorgioBrux pushed a commit to GiorgioBrux/discord.js that referenced this pull request Oct 1, 2020
Co-authored-by: Sugden <28943913+NotSugden@users.noreply.github.com>
GiorgioBrux pushed a commit to GiorgioBrux/discord.js that referenced this pull request Oct 17, 2020
Co-authored-by: Sugden <28943913+NotSugden@users.noreply.github.com>
AGuyNamedJens added a commit to AGuyNamedJens/discord.js that referenced this pull request Oct 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants