teleportd/node-dynamodb — Issue #24: Incorrect use of try..catch inside ddb.js
Author: garo
Created: 2012-02-20 07:34:31+00:00
State: open

There's a number of places in the ddb.js where a try...catch block is used with a callback inside the block. This causes severe and hard-to-debug problems when the catch clause catches an exception which happens inside the try..catch block.

For example:

try {
var json = JSON.parse(body);

if(res.statusCode >= 300) {
var err = new Error(op + ' [' + res.statusCode + ']: ' + (json.message || json['__type']));
// code stripped away
cb(err);
}
else {
cb(null, json);
}
}
catch(err) {
cb(err);
return;
}

The try..catch is required for the JSON.parse() method, because the parse() will throw an exception if it tries to parse incorrect json. However notice that there's a cb() call inside the try..catch block. Now if there's any kind of exception inside the callback function (for example an assert from a test suite), the try..catch block will catch that exception and return it again in the callback. This is very bad behavior and can cause havoc for the call stack, especially when there's asynchronous code involved.

The correct way to correct this is not to do any callbacks inside a try..catch block. For example this is correct:

try {
var json = JSON.parse(body);
}
catch(err) {
cb(err);
return;
}

if(res.statusCode >= 300) {
var err = new Error(op + ' [' + res.statusCode + ']: ' + (json.message || json['__type']));
// code stripped away
cb(err);
}
else {
cb(null, json);
}

There's also similar bad usage on many of other functions, for example getItem, putItem, deleteItem etc.