fix: wrap errors to improve async stack trace#5987
fix: wrap errors to improve async stack trace#5987DigitalBrainJS merged 13 commits intoaxios:v1.xfrom
Conversation
|
thanks for putting this together in a PR @ikonst. I did a bit more testing and was thinking it might actually be better to only adjust the stacktrace in lib/core/dispatchRequest. The current stack traces only seem to be a problem when dispatching the actual request, but there could be other errors in the interceptor chain that we don't want to mess with. Using the test script I shared in the original issue and this PR: const axios = require('axios');
async function makeRequest() {
await axios.get('https://httpstat.us/500');
}
async function main() {
await makeRequest();
}
main().catch((err) => {
console.error(err.stack);
});I get this stack trace: But if I add an interceptor that throws an error, you lose the context of which interceptor failed: const axios = require('axios');
axios.interceptors.request.use(() => {
throw new Error('kaboom');
});
async function makeRequest() {
await axios.get('https://httpstat.us/500');
}
async function main() {
await makeRequest();
}
main().catch((err) => {
console.error(err.stack);
});This was the patch I was testing (for node.js): With this patch I get these stack traces: So for the case where the interceptor throws, it still keeps the callsite at line 4 (inside the interceptor) where the error originates. |
|
Does it help if you inspect the .cause? |
|
P.s. I also had a version where I mutate the stack trace instead of wrapping. I thought wrapping is generally the more idiomatic way. If we do splice the stack, then perhaps we could check that the stack we add to doesn't already contain the stack we're adding. |
|
@DigitalBrainJS could you help with the review? |
1 similar comment
|
@DigitalBrainJS could you help with the review? |
|
We've been running this PR as a patch in production for a few weeks now at work, and it works great. Sentry errors for timeouts report enough of where they are happening to be useful. |
| dummy.stack = new Error().stack; | ||
| } | ||
| // slice off the Error: ... line | ||
| dummy.stack = dummy.stack.replace(/^.+\n/g, ''); |
There was a problem hiding this comment.
g flag has no effect here
There was a problem hiding this comment.
Ah, you're right! I think I was assuming multiline mode by default and thought g negates that. 🤦
| // slice off the Error: ... line | ||
| dummy.stack = dummy.stack.replace(/^.+\n/g, ''); | ||
| // match without the 2 top stack lines | ||
| if (!err.stack.endsWith(dummy.stack.replace(/^.+\n.+\n/g, ''))) { |
There was a problem hiding this comment.
g flag has no effect here
| } | ||
| } | ||
|
|
||
| _dispatchRequest(configOrUrl, config) { |
There was a problem hiding this comment.
It would probably be better to call this _request for now since we are wrapping the original request function
| assert.equal(thrown.message, 'Operation has been canceled.'); | ||
| done(); | ||
| }); | ||
| assert.rejects( |
There was a problem hiding this comment.
uh, looks like a floating promise
There was a problem hiding this comment.
nm, it wasn't since we call .then(done).catch(done) on it, but I added void before it to clarify (per convention here)
e4e102b to
09575e9
Compare
ff00a78 to
77430cf
Compare
|
@DigitalBrainJS could you approve CI again? |
|
@DigitalBrainJS ping |
|
Maybe we should add |
|
@DigitalBrainJS can you please approve the workflows? |
V8 has async stack traces across
awaits, so we're wrappingrequestand intentionally awaiting it to allow any error to "cross", then wrapping the exception therefore getting a stack trace that includes the caller, while at the same keep the original exception as thecause.Fixes #2387.