Skip to content

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
Co-authored-by: Sugden <28943913+NotSugden@users.noreply.github.com>
@iCrawl iCrawl requested a review from SpaceEEC September 22, 2020 07:33
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

Development

Successfully merging this pull request may close these issues.

Support more complete async stack traces when failures occur during API calls Reactions getting stuck Incomplete Error Stack Trace Capturing

6 participants