fix: wrap errors to improve async stack trace

[ikonst]

V8 has async stack traces across awaits, so we're wrapping request and 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 the cause.

Fixes #2387.

[andrew0]

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:

AxiosError: Request failed with status code 500
    at AxiosError.from (node_modules/axios/dist/node/axios.cjs:837:14)
    at Axios.request (node_modules/axios/dist/node/axios.cjs:3820:61)
    at process.processTicksAndRejections (node:internal/process/task_queues:95:5)
    at async makeRequest (test.js:4:3)
    at async main (test.js:8:3)

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);
});

Error: kaboom
    at AxiosError.from (node_modules/@fds/request/node_modules/axios/dist/node/axios.cjs:837:14)
    at Axios.request (node_modules/axios/dist/node/axios.cjs:3820:61)
    at async makeRequest (test.js:8:3)
    at async main (test.js:12:3)

This was the patch I was testing (for node.js):

diff --git a/dist/node/axios.cjs b/dist/node/axios.cjs
index 8d8b06a66eb5b72fb5d4cbe2bdbd085a1c490aa0..10cd79013e612870f24c08d56765fa154eeab2ab 100644
--- a/dist/node/axios.cjs
+++ b/dist/node/axios.cjs
@@ -3570,6 +3570,17 @@ function dispatchRequest(config) {
 
     return response;
   }, function onAdapterRejection(reason) {
+    if (reason instanceof Error) {
+      const carrier = {};
+      if (Error.captureStackTrace) {
+        Error.captureStackTrace(carrier);
+      } else {
+        carrier.stack = new Error().stack;
+      }
+
+      reason.stack = reason.stack + carrier.stack.replace(/\S.*/, '');
+    }
+
     if (!isCancel(reason)) {
       throwIfCancellationRequested(config);
 

With this patch I get these stack traces:

AxiosError: Request failed with status code 500
    at settle (node_modules/axios/dist/node/axios.cjs:1913:12)
    at IncomingMessage.handleStreamEnd (node_modules/axios/dist/node/axios.cjs:2995:11)
    at IncomingMessage.emit (node:events:525:35)
    at endReadableNT (node:internal/streams/readable:1359:12)
    at process.processTicksAndRejections (node:internal/process/task_queues:82:21)
    at onAdapterRejection (node_modules/axios/dist/node/axios.cjs:3576:15)
    at process.processTicksAndRejections (node:internal/process/task_queues:95:5)
    at async makeRequest (test.js:4:3)
    at async main (test.js:8:3)

Error: kaboom
    at test.js:4:9
    at async makeRequest (test.js:8:3)
    at async main (test.js:12:3)

So for the case where the interceptor throws, it still keeps the callsite at line 4 (inside the interceptor) where the error originates.

[ikonst]

Does it help if you inspect the .cause?

[ikonst]

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.

[ikonst]

@DigitalBrainJS could you help with the review?

[ikonst]

@DigitalBrainJS could you help with the review?

[alvarlagerlof]

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.

[DigitalBrainJS]

g flag has no effect here

[ikonst]

Ah, you're right! I think I was assuming multiline mode by default and thought g negates that. 🤦

[DigitalBrainJS]

g flag has no effect here

[DigitalBrainJS]

It would probably be better to call this _request for now since we are wrapping the original request function

[ikonst]

uh, looks like a floating promise

[ikonst]

nm, it wasn't since we call .then(done).catch(done) on it, but I added void before it to clarify (per convention here)

[ikonst]

@DigitalBrainJS could you approve CI again?

[ikonst]

@DigitalBrainJS ping

[ikonst]

Maybe we should add --async-stack-traces to the Node options of the 12.x job? In Node 12, it still wasn't the default (https://thecodebarbarian.com/async-stack-traces-in-node-js-12.html).

[ikonst]

@DigitalBrainJS can you please approve the workflows?