Node.js error-handling gotcha: synchronous errors from an asynchronous function

One thing to look out for in Node.js: functions that may throw an error either synchronously or asynchronously (which, technically, is any function that does async work). You may think you've got pretty good error-handling, but they complicate things.

For instance, here's a basic example:

function getAdditionalData(input) {
  const url = getRequestUrl(input);
  
  return makeHttprequest(url).then(response => {
    return response.data;
  });
}

app.get('/data', (req, res) => {
  const input = calculateInput();
  return getAdditionalData(input).then(result => {
    return { input, result };
  });
});

This is a simple endpoint that calls getAdditionalData() to get some data and return to the user. The function does an asynchronous thing (HTTP request), so it returns a promise.

Let's think about error-handling. If something goes wrong (maybe the APi is unavailable), we don't want our whole app to crash. Instead, we'd rather return the incomplete data to the user. The obvious way to achieve this is to add a .catch():

app.get('/data', (req, res) => {
  const input = calculateInput();
  return getAdditionalData(input).then(result => {
    return { input, result };
  }).catch(e => {
    // Report error somehow
    console.log("An error occurred", e);
    // But return the original instead of crashing
    return { input };
  });
});

Most folks would stop here. But the hidden problem is that getAdditionalData() may throw a synchronous error, and that will not be caught by promise.catch(). For example, if the function getRequestUrl() does not exist, then a ReferenceError will be thrown. And it will not be passed to our .catch. This is because Promise rejections (what .catch gets) and exceptions are two independent error-handling mechanisms in Node.js. This exception will not be handled, and will either cause the process to crash, or be caught by your global process.on('uncaughtException') handler.

How do we fix this? There are two ways. Option one: switch to async/await, which converts Promise rejections to thrown exceptions, so you can handle both with try/catch.

app.get('/data', async (req, res) => {
  const input = calculateInput();
  try {
    const result = await getAdditionalData(input);
    return { input, result };
  } catch(e) {
    console.log("An error occurred", e);
    return { input };
  }
});

However, try/catch can disrupt the flow of your code sometimes, and you might want to stick with raw promises. In that case, you can use the other option: make sure getAdditionalData() only rejects, instead of throws. You can do this by marking it as async (even if you aren't using await inside it):

-function getAdditionalData(input) {
+async function getAdditionalData(input) {
+ // Now, thrown errors will be converted to rejections
  // ...
}

app.get('/data', (req, res) => {
  const input = calculateInput();
  return getAdditionalData(input).then(result => {
    return { input, result };
  }).catch(e => {
    console.log("An error occurred", e);
    return { input };
  });
});

Of course, you can only do this if you can modify the function's source code. Otherwise, you'd have to do something like this:

function getAdditionalData(input) {
  // ...
}

app.get('/data', (req, res) => {
  const input = calculateInput();
  return new Promise((resolve, reject) => {
    try {
      getAdditionalData(input).then(resolve);
    } catch (e) {
      reject(e);
    }
  }).catch(e => {
    console.log("An error occurred", e);
    return { input };
  });
});

...in which case, you're back to try/catch, so you might as well use the first method.

I guess the moral of this story is: if your function returns a promise, it might be a good idea to mark it as async, so its errors are always turned into rejections.

Error handling in Node.js can be a real minefield, what with synchronous code, promises, callbacks, and event emitters. Unexpected scenarios can arise when they interact, like when an error is thrown inside a timer, or an error is thrown in an error event handler. James Snell has an example of this, and his "Broken Promises" talk goes into some depth on some gotchas around working with promises.

UPDATE

Since publishing this article, ,I came across this W3C guide, which actually states it unambiguously:

In particular, promise-returning functions should never synchronously throw errors, since that would force duplicate error-handling logic on the consumer: once in a catch (e) { ... } block, and once in a p.catch(e => { ... }) block. Even argument validation errors are not OK. Instead, all errors should be signaled by returning rejected promises.

So, yes: if your function returns a promise, synchronous errors are an anti-pattern.

When you think about it, it makes a lot of sense. If you were using a callback-type async function, you'd expect errors to be passed to the callback. You wouldn't expect to do this:

try {
  fs.readFile(file, (err, contents) => {
    if (err != null) {
      // Handle error
    }
    // Do something with the contents
  });
} catch (e) {
  // Also handle error, maybe ???
}

No, you wouldn't. Callback-type async functions typically pass all their errors to the callback, and so should promise-type ones.



I write about my software engineering learnings and experiments. Stay updated with Tentacle: tntcl.app/blog.shalvah.me.

Powered By Swish