Async, callbacks, and closures

Question

After having spent a month or two trying to learn JavaScript, especially functional programming, async, and closures, I finally get it. But I don't know if I'm writing elegant code... Specifically, I'm not sure if I'm creating too many closures, because I don't entirely grasp when contexts remain and when they're marked for garbage collection.

You can probably tell from the example that I'm using node.js -- basically, this is just a piece of test code that connects to a database, gets 20,000 records asynchronously with individual SELECT queries (I'm just doing some profiling, don't worry), and manipulates the data. At the end, it tells me how long it took and closes the MySQL connection.

In order to get a final timestamp and close the connection, I needed something like the async library to keep track of all the various asynchronous functions that are being spawned. It takes all my queries as an array of functions (using a factory called makeQuery()) and then runs a callback to do the cleanup.

Questions:

1. Am I creating a huge amount of contexts by creating and returning the function doQuery() which uses the argument id in the makeQuery() function, thereby causing 20,000 contexts to persist until async.parallel() is run?

2. Is the factory function makeQuery() a good and tidy way to create this array of doQuery() functions? Can you suggest any better way to do it?

var mysql = require('mysql');
var radix64 = require('./lib/radix64');
var async = require('async');

var client = mysql.createClient({user: 'root', password: '12345678'});
client.query('USE nodetest');

var startTime = (new Date()).getTime(); // poor man's profiling
var queries = []; // this will be fed to async.parallel() later

var makeQuery = function makeQuery (id) { // factory function to create the queries
    return function doQuery (cb) {
        client.query('SELECT * FROM urls WHERE id='+id,
            function logResults (e, results, fields) {
                results = results[0];
                results.key = radix64.fromNumber(results.id); // yep, I'm converting the
                                                              // number into base-64 notation
                cb(); // if I wanted to do something useful at the end, I would have
                      // called cb(results) instead, which compiles an array of results
                      // to be accessed by the final callback
            }
        );
    };
};

for (i = 1001; i <= 20000; i ++) { // build the list of tasks to be done in parallel
    queries.push(makeQuery(i));
}

// run those tasks
async.parallel(queries, function finished () { // clean up and get the elapsed time
    console.log('done');
    client.end();
    console.log(((new Date()).getTime() - startTime) / 1000);
});

For those of you unfamiliar with async, it's a userland module built for node.js and the browser. Each function in the array that gets passed to async.parallel() is obliged to take a callback and then run that callback once it's done, in order to let async.parallel() know it's done.

Answer 1

var makeQuery = function makeQuery (id) {

There is no need to make makeQuery a local variable, just use a function declaration.

results = results[0];
results.key = radix64.fromNumber(results.id);

Your just augmenting the object, your not doing anything with it.

Other then that, your not creating new functions in a loop, your using a function constructor. This correct.

Your running all 19000 queries in parallel rather then in waterfall, which is also correct.

Edit:

Question 1:

You need to create 20000 contexts. Because there are 20000 values of i. Worrying about there being 20000 functions is a micro optimisation. v8 optimizes the hell out of your code.

It actually splits your functions into a hidden class seperate from the closure context, and in my memory you simply have 20000 values of i and one value for the function.

Just to remind you of rule 1.

Never underestimate V8

Question 2:

makeQuery as a factory is the correct pattern to use.

The only other optimisation you can do is to write a real SQL query rather then 20000 dummy ones. But I'll ignore that because it's a dummy example.

Answer 2

When in doubt, go with restricting the closure context to the enclosing function scope:

function makeLog(cb) {
    return function (e, results, fields) { // logResults
        results = results[0];
        results.key = radix64.fromNumber(results.id); // yep, I'm converting the
                                                      // number into base-64 notation
        cb(); // if I wanted to do something useful at the end, I would have
              // called cb(results) instead, which compiles an array of results
              // to be accessed by the final callback
    };
}

function makeQuery(id, aClient) { // factory function to create the queries
    return function (cb) { // doQuery
        aClient.query('SELECT * FROM urls WHERE id=' + id, makeLog(cb));
    };
}

for (i = 1001; i <= 20000; i++) { // build the list of tasks to be done in parallel
    queries.push(makeQuery(i, client));
}

Otherwise, you can as well get rid of the factories altogether:

for (i = 1001; i <= 20000; i++) { // build the list of tasks to be done in parallel
    queries.push(function (cb) { // doQuery
        client.query('SELECT * FROM urls WHERE id=' + i,
            function (e, results, fields) { // logResults
                results = results[0];
                results.key = radix64.fromNumber(results.id); // yep, I'm converting the
                                                              // number into base-64 notation
                cb(); // if I wanted to do something useful at the end, I would have
                      // called cb(results) instead, which compiles an array of results
                      // to be accessed by the final callback
            });
    });
}