Pitfalls of Promisifying by hand

ByPanu Pitkamaki

When wrapping a traditional style callback naively into a Promise, you might end up with code that looks like this

    var deferred = Q.defer();
    fs.readFile(file, "UTF-8", function(error, text) {
        if (error) {
            deferred.reject(new Error(error));
        } else {
            deferred.resolve(text);
        }
    });
    return deferred.promise;

calling this deferredRead(), you would then use it

deferredRead("words.txt").then((words) => {
    // do something with the data
}).fail((e) => {
    // error handling
});

There exists 2 very serious shortcomings in this approach. Can you spot them?

Initiating function throws exception

If the initiating function throws exception, it will not propagate to the fail()-handler. That is, before the deferred is properly set up if fs.readFile() throws an error, the program will crash due to uncaught exception.

A better way would be to wrap the initiating function inside a try-catch block.

Result callback is called multiple times

The Promises/A+ spec states that a promise can only be fulfilled or rejected once.

promise.then(onFulfilled, onRejected)

A promise must provide a then method to access its current or eventual value or reason.

If onFulfilled is a function, it must not be called more than once.
(Promises/A+ Section 2.2.2)

The wrapping function does not take this into account. It allows the callback to be called infinite number of times. A better way would be to wrap the callback function around a gatekeeper logic that allows it to be run only once and that would throw error otherwise.

On the other hand, ES6 spec states that if a promise is already resolved then further resolutions are just ignored. This would argue that in this case error should not be thrown. Implementations on this vary among promise libraries: kew does throw error but major ones like Bluebird, Q or native Promises do not.

The perfect Promises/A+ version

    var deferred = Q.defer();
    var resolved = false;
    try {
        fs.readFile(file, "UTF-8", function(error, text) {
            if (resolved) {
                throw new Error("Already resolved.");
            }
            resolved = true;
            if (error) {
                deferred.reject(new Error(error));
            } else {
                deferred.resolve(text);
            }
        });
    } catch (e) {
        // needed to handle special case when callback is called
        // synchronously, normally it is asynchronous
        if (e.message === "Already resolved.") {
            throw e;
        }
        deferred.reject(e);
    }
    return deferred.promise;

This is here for the sake of reference. However, you shouldn't try to write this beast every time you need a callback wrapped inside a promise. It's pretty error prone and you easily miss something. Better alternative is to use for example Bluebird.promisify() for this (see also promisifyAll() for doing this to a complete module at once).

The perfect ES6 native Promises version

    return new Promise((resolve, reject) => {
        fs.readFile(file, "UTF-8", function(error, text) {
            if (error) {
                reject(new Error(error));
            } else {
                resolve(text);
            }
        });
    });

If we leverage ES6 features we can write this more elegantly. The explicit try-catch can be eliminated since the constructor will call the reject handler in case an exception occurrs in the function. We can also remove the gatekeeper logic since if resolved more than once, the further resolves are just ignored.

But again, consider using ready made library for promisifying callbacks.

Node doesn't wait for your database call to finish?

Learn how asynchronous calls work and make your app run as you intended. Get short email course on asynchronicity and two chapters from Finish Your Node App.