67

I'm using promises and have code that looks like the following:

function getStuff() { 
  return fetchStuff().then(stuff => 
    process(stuff)
  ).catch(err => {
    console.error(err);
  });
}

Or:

async function getStuff() { 
  try {
    const stuff = await fetchStuff();
    return process(stuff);
  } catch (err) { 
    console.error(err);
  }
}

I was doing this to avoid missing on errors but was told by a fellow user that I shouldn't be doing this and it is frowned upon.

  • What's wrong with return ….catch(err => console.error(err))?
  • I've seen a lot of code that does this, why?
  • What should I do instead?
Bergi
  • 630,263
  • 148
  • 957
  • 1,375
Benjamin Gruenbaum
  • 270,886
  • 87
  • 504
  • 504
  • "`catch`-`void` antipattern" maybe? – Bergi Jun 17 '18 at 12:39
  • hint hint https://github.com/nodejs/promises-debugging/issues/6 ;) – Benjamin Gruenbaum Jun 17 '18 at 13:21
  • If all of these answers are correct, why does the latest LTS version of node.js **still** give the message `[DEP0018] DeprecationWarning: Unhandled promise rejections are deprecated...`? I believe the definition of deprecated is "usable but regarded as obsolete and best avoided." (Although the comments about the catch returning `undefined` are still valid) – David784 Jun 17 '18 at 14:15
  • 1
    @David784 I'm sorry for that! (that's my fault). The wording is unfortunate and I've been meaning to change it since we landed it in 7.7. Madara made the errors a lot nicer in v10 and we're going to fix that very soon - unhandled rejections will terminate the process if they get garbage collected by default in v11 and v12 and the deprecation warning will be gone. – Benjamin Gruenbaum Jun 17 '18 at 14:24
  • 1
    @David784 and you can track current progress in https://github.com/nodejs/promise-use-cases/issues/27 - I've edited my answer a bit to try and reflect that. – Benjamin Gruenbaum Jun 17 '18 at 14:25
  • Isn't `err => console.error(err)` equivalent to just `console.error`? – user76284 Jun 17 '18 at 17:59
  • @user76284 Yes (as long as the function doesn't require to be invoked as a method on the `console` object, which standard `console.error` indeed doesn't), but it's easier to understand – Bergi Jun 17 '18 at 21:41
  • 1
    Undercutting the question a bit, I'd be highly skeptical of someone who tells you not to do something, but then doesn't explain _why_ that is the case. If this fellow user has a valid argument against using it, they should mention it. Or you should ask them for clarification first; since we can't know the exact contect (or their opinion). – Flater Jun 18 '18 at 06:51
  • 1
    @Flater Note I wrote both the question and the answer, I did this since this sort of thing comes up a lot when we analyzed repos and code "in the wild" for the Node.js summit. In fact I did it exactly so that we can point people to "why". – Benjamin Gruenbaum Jun 18 '18 at 06:56

5 Answers5

61

Why does old code do this?

Historically, older (pre 2013) promise libraries 'swallowed' unhandled promise rejections you have not handled yourself. This has not been the case in anything written since then.

What happens today?

Browsers and Node.js already automatically log uncaught promise rejections or have behaviour for handling them and will log them automatically.

Moreover - by adding the .catch you are signalling to the method calling the function that undefined is returned:

// undefined if there was an error
getStuff().then(stuff => console.log(stuff)); 

The question one should be asking oneself when writing async code is usually "what would the synchronous version of the code do?":

function calculate() { 
  try {
    const stuff = generateStuff();
    return process(stuff);
  } catch (err) { 
    console.error(err);
    // now it's clear that this function is 'swallowing' the error.
  }
}

I don't think a consumer would expect this function to return undefined if an error occurs.

So to sum things up - it is frowned upon because it surprises developers in the application flow and browsers log uncaught promise errors anyway today.

What to do instead:

Nothing. That's the beauty of it - if you wrote:

async function getStuff() { 
  const stuff = await fetchStuff();
  return process(stuff);
}
// or without async/await
const getStuff = fetchStuff().then(process);

In the first place you would get better errors all around anyway :)

What to do if I'm running an old version of Node.js?

Old versions of Node.js might not log errors or show a deprecation warning. In those versions you can use console.error (or proper logging instrumentation) globally:

// or throw to stop on errors
process.on('unhandledRejection', e => console.error(e));
Mike Chamberlain
  • 39,692
  • 27
  • 110
  • 158
Benjamin Gruenbaum
  • 270,886
  • 87
  • 504
  • 504
  • 1
    When I log errors like this (assuming there is no problem with the return value being undefined or I'm re-throwing the error), I add information like the file name, line number, etc. or a simple message that gives context. If I simply rely on "unhandledRejection" events this information is not present and many times it means not knowing what operation actually failed (i.e. no context). – Itay Jun 17 '18 at 12:29
  • 2
    @Itay thanks for the comment. Browsers and Node will log the error like an exception - including file number, file name, etc. Note that it is perfectly fine to rethrow (that is, `} catch(e) { attachMetadata(e); throw e; }`) in order to attach metadata rather than `console.error`. Does that address your issue? – Benjamin Gruenbaum Jun 17 '18 at 12:32
  • 1
    @Itay note that this changed in recent versions of browsers and Node.js, and as you know - we're (Node and V8) going to ship async stack traces for production for everyone. – Benjamin Gruenbaum Jun 17 '18 at 12:33
  • ```attachMetadata(e)``` seems like a valid solution, though I can't really know if I'm supposed to modify the thrown object. It seems like a better solution (if supported by the engine) would be as in Java - throwing another exception with the initial exception as the cause. – Itay Jun 17 '18 at 12:36
  • can you attach a link that describes async stack traces spec/proposal? – Itay Jun 17 '18 at 12:46
  • 3
    `process.on('unhandledRejection', ...)`...never even thought to look for the existence of this. Thank you!!!! – David784 Jun 17 '18 at 14:33
  • @David784 if there is any way you think we can make it clearer let me know. I share some of my pain when we were adding it in https://www.youtube.com/watch?v=LGpmUyFnyuQ - We've been thinking of writing a guide for using promises and async functions in Node.js (I originally wrote this for the guide but opted to post it here since there is no consensus on it yet). – Benjamin Gruenbaum Jun 17 '18 at 14:35
  • 1
    @BenjaminGruenbaum Seems like the main hurdle is just learning that it exists. If I hadn't seen your Q/A today, I would've just kept up with the `.catch`es. (As you say in your video, importance of stackoverflow/documentation can't be overemphasized!) Maybe it would also be possible to add a "for more information" link to the console warning that node throws on an unhandled promise rejection? The more signposts pointing in the right direction, the better the chances that more devs will find it – David784 Jun 17 '18 at 15:17
  • Thanks for the suggestion David, I will bring that up for discussion internally with the documentation team :) (Note that any error changes have to wait a major version because people who build tooling around them rely on the error format and we take backwards compatibility very seriously) – Benjamin Gruenbaum Jun 17 '18 at 15:23
  • 1
    Reading this thread, one could be forgiven the conclusion that app code shouldn't contain "catch" at all. Is that correct? In what circumstances is catch positively recommended? – danh Jun 17 '18 at 15:51
  • 1
    @danh suggestions for improvement are very welcome! (thanks for bringing this up). `.catch` is not recommended _unless you are actually handling the error_. That is, if the `.catch` performed actual recovery for the error then it would have been appropriate. That is, you should let error propagate to the first method that can handle them and no further. Let me know if that answers your question and if so I'll edit it into the answer. – Benjamin Gruenbaum Jun 17 '18 at 15:57
  • 1
    Yes @BenjaminGruenbaum, thanks. Maybe to restate (and to check my understanding), lower on the stack, in order to cleanup and put the error into a form most consumable by the caller (e.g. controller code for a service, UI for an app, entry point for an api, etc) – danh Jun 17 '18 at 16:14
  • I feel like `.catch`ing it in the main file, which bootstraps everything else, is not that bad (and acceptable until [Node.js really terminates the process](https://stackoverflow.com/q/40500490/603003)) and akin to try-catching in the main loop of C++ or Java code. What is your take on this? – ComFreek Jun 17 '18 at 18:02
  • @ComFreek if you're handling everything then that's fine – Benjamin Gruenbaum Jun 17 '18 at 22:43
  • @BenjaminGruenbaum - once we have async stack traces, we won't need to attach metadata, correct? Right now rejections are useless without it. Following the 'no catch' rule, we end up with general errors (e.g. "cannot find property .length of undefined") without real context (e.g. in a library that handles XML parsing), so we don't even know where to start looking – Ran Halprin Jun 21 '18 at 17:30
  • @RanHalprin note we already have async stack traces with the inspector attached, the V8 team have agreed to work on this for production and Benedikt said it's a priority. Also, you always get the last frame - so it's "cannot read property `.length` of `foo` with a stack trace (not an async one though). We improved the warnings for Node 10 and Node 11 will improve the situation. You can track this in either https://github.com/nodejs/promise-use-cases or https://github.com/nodejs/promises-debugging – Benjamin Gruenbaum Jun 21 '18 at 17:42
14

What's wrong with return ….catch(err => console.error(err))?

It returns a promise that will fulfill with undefined after you handled the error.

On it's own, catching errors and logging them is fine at the end of a promise chain:

function main() {
    const element = document.getElementById("output");
    getStuff().then(result => {
        element.textContent = result;
    }, error => {
        element.textContent = "Sorry";
        element.classList.add("error");
        console.error(error);
    });
    element.textContent = "Fetching…";
}

However if getStuff() does catch the error itself to log it and do nothing else to handle it like providing a sensible fallback result, it leads to undefined showing up in the page instead of "Sorry".

I've seen a lot of code that does this, why?

Historically, people were afraid of promise errors being handled nowhere which lead to them disappearing altogether - being "swallowed" by the promise. So they added .catch(console.error) in every function to make sure that they'd notice errors in the console.

This is no longer necessary as all modern promise implementation can detect unhandled promises rejections and will fire warnings on the console.

Of course it's still necessary (or at least good practice, even if you don't expect anything to fail) to catch errors at the end of the promise chain (when you don't further return a promise).

What should I do instead?

In functions that return a promise to their caller, don't log errors and swallow them by doing that. Just return the promise so that the caller can catch the rejection and handle the error appropriately (by logging or anything).

This also simplifies code a great lot:

function getStuff() { 
  return fetchStuff().then(stuff => process(stuff));
}

async function getStuff() { 
  const stuff = await fetchStuff();
  return process(stuff);
}

If you insist on doing something with the rejection reason (logging, amending info), make sure to re-throw an error:

function getStuff() { 
  return fetchStuff().then(stuff =>
    process(stuff)
  ).catch(error => {
    stuffDetails.log(error);
    throw new Error("something happened, see detail log");
  });
}

async function getStuff() {
  try {
    const stuff = await fetchStuff();
    return process(stuff);
  } catch(error) {
    stuffDetails.log(error);
    throw new Error("something happened, see detail log");
  }
}

Same if you are handling some of the errors:

function getStuff() { 
  return fetchStuff().then(stuff =>
    process(stuff)
  ).catch(error => {
    if (expected(error))
      return defaultStuff;
    else
      throw error;
  });
}

async function getStuff() {
  try {
    const stuff = await fetchStuff();
    return process(stuff);
  } catch(error) {
    if (expected(error))
      return defaultStuff;
    else
      throw error;
  }
}
Bergi
  • 630,263
  • 148
  • 957
  • 1,375
  • Is possible to use catch to restore promise from error state and i.e. return promise fulfilled by fallback value? – bigless Jun 17 '18 at 13:14
  • @Bergi it is a good practice to catch your errors but we are never really sure when the promise chain is going to end. In the future, one can always extend on the current chain. Catching errors instead of just Promise rejections always seems like an over-kill to me. It has always led to weird flow in the application. – yeshashah Jun 17 '18 at 13:23
  • @bigless yes, just do `promise.catch(error => { if (expected(error)) return fallback; else throw error; })` – Bergi Jun 17 '18 at 13:37
  • 1
    @yeshashah Not sure what you mean by "one can always extend on the current chain". Have a look at the `main` example in my answer: that's definitely the end of the promise chain, `main` doesn't return anything. – Bergi Jun 17 '18 at 13:38
  • @Bergi IMO, your comment here should be part of your answer, as the pattern is common and not mentioning it can imply that you shouldn't be catching the errors you're expecting/can handle/can recover from, while generating a reject via `throw` for those errors you don't *actually* handle. – Makyen Jun 17 '18 at 15:51
4

The reason you should not catch errors unless absolutely required (which is never) is that

Apart from swallowing promise rejections, catch handler also swallows any JS errors that occurs in any successive code run by the corresponding success handler.

Implications

  1. Once an error is caught by a catch handler, it is considered as done and handled. All the successive promise subscribers in the promise chain will call their success handlers instead of failure or catch handlers. This leads to weird behaviours. This is never the intended code flow.

  2. If a function at lower level like a service method (getStuff) handles errors in catch, it breaks the principle of Separation of Concerns. A responsibility of service handler should be solely to fetch data. When that data call fails, the application who is calling that service handler should manage the error.

  3. The catching of error in some function being caught by another one, results in weird behaviours all around and makes it really hard to track root causes of bugs. To track such bugs we have to enable the Break on Caught Exceptions in the Chrome dev console which will break on every catch and could take hours at an end to debug.

It is always a good practice to handle promise rejections but We should always do that using failure handler over catch handler. A failure handler will only catch Promise rejections and lets the application break, if any JS error occurs, which is how it should be.

yeshashah
  • 1,536
  • 11
  • 19
2

error is much too generic, it is a catch all but there are only so many things that the operation would fail with, error is everything errorSomethingSpecific gives granularity

mmx
  • 35
  • 2
0

The most general statement here, that applies in languages beyond javascript, is don't 'catch' an error unless you plan to 'handle' the error. Logging is not handling.

i.e. In general, the best (only?) reason for a catch is to handle/'deal with' the error in a constructive way that allows the code to carry on without any further problems. And again, a line of logging probably never achieves that...

spechter
  • 2,058
  • 1
  • 17
  • 23