# Async, callbacks, and closures — am I doing it right?

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.

[EDIT] I just realised I didn't ask any questions. Here they are:

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?

I think that's it, but if you notice any other issues, let me know!

var mysql = require('mysql');
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));
}

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.

-
@Raynos yes. Oops. Just a test server running on 127.0.0.1, but still... –  Paul d'Aoust Nov 16 '11 at 16:22
@Raynos would you be able to delete your comment, to hide the evidence? I removed the password in my code sample. –  Paul d'Aoust Nov 16 '11 at 16:34
There's a change log on the question, you know. –  Raynos Nov 16 '11 at 16:55
oh yeah, you're right. Dang. –  Paul d'Aoust Nov 16 '11 at 17:10

var makeQuery = function makeQuery (id) {

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

results = results[0];


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.

-
Re: having makeQuery a local variable, I guess you're right... I've just been too Crockfordised, that's all :) And in regards to augmenting results and then totally throwing it away, I know... just doing a speed test, so I don't want to actually use it. Thanks for the confirmation that using a constructor is correct... Also, I updated my question with actual explicit questions! My big concern is whether the constructor function is creating 20,000 contexts on account of the closure in which the returned function needs the id argument. Oh, and whether that's a problem at all. –  Paul d'Aoust Nov 16 '11 at 16:32
Yep, it's a dummy example. Trying to simulate the real-world use case, in which 20,000 people might indeed hit the server at once with their tiny little queries. Glad to know V8 won't mind about all my contexts. So when you say it splits that returned function out into its own, do you know if it would still do that if it were an anonymous function? Another Crockford-instilled anxiety. –  Paul d'Aoust Nov 16 '11 at 16:37
@Pauld'Aoust functions are functions. There's no difference –  Raynos Nov 16 '11 at 16:54
Oh, okay. Crockford mentioned something about naming your functions so that you get 20,000 references to one function rather than 20,000 anonymous functions in the heap. Perhaps V8 doesn't do it that way, or else I didn't understand him correctly. At any rate, I guess it's good practice to name your callbacks so you get something to look at in your stack traces. –  Paul d'Aoust Nov 16 '11 at 17:14
@Pauld'Aoust the point is you have one named function nameQuery and 20000 anonymous functions (the functions returned from the nameQuery). –  Raynos Nov 16 '11 at 17:22

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
});
});
}

-