How to reject (and properly use) Promises?

Question

Short story:

• Talking about Promises/A+, what is the proper way to reject a promise - throwing an error? But if I miss the catch - my whole app will blow!

• How to use promisify and what are the benefits of it (maybe you'll need to read the longer version)?

• Is .then(success, fail) really an anti-pattern and should I always use .then(success).catch(error)?

Longer version, described as real life problem (would love someone to read):

I have a library that uses Bluebird (A+ Promise implementation library), to fetch data from database (talking about Sequelize). Every query returns a result, but sometimes it's empty (tried to select something, but there wasn't any). The promise goes into the result function, because there is no reason for an error (not having results is not an error). Example:

Entity.find(1).then(function(result) {
  // always ending here, despite result is {}
}).catch(function(error) {
  // never ends here
});

I want to wrap this and check if the result is empty (can't check this everywhere in my code). I did this:

function findFirst() {
  return Entity.find(1).then(function(result) {
    if (result) { // add proper checks if needed
      return result; // will invoke the success function
    } else {
      // I WANT TO INVOKE THE ERROR, HOW?! :)
    }
  }).catch(function(error) {
    // never ends here
  });
}

findFirst().then(function(result) {
  // I HAVE a result
}).catch(function(error) {
  // ERROR function - there's either sql error OR there is no result!
});

If you are still with me - I hope you will understand what's up. I want somehow to fire up the error function (where "ERROR function" is). The thing is - I don't know how. I've tried these things:

this.reject('reason'); // doesn't work, this is not a promise, Sequelize related
return new Error('reason'); // calls success function, with error argument
return false; // calls success function with false argument
throw new Error('reason'); // works, but if .catch is missing => BLOW!

As you can see by my comments (and per spec), throwing an error works well. But, there's a big but - if I miss the .catch statement, my whole app blows up.

Why I don't want this? Let's say I want to increment a counter in my database. I don't care about the result - I just make HTTP request.. So I can call incrementInDB(), which has the ability to return results (even for test reasons), so there is throw new Error if it failed. But since I don't care for response, sometimes I won't add .catch statement, right? But now - if I don't (on purpose or by fault) - I end up with your node app down.

I don't find this very nice. Is there any better way to work it out, or I just have to deal with it?

A friend of mine helped me out and I used a new promise to fix things up, like this:

function findFirst() {
  var deferred = new Promise.pending(); // doesnt' matter if it's Bluebird or Q, just defer
  Entity.find(1).then(function(result) {
    if (result) { // add proper checks if needed
      deferred.resolve(result);
    } else {
      deferred.reject('no result');
    }
  }).catch(function(error) {
    deferred.reject('mysql error');
  );

  return deferred.promise; // return a promise, no matter of framework
}

Works like a charm! But I got into this: Promise Anti Patterns - wiki article written by Petka Antonov, creator of Bluebird (A+ implementation). It's explicitly said that this is wrong.

So my second question is - is it so? If yes - why? And what's the best way?

Thanks a lot for reading this, I hope someone will spent time to answer it for me :) I should add that I didn't want to depend too much on frameworks, so Sequelize and Bluebird are just things that I ended up working with. My problem is with Promises as a global, not with this particular frameworks.

Solution 1

Please ask only a single question per post :-)

Is .then(success, fail) really an anti-pattern and should I always use .then(success).catch(error)?

No. They just do different things, but once you know that you can choose the appropriate one.

How to use promisify and what are the benefits of it?

I think the Bluebird Promisification docs explain this pretty well - it's used to convert a callback api to one that returns promises.

Talking about Promises/A+, what is the proper way to reject a promise - throwing an error?

Yes, throwing an error is totally fine. Inside a then callback, you can throw and it will be caught automatically, resulting to the rejection of the result promise.

You can also use return Promise.reject(new Error(…));; both will have absolutely the same effect.

A friend of mine helped me out and I used a new promise to fix things up, like this: […]

No. You really shouldn't use that. Just use then and throw or return a rejected promise in there.

But if I miss the catch statement - my whole app will blow!

No, it won't. Notice that .catch() method is not a try catch statement - the error will already be caught where your then callback was invoked. It is then only passed to the catch callback as an argument, the .catch() call is not required to capture exceptions.

And when you miss .catch(), your app won't blow. All what happens is that you have a rejected promise laying around, the rest of your app won't be affected by this. You will get an unhandled rejection event, which can be dealt with globally.

Of course, that should not happen; every promise chain (not every promise instance) should be ended with a .catch() that handles errors appropriately. But you definitely don't need .catch() in every helper function, when it returns a promise to somewhere else then errors will usually be handled by the caller.

Solution 2

Talking about Promises/A+, what is the proper way to reject a promise - throwing an error? But if I miss the catch - my whole app will blow!

And if you use return codes to signal errors instead of exceptions, missing the check will cause subtle bugs and erratic behavior. Forgetting to handle errors is a bug, but having your app blow up in such an unfortunate case is more often better than letting it continue in corrupted state.

Debugging forgotten error code check that is only apparent by the application behaving weirdly at some unrelated-to-the-source-of-error place is easily many orders of magnitude more expensive than debugging a forgotten catch because you have the error message, source of error and stack trace. So if you want productivity in typical application development scenario, exceptions win by a pretty huge margin.

.then(success, fail)

This is usually signal that you are using promises as glorified callbacks where the callbacks are simply passed separately. This is obviously an anti-pattern as you will not get any benefits from using promises in this way. If that's not the case, even then you are just making your code slightly less readable compared to using .catch(), similar to how method(true, true, false) is much less readable than method({option1: true, option2: true, option3: false}).

How to use promisify and what are the benefits of it (maybe you'll need to read the longer version)?

Most modules only expose a callback interface, promisify turns a callback interface automatically into a promise interface. If you are writing such code manually (using deferred or new Promise), you are wasting your time.

So my second question is - is it so? If yes - why? And what's the best way?

The best way is to chain the promise:

function findFirst() {
  return Entity.find(1).tap(function(result) {
    if (!result) throw new Error("no result");
  });
}

It's better because it's simpler, shorter, more readable and far less error-prone way to do exactly the same thing.

Comments

[t.niese]

To the But, there's a big but - if I miss the .catch statement, my whole app blows up. isn't that what exceptions are about? You should always handle exception, even if you don't care about the failure you should make that clear in the code by adding a .catch with a comment that you don't care about it. Imagine you will later look into your code again and recognize that there is an unhanded exception but you don't remember why it is unhandled, then you will spend much time to determine the reason.

[JHH]

I think t.niese's comment should be an answer. Yes, it may be easier for any dev to write one line of code than to write proper code, but that is no excuse. Way too often I see code from inexperienced developers where exceptions thrown from some API are silently swallowed and converted to returning null etc. Some developers seem to have some kind of fundamental fear of exceptions. With proper use of catch() and done() I don't really see any issue. Besides, you could always write your own uncaught exception handler if needed?

[jfriend00]

I don't see how rejecting a promise or resolving with a value that communicates everything didn't succeed rather than throwing an exception is an anti-pattern. Antonov seems to be just arguing that one should use .catch() instead of the fail handler in .then(). That seems purely like a style issue to me and he's just expressing his opinion. He doesn't offer any convincing justification for that opinion other than a rough analogy to use try/catch for synchronous code. But, there are hundreds of other ways errors are returned in synchronous code too.

[Andrey Popov]

Thanks for the short reply. I just have to notice the article you've mentioned (error codes vs exceptions) - if you read it closely, you will see that exceptions have a lot of drawbacks and that they "should be embraced by all lazy programmers writing low-quality code like myself". Maybe it's just a matter of opinion. Thanks for the reply though!

[Esailija]

@AndreyPopov I think you should get down the basics unless you work at NASA in which case you should use error codes