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!
You could just return the promise in `fetchFirstUser`. Test suites should let you return a promise, so there’s no need for callbacks at all in any of this example.
Correct! The main thing is to not mix the two. Either use just promises, or just use callbacks, but you have to be really careful when you want to go between the two.
My example is contrived, but in the larger project that I was working in, almost all of the existing code used callbacks. Refactoring all that code to use promises, just to accommodate the two or three places that actually needed them, would have been silly.
I noticed a problem with your example:
it(“should pass us a null if there are no users”, done => {
fetchFirstUser(user => {
expect(user).to.not.be.null
done()
})
});
Since your function has two arguments, shouldn’t you pass the callback to the second parameter, not `iswhatIwant`?
it(“should pass us a null if there are no users”, done => {
fetchFirstUser(‘wut’, user => {
expect(user).to.not.be.null
done()
})
});
When I run the test locally this way, Mocha actually does actually fail and say that user should not have a null value. You mixed callbacks and Promises just fine.
Ugh, the “iswhatIwant” was a leftover from an earlier version of my example, and when I pasted the final version of my examples into my post, I missed that function :(
Sorry for the confusion. I updated that code snippet to match the code that I actually ran.
haha, no problem man, thanks!
I don’t degrade promises into callbacks. Some of my modules use callbacks and I convert them as soon as possible to promises. I haven’t experienced any issues at all. The point being, it’s okay to mix callback and promises, just don’t degrade promises into basic callbacks. Once a promise, stay a promise.
Either way, even with that minor correction, your example doesn’t do what you claim. expect(user).to.not.be.null does actually throw an unhandled Promise rejection with current Node, Mocha, Knex and SQLite.
It’s still not a good practise to use callbacks, mainly due to the maintainability and legibility concerns, but if an error arises when mixing callbacks and Promises, this article and example do not prove that.
Using asCallback is great advice as it gives you one less thing to think about. If you’re not using bluebird, though, such as when you’re using native Promises, I want to make you don’t forget about the possibility of passing a rejection handler as a second argument to #then, like so:
“`
function fetchFirstUser(callback) {
knex.first().from(“users”).then(
(user) => {
callback(user);
},
(err) => {
callback(null);
}
);
}
“`
This way, your rejection/error handler will catch errors from Knex, but not from the callback call in your fulfillment handler.
I agree with your overall premise of not mixing the two as it leads to confusion. However, I would argue that you’re not using callbacks the right way.
The standard callback signature is `callback(err, result)`, as seen in knex.asCallback. So if you write your test like so – you avoid that ambiguity:
function fetchFirstUser(callback) {
knex.first().from(“users”).then(user => callback(null, user)).catch(callback);
}
it(“should pass us a null if there are no users”, function(done) {
fetchFirstUser(function(err, user) {
expect(err).to.be.null;
expect(user).to.be.null;
done();
})
});
fetchFirstUser can then be further simplified to
function fetchFirstUser(callback) {
db.first().from(“users”).asCallback(callback)
}
That’s code really poor – if you’d be wrapping a callback inside another callback – how would you handle that? You’d pass the error as a first parameter, right? Why aren’t you doing it here?
function fetchFirstUser(callback) {
knex.first().from(“users”).then((user) => {
callback(null, user);
}).catch((e) => {
callback(e);
});
}
Simply wrap calling your callbacks in calls to process.nextTick(), and everything is fine. I have written about this a few weeks ago (in German): https://www.heise.de/developer/artikel/Callbacks-Promises-Es-ist-kompliziert-3665393.html
The hint to never mix both is theoretically right, but practically not doable. You will sooner or later have situations where you have to mix both worlds.
Thanks for taking the time to write this , Job. I learned something from it.