Who should handle errors in THE callback?

[defeo]

Hi,

Most engines in consolidate do error handling using a code similar to this one (example taken from the Eco wrapper):

try {
  fn(null, engine.render(str, options));
} catch (err) {
  fn(err);
}

I'm concerned about what happens when an exception is raised inside the callback fn, but outside of the engine.render call. In my opinion, fn should be responsible for handling errors happening inside engine.render, but not for errors happening inside fn itself.

Consider an example where the code of fn contains an undefined reference. In this case an exception is raised inside the first call to fn, caught, then eventually raised again inside the second call to fn. This is not necessarily the behavior one would expect. For a concrete example, consider the following express application

var express = require('express');
var cons = require('consolidate');

var called = false

express()
  .engine('js', cons.hogan)
  .set('views', __dirname)
  .get('*', function(req, res) {
    res.render('demo.js', function(err, result) {
      if (called) throw new Error('Already called');
        called = true;
        ress.send(result);    // ress is undefined
      });
  }).listen(8080);

Here the real problem is that the variable ress is undefined, but the user receives a Error('Already called') instead. This example is inspired by a real world issue I had trying to use async.js together with consolidate.js.

I'm relatively new to express, but IMHO it would make more sense to rewrite the engine wrappers along the following lines:

try {
  var result = engine.render(str, options);
} catch (err) {
  fn(err);
  return;
}
fn(null, result);

[tj]

yeah we shouldn't invoking the callback twice, this is a common bug with node crap

[ForbesLindesay]

This was the thinking behind #85 which fixes this issue.

[doowb]

Closing this but working on getting the changes merged in from the mentioned PR.