16 Comments

Never Mix Promises and Callbacks in NodeJS

When using Node.js, you should never mix promises and callbacks. It’s a recipe for confusion. The problem is pretty obvious in hindsight, but it sure threw me for a loop the first time I ran into it.

I’ve created an example Mocha test case that demonstrates the problem. In it, I’m using Knex to fetch a user from a database. Knex queries return Bluebird promises by default, which I use to complete a callback that I’ve passed in. Basically, this is what you want to avoid:


// a somewhat silly minimal example:
function fetchFirstUser(callback) {
  knex.first().from("users").then((user) => {
    callback(user);
  }).catch((e) => {
    callback(null);
  });  
}

it("should pass us a null if there are no users", function(done) {
  fetchFirstUser(function(user) {
    expect(user).to.be.null;
    done();
  })
});

Looks innocuous enough, right? When this happened to me, so many things went wrong at once, and then erased their tracks, that it took me a while to figure out what broke. Here’s an abridged version of my learning experience :)

When I run the above test, I get:


  fetchFirstUser
    ✓ should pass us a null if there are no users


  1 passing (142ms)

Seems OK, right?
Now let’s make sure our test fails. We’ll change the expectation to:

expect(user).to.not.be.null;

Now, when I run this test, I get:


  fetchFirstUser
    ✓ should pass us a null if there are no users


  1 passing (142ms)

Oh No…

Oh. This can’t be good.
Let’s change the expectation to something nonsensical:

expect(user).to.be.equal(5);

Now, we get:


  fetchFirstUser
Unhandled rejection AssertionError: expected null to equal 5
    at test/testsomething.spec.js:45:26
    at db.first.from.then.catch (test/testsomething.spec.js:21:5)
    at tryCatcher (node_modules/bluebird/js/release/util.js:16:23)
    at Promise._settlePromiseFromHandler (node_modules/bluebird/js/release/promise.js:510:31)
    at Promise._settlePromise (node_modules/bluebird/js/release/promise.js:567:18)
    at Promise._settlePromise0 (node_modules/bluebird/js/release/promise.js:612:10)
    at Promise._settlePromises (node_modules/bluebird/js/release/promise.js:687:18)
    at Async._drainQueue (node_modules/bluebird/js/release/async.js:133:16)
    at Async._drainQueues (node_modules/bluebird/js/release/async.js:143:10)
    at Immediate.Async.drainQueues (node_modules/bluebird/js/release/async.js:17:14)
    at runCallback (timers.js:651:20)
    at tryOnImmediate (timers.js:624:5)
    at processImmediate [as _immediateCallback] (timers.js:596:5)

    1) should pass us a null if there are no users


  0 passing (2s)
  1 failing

  1) fetchFirstUser should pass us a null if there are no users:
     Error: Timeout of 2000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves.

Promises Eat Exceptions

OK, now we’re getting somewhere. There are two weird things going on:
1) It’s saying user is null, so why didn’t not.be.null fail?
2) Why did it time out? The test should have failed immediately when the exception occurred.

#2 is the critical hint. Something must be catching that exception–d’oh!

Let’s try the be.null case again, but this time, we’ll add a couple of extra console.logs.

One above the expect:


console.log("XXXX - In callback");
expect(user).to.be.null;

and one in the catch:


.catch((e) => {
    console.log(e);
    callback(null);
  });

Now, we get:


  fetchFirstUser
XXXX - In callback
{ AssertionError: expected undefined to be null
    at test/testsomething.spec.js:45:25
    at db.first.from.then (test/testsomething.spec.js:17:5)
    at tryCatcher (node_modules/bluebird/js/release/util.js:16:23)
    at Promise._settlePromiseFromHandler (node_modules/bluebird/js/release/promise.js:510:31)
    at Promise._settlePromise (node_modules/bluebird/js/release/promise.js:567:18)
    at Promise._settlePromise0 (node_modules/bluebird/js/release/promise.js:612:10)
    at Promise._settlePromises (node_modules/bluebird/js/release/promise.js:691:18)
    at Async._drainQueue (node_modules/bluebird/js/release/async.js:133:16)
    at Async._drainQueues (node_modules/bluebird/js/release/async.js:143:10)
    at Immediate.Async.drainQueues (node_modules/bluebird/js/release/async.js:17:14)
    at runCallback (timers.js:651:20)
    at tryOnImmediate (timers.js:624:5)
    at processImmediate [as _immediateCallback] (timers.js:596:5)
  message: 'expected undefined to be null',
  showDiff: false,
  actual: undefined,
  expected: undefined }
XXXX - In callback
    ✓ should pass us a null if there are no users


  1 passing (124ms)

Callback Gets Called Twice

We can see what’s happening: Our callback is getting called twice!

It’s first getting called in the normal expected path as callback(user), but rather than passing in null as I was expecting, db.first().from("users"), is giving me an undefined. This is causing our expect(user).to.be.null to fail and raise an exception.

But! That exception is caught in my .catch() block, which then calls the callback again, but this time with null, which causes the expect(user).to.be.null to pass. Then we call done(), and the the test passes.

The moral of the story is, don’t call callbacks from inside promises. They will swallow your exceptions unexpectedly.

The solution in my case was to use the asCallback method to escape from the promise land:


  db.first().from("users").asCallback((error, user) => {
    if (error) {
      callback(null)
    } else {
      callback(user || null);
    }
  });

Make Sure Your Tests Fail First

This also a nice example of why you should always make sure your tests fail as expected. This latest bug is one of many that I’ve discovered by trying to make a test fail, and finding that it unexpectedly passes. If I hadn’t made sure my test failed, I would not have noticed this until much much later.

Also, watch out for Sinon’s useFakeTimers(). The asCallback method uses timers to escape the promises, and useFakeTimers() makes timer code synchronous, which causes all of our exceptions to get swallowed again!