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

Incomplete Error Stack Trace Capturing #2496

Closed
1 of 2 tasks
kyranet opened this issue Apr 22, 2018 · 5 comments · Fixed by #2744 or #4835
Closed
1 of 2 tasks

Incomplete Error Stack Trace Capturing #2496

kyranet opened this issue Apr 22, 2018 · 5 comments · Fixed by #2744 or #4835

Comments

@kyranet
Copy link
Member

kyranet commented Apr 22, 2018

Please describe the problem you are having in as much detail as possible:

For months, the error stack traces were incredibly poor in DiscordAPIError instances, where in small bots this is not such as problem since the code can be found and fixed easily, this becomes a more serious issue when it comes to massive bots that have dozens of thousands lines of code, where a single error/bug is already very hard to find and debug without a proper error stack.

DiscordAPIError: Missing Permissions
     at item.request.gen.end (/.../node_modules/discord.js/src/rest/handlers/RequestHandler.js:79:65)
     at then (/.../node_modules/snekfetch/src/index.js:218:21)
     at process._tickCallback (internal/process/next_tick.js:178:7)

Whilst I understand errors should be handled (and they are, those errors are handled by either try/catch or with Promise#catch()), but generally speaking, you can have many functions that interact with the Discord API that may not be handled correctly, this risk, again, becomes bigger as the bot's code becomes larger.

Multipurpose bots (like mine) are greatly affected by this issue, since they make many kinds of requests or abstract complex "modules" separately from what comes to be "pieces" (most of them use a framework) such as commands, events... e.g. starboard, where it interacts with messages listening to the messageReactionAdd event, check in the cache if the message was starred already (or check a database, in more complex modules), then send in case the message wasn't starred, or edit it. There are many kinds of things a single module can do for the sake of working correctly.

I understand that the Discord API throws generic error codes (such as in this case, 50013 [Missing Permissions]), and discord.js handles those errors correctly, the stack is, however, incomplete. The code 50013, and the name Missing Permissions, are too generic. We also have DiscordAPIError#path which, thankfully, tells us the endpoint used so we can recognize which type (reaction, message create, message edit, message delete...) (but does not tell us which HTTP request has been used!, message edit and message delete have the same URL, but the former is a PATCH and the latter is a DELETE).

The line that throws the error (with an useless stack) is here:

item.reject(err.status >= 400 && err.status < 500 ? new DiscordAPIError(res.request.path, res.body) : err);
.

The error handling has been done in multiple ways, but the following is the only one that ensures an almost-accurate error stack, however, this is ugly, large, and be misleading when chaining multiple API requests:

const { DiscordAPIError } = require('discord.js');

// ...
try {
  // Do an API operation without permissions or a malformed
  // API request, e.g. messages with a too-long content, embeds
  // with many fields...
} catch (error) {
  if (error instanceof DiscordAPIError) Error.captureStackTrace(error);
  console.error(error);
}
// ...

Which outputs this to console (the test has been done with eval):

DiscordAPIError: Missing Permissions
    at eval (eval at eval (/.../commands/System/Admin/eval.js:96:13), <anonymous>:1:7)
    at module.exports.eval (/.../commands/System/Admin/eval.js:96:13)
    at module.exports.timedEval (/.../commands/System/Admin/eval.js:84:9)
    at module.exports.run (/.../commands/System/Admin/eval.js:20:54)
    at module.exports.runCommand (/.../node_modules/klasa/src/monitors/commandHandler.js:89:90)
    at process._tickCallback (internal/process/next_tick.js:178:7)

And the error stack above is more helpful, it tells the end developer the line that failed. Unfortunately, I am not very confident with discord.js' REST so I do not know what would be causing the issue, or if it's caused by snekfetch (since it's where the stack starts at). But I will be very thankful if this issue gets solved. It is not like it's in the top of my priorities, but I have created this issue to point out two issues:

  • The DiscordAPIError class does capture the path, but not the method (as mentioned before, message edit and message delete have the same endpoint, but they differ on the method).
  • The Error stacks are incomplete and are too hard to debug in large bots since they do not give us the location of our code that caused the issue.

Include a reproducible code sample here, if possible:

In a channel your bot has no permissions to send a message (to make it throw a DiscordAPIError):

message.channel.send('Test')
  .catch(console.error);

Further details:

  • discord.js version: master b8a9a76

  • node.js version: Node.js 8.11.1 / 9.11.1 / 10.0.0-rc.1 (Happens in any compatible version)

  • Operating system: Windows 10 Fall Creators Update, Ubuntu 16.04

  • Priority this issue should have: low, this issue can be monkey patched (although losing performance for this), and a very refined error handler can display half of the details needed to find the issue.

  • I found this issue while running code on a user account

  • I have also tested the issue on latest master, commit hash: b8a9a76cf6d9a1ac59b95913ed9bc7dc7d0f61da (not latest, but /src/rest hasn't changed in 4 months).

@ThunderFrost0001
Copy link

UPDATE! The files will no longer appear at src/rest/handlers/RequestHandler.js , it was
at src/client/rest/handlers/RequestHandler.js now!

@devsnek
Copy link
Member

devsnek commented Jun 2, 2018

the missing frames happen as soon as an async context is entered. this affects all code run in node, not just discord.js

this is an ongoing issue with the node.js diagnostics team is working on.

@MrJacz
Copy link
Contributor

MrJacz commented Jun 3, 2018

@MutantFrost The example error is using Discord.js master, the folder structured compared to stable has changed.

@iCrawl
Copy link
Member

iCrawl commented Aug 10, 2018

This seems more like a node issue overall, really don't think we should do any heavy lifting here.

@kyranet
Copy link
Member Author

kyranet commented Aug 10, 2018

One of the things I have though of was to use Error.captureStackTrace in a part outside the async context @devsnek described, such as in the router, but I have never tested it.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 27, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
6 participants