9

I've inherited a codebase at work that contains a dozen or so examples of the following pattern:

var promise = null;
try {
    promise = backendService.getResults(input);
}
catch (exception) {
    console.err(exception);
}
if (promise !== null) {
    promise.then(function (response) {
        // do stuff
    })
    .catch(function (error) {
        console.err(error);
    });
}

Where backendService is an Angular service that in turn calls a REST service through $http.

So here's my question: is that try/catch really necessary? Will there ever be any scenario where a particular error/exception is thrown that the promise's .catch fails to catch?

This has been the subject of a bit of debate on the team all morning, and the only resolution we've come up with is that we don't think it's necessary, but (a) changing it breaks the tests that were written alongside it (which would also need to be changed), and (b) well... it's defensive coding, right? It's not a Bad Thing.

The merits of actually bothering to refactor it into oblivion when there are more important things to do aren't what I'm asking about, though. I just want to know if it's a reasonable pattern when promises are being passed around like this (in AngularJS specifically, if that makes a difference), or just paranoia.

daemone
  • 1,183
  • 1
  • 17
  • 28
  • its not needed. you can have only your `then()` and `catch()` – Felipe Skinner Jul 28 '15 at 14:11
  • seems to me that it's only partially useful. If you have an exception in the code that triggers the XHR (presumably) to get the results, this code would catch, but typically that code is much more well known and not as subject to runtime exceptions. The tricky part is the response from the server- you are much more likely to have an exception there due to the service returning something you didnt expect. This would be handled by the promise rejection. I don't think the try/catch is necessary if you are handling `input` safely (normally) in your service. – pherris Jul 28 '15 at 14:24
  • 1
    It would be relatively easy to test. Start by introducing syntax errors in the method of various kinds, both before the $http call and inside it's callback, and then finish up with testing http errors such as 404 500 and CORS error. My assumption would be that .catch catches everything after http request is started and the promise is returned, but not before. – Kevin B Jul 28 '15 at 14:25

2 Answers2

4

Do promises in AngularJS catch every exception/error?

No. Only exceptions that are thrown from inside then/catch callbacks are automatically caught. All errors happening outside of them will need to be handled explicitly.

Will there ever be any scenario where a particular error/exception is thrown that the promise's .catch fails to catch?

Yes. That backendService.getResults(input) call might not return a promise, but it can throw an exception. Or it doesn't even get that far when backendService is null, and you'll get a ReferenceError or .getResults is not a function and you'll get a TypeError.

is that try/catch really necessary?

Not really. In the latter case, that your code has a grave mistake, you probably don't care to throw and crash. The former case, that backendService.getResults(input) throws, is heavily despised. Asynchronous functions should never throw but only return promises - exactly for the reason that you don't have to write two error handling statements then.

well... it's defensive coding, right? It's not a Bad Thing.

Yeah, pretty much. But it is questionable here. A synchronous exceptions is really unexpected here, not just a service whose failure can be handled. The logging message in the catch block should signify that.

Notice that it also isn't defensive enough. It doesn't catch the probably more likely mistake where getResults() does return, but something that is not a promise. Calling .then() on that might throw. Similarly, the if (promise !== null) is dubious, as it hides when null is returned erroneously (we really could need try-catch-else here).

Community
  • 1
  • 1
Bergi
  • 630,263
  • 148
  • 957
  • 1,375
  • To your last paragraph: Sorry, that was my mistake. I've edited the code block in my question to clarify. The declaration of `promise` before the try is actually initialising it to `null`. – daemone Jul 28 '15 at 14:57
  • To the rest. Thanks, this is pretty much what I figured. `backendService` is as much under my control now as the code that calls it, and I can tell you that it absolutely returns a promise, and would only ever `throw` if something catastrophic and vanishingly unlikely happened. Like entire files being left out of the build. It *does not* contain the word `throw`, and if it did I would have removed it for exactly the reason you describe. I like the phrase "heavily despised", by the way. It fits quite nicely. – daemone Jul 28 '15 at 15:15
1

is that try/catch really necessary?

Not really. As long as your backendService.getResults() returns a promise. Like return $http(...)

Will there ever be any scenario where a particular error/exception is thrown that the promise's .catch fails to catch?

I don't think so, since any error will be a rejected promise, it will fall into your .catch() handler

well... it's defensive coding, right? It's not a Bad Thing.

It depends... Javascript try/catch is has some performance issues. So if you're using just for the sake of making sure, you could remove it :)

Take a further look at try-catch discussion here if you wish: Javascript Try-Catch Performance Vs. Error Checking Code

Community
  • 1
  • 1
Felipe Skinner
  • 16,246
  • 2
  • 25
  • 30