discordjs / discord.js

A powerful JavaScript library for interacting with the Discord API
https://discord.js.org
Apache License 2.0
25.31k stars 3.97k forks source link

Incomplete Error Stack Trace Capturing #2496

Closed kyranet closed 6 years ago

kyranet commented 6 years ago

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: https://github.com/discordjs/discord.js/blob/b186472785db6bbe7de37e1a75104d79dd0cc44f/src/rest/handlers/RequestHandler.js#L79.

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:

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:

ThunderFrost0001 commented 6 years ago

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

devsnek commented 6 years ago

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 commented 6 years ago

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

iCrawl commented 6 years ago

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

kyranet commented 6 years ago

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.