Node.js, the pyramid of doom (even with async), can you write it better?

Question

I consider myself a very experienced node.js developer.

Yet I still wonder if there is a better way to write the following code so I don't get the pyramid of doom... Now I went easy on you, I have some code that my pyramid gets as high as 20 floors, no kidding; and that's WITH using async.js !!!

The problem is really that I have many dependencies on previews variables so everything must be nested. The guy that wrote the book "Async Javascript, build more responsive Apps with less code" explains that he would put the functions at the root scope, which sure, would get rid of the pyramid, but now you would have a whole bunch of high scope variables (possibly even global, depending at the scope you declare them at) and this pollution can result in some pretty nasty bugs (this could cause var conflicts with other scripts if set at global space (sure you could use self invoking functions, more yachhh... or even worse, since we are dealing with async, variable overrides...). In fact, the beauty of closure is pretty mush out the door.

What he recommend is doing something like:

function checkPassword(username, passwordGuess, callback) {
    var passwordHash;
    var queryStr = 'SELECT * FROM user WHERE username = ?';
    db.query(selectUser, username, queryCallback);
    function queryCallback(err, result) {
        if (err) throw err;
        passwordHash = result['password_hash'];
        hash(passwordGuess, hashCallback);
    }

    function hashCallback(passwordGuessHash) {
        callback(passwordHash === passwordGuessHash);
    }
}

again, not a clean approach IMHO.

So, if you look at my code (again, this is just a snippet, I get much bigger nests in other places) you will often see my code getting further and further apart from the left; and that's with using things like waterfall and async forEach...

here is a small example:

ms.async.eachSeries(arrWords, function (key, asyncCallback) {
    pg.connect(pgconn.dbserver('galaxy'), function (err, pgClient, pgCB) {
        statement = "SELECT * FROM localization_strings WHERE local_id = 10 AND string_key = '" + key[0] + "'";
        pgClient.query(statement, function (err, result) {
            if (pgconn.handleError(err, pgCB, pgClient)) return;
            // if key doesn't exist go ahead and insert it
            if (result.rows.length == 0) {
                statement = "SELECT nextval('last_resource_bundle_string_id')";
                pgClient.query(statement, function (err, result) {
                    if (pgconn.handleError(err, pgCB, pgClient)) return;
                    var insertIdOffset = parseInt(result.rows[0].nextval);
                    statement = "INSERT INTO localization_strings (resource_bundle_string_id, string_key, string_revision, string_text,modified_date,local_id, bundle_id) VALUES ";
                    statement += "  (" + insertIdOffset + ",'" + key[0] + "'," + 0 + ",'" + englishDictionary[key[0]] + "'," + 0 + ",10,20)";
                    ms.log(statement);
                    pgClient.query(statement, function (err, result) {
                        if (pgconn.handleError(err, pgCB, pgClient)) return;
                        pgCB();
                        asyncCallback();
                    });
                });
            }
            pgCB();
            asyncCallback();
        });
    });
});

On my deep scripts I counted over 25 closing parenthesis, CRAZY, and all while remembering where to call my last callBack so async continues to next iteration...

Is there a solution to this problem? Or it is just the natrure of the beast?

Note that you shouldn't use `passwordHash === passwordGuessHash` to compare a hashed string with a password, as this is vulnerable to [timing attacks](http://en.wikipedia.org/wiki/Timing_attack). Your crypto hash library ([bcrypt](http://codahale.com/how-to-safely-store-a-password/)) should have a cryptographically-safe comparison function you can use instead. — Stuart P. Bentley, Aug 29 '14 at 20:46
If you want to -estetically- avoid having a lot of nesting, you can use `nimble` to chain the calls. If you want to -stackframely- avoid having a lot of nesting, you can defer the next call using a timer, declaring each function. — Luis Masuelli, Aug 29 '14 at 20:47
You can use a promise-based style using something like [`q`](https://github.com/kriskowal/q). — tcooc, Aug 29 '14 at 20:47
There is no framework that I know of for node.js to get away from async... — born2net, Aug 29 '14 at 20:50
have you tried wait.for? https://github.com/luciotato/waitfor — Lucio M. Tato, Sep 04 '14 at 21:34

score 3 · Accepted Answer · edited Dec 18 '14 at 15:07

As Mithon said in his answer, promises can make this code much clearer and help to reduce duplication. Let's say that you create two wrapper functions that return promises, corresponding to the two database operations you're performing, connectToDb and queryDb. Then your code can be written as something like:

ms.async.eachSeries(arrWords, function (key, asyncCallback) {
  var stepState = {};
  connectToDb('galaxy').then(function(connection) {
    // Store the connection objects in stepState
    stepState.pgClient = connection.pgClient;
    stepState.pgCB = connection.pgCB;

    // Send our first query across the connection
    var statement = "SELECT * FROM localization_strings WHERE local_id = 10 AND string_key = '" + key[0] + "'";
    return queryDb(stepState.pgClient, statement);
  }).then(function (result) {
    // If the result is empty, we need to send another 2-query sequence
    if (result.rows.length == 0) {
       var statement = "SELECT nextval('last_resource_bundle_string_id')";
       return queryDb(stepState.pgClient, statement).then(function(result) {
         var insertIdOffset = parseInt(result.rows[0].nextval);
         var statement = "INSERT INTO localization_strings (resource_bundle_string_id, string_key, string_revision, string_text,modified_date,local_id, bundle_id) VALUES ";
         statement += "  (" + insertIdOffset + ",'" + key[0] + "'," + 0 + ",'" + englishDictionary[key[0]] + "'," + 0 + ",10,20)";
         ms.log(statement);
         return queryDb(stepState.pgClient, statement);
       });
     }
  }).then(function (result) {
    // Continue to the next step
    stepState.pgCB();
    asyncCallback();
  }).fail(function (error) {
    // Handle a database error from any operation in this step...
  });
});

It's still complex, but the complexity is more manageable. Adding a new database operation to every "step" no longer requires a new level of indentation. Also notice that all error handling is done in one place, rather than having to add an if (pgconn.handleError(...)) line every time you perform a database operation.

Update: As requested, here's how you might go about defining the two wrapper functions. I'll assume that you're using kriskowal/q as your promise library:

function connectToDb(dbName) {
  var deferred = Q.defer();
  pg.connect(pgconn.dbserver(dbName), function (err, pgClient, pgCB) {
    if (err) {
      deferred.reject(err)
    } else {
      deferred.resolve({pgClient: pgClient, pgCB: pgCB})
    }
  });
  return deferred.promise;
}

You can use this pattern to create a wrapper around any function that takes a single-use callback.

The queryDb is even more straightforward because its callback gives you either a single error value or a single result value, which means that you can use q's built-in makeNodeResolver utility method to resolve or reject the deferred:

function queryDb(pgClient, statement) {
  var deferred = Q.defer();
  pgClient.query(statement, deferred.makeNodeResolver());
  return deferred.promise;
}

For more information on promises, check out my book: Async JavaScript, published by PragProg.

Hi Trevor, would you mind including an example of onnectToDb and queryDb, just wanted to make sure I get it from a Pro ;) — born2net, Aug 29 '14 at 22:17

score 2 · Answer 2 · answered Aug 29 '14 at 20:50

The problem to this sort of thing is promises. If you haven't heard of them, I suggest reading up on kriskowal's q.

Now, I don't know if the db.query returns a promise or not. If it doesn't you might be able to find a db-wrapper that does or a different db library. If that is not an option, you may "promisify" the db-library you're using. See Howto use promises with Node, and especially the section "Wrapping a function that takes a Node-style callback".

Best of luck! :)

Anything that takes a callback can be made into a promise. The answer even gives the link on how to do so. — Preston S, Aug 29 '14 at 20:56

score 1 · Answer 3 · answered Aug 29 '14 at 20:53

The simplest way to combat the async pyramid of hell is to segregate your async callbacks into smaller functions that you can place outside your main loop. Chances are you can at least break some of your callbacks into more maintainable functions that can be used elsewhere in your code base, but the question you're asking is a bit vague and can be solved in a large number of ways.

Also, you should consider what Stuart mentioned in his answer and try to combine some of your queries together. I'm more concerned that you have 20+ nested calls which would indicate something seriously erroneous in your callback structure so I'd look at your code first before anything else.

Stuart P. Bentley · Answer 4 · 2014-08-29T20:54:07.003

0

Consider rewriting your code to have less back-and-forth with the database. The rule of thumb I use to estimate an app's performance under heavy load is that every async call will add two seconds to the response (one for the request, and one for the reply).

For example, is there maybe a way you could offload this logic to the database? Or a way to "SELECT nextval('last_resource_bundle_string_id')" at the same time as you "SELECT * FROM localization_strings WHERE local_id = 10 AND string_key = '" + key[0] + "'" (perhaps a stored procedure)?

edited Aug 29 '14 at 20:54

answered Aug 29 '14 at 20:47

Stuart P. Bentley

10,195
10
55
84

that could help, but just a workaround and sometimes you simply have to, plus I just gave this as an example, in many other scenarios like reading files, you simply have to async... – born2net Aug 29 '14 at 20:52
You'll always have to async, but you can reduce *how much* you async. For example, if I had to read a file, to get the name of another file to read, to get the name of another file to read, I would look into refactoring my request / naming scheme so I can start at that last file. – Stuart P. Bentley Aug 29 '14 at 20:53
1

Reducing your number of hops isn't a workaround, it's *good engineering and design*. – Stuart P. Bentley Aug 29 '14 at 20:59

Scott Long · Answer 5 · 2015-09-30T21:51:54.093

I break each level of the pyramid of doom into a function and chain them one to the other. I think it is a lot easier to follow. In the example above i'd do it as follows.

ms.async.eachSeries(arrWords, function (key, asyncCallback) {

  var pgCB;
  var pgClient;

  var connect = function () {
    pg.connect(pgconn.dbserver('galaxy'), function (err, _pgClient, _pgCB) {
      pgClient = _pgClient;
      pgCB = _pgCB;
      findKey();
    });
  };

  var findKey = function () {
    statement = "SELECT * FROM localization_strings WHERE local_id = 10 AND string_key = '" + key[0] + "'";
    pgClient.query(statement, function (err, result) {
      if (pgconn.handleError(err, pgCB, pgClient))
        return;
      // if key doesn't exist go ahead and insert it
      if (result.rows.length == 0) {
        getId();
        return;
      }
      pgCB(); 
      asyncCallback(); 
    });

  };

  var getId = function () {
    statement = "SELECT nextval('last_resource_bundle_string_id')";
    pgClient.query(statement, function (err, result) {
      if (pgconn.handleError(err, pgCB, pgClient))
        return;
      insertKey();
    });
  };

  var insertKey = function () {
    var insertIdOffset = parseInt(result.rows[0].nextval);
    statement = "INSERT INTO localization_strings (resource_bundle_string_id, string_key, string_revision, string_text,modified_date,local_id, bundle_id) VALUES ";
    statement += "  (" + insertIdOffset + ",'" + key[0] + "'," + 0 + ",'" + englishDictionary[key[0]] + "'," + 0 + ",10,20)";
    ms.log(statement);
    pgClient.query(statement, function (err, result) {
      if (pgconn.handleError(err, pgCB, pgClient))
        return;
      pgCB();
      asyncCallback();
    });
  };

  connect();

});

Node.js, the pyramid of doom (even with async), can you write it better?

5 Answers5

Linked