Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Handle synchronous exceptions better #85

Open
wants to merge 1 commit into
from

Conversation

Projects
None yet
2 participants
Contributor

ForbesLindesay commented Jan 7, 2013

Same as #83 but with a fixed diff

@simov simov commented on the diff Jan 8, 2013

lib/consolidate.js
@@ -149,10 +149,37 @@ function fromStringRenderer(name) {
}
/**
+ * Convert a function that may throw syncronous errors into a well behaved async
+ * function that calls it's callback exactly once.
+ *
+ * @param {Function} fn
+ * @return {Function}
+ * @api private
+ */
+function handleErrors(fn) {
+ return function (str, options, cb) {
+ var finished = false;
@simov

simov Jan 8, 2013

Contributor

What does finished do? Thanks.

@ForbesLindesay

ForbesLindesay Jan 8, 2013

Contributor

It prevents the callback being called multiple times. For example:

cons.temp.render = function (str, opts, cb) {
  try {
    cb(new Error('this never goes well');
  } catch (ex) {
    cb(ex);
  }
};

cons.temp.render('', {}, function (err, res) {
  console.log('There was an error');
  throw err;
});

In there the console.log will be hit twice:

There was an error
There was an error

Error: this never goes well
  Stack Trace
  Stack Trace
  Stack Trace
  Stack Trace

This kind of bug exists in reality in lots of places in the code and it's really hard to debug, so it's easiest to just fix it at the top level.

@simov

simov Jan 8, 2013

Contributor

Can we just

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

ForbesLindesay Jan 8, 2013

Contributor

You can for synchronous parsers, and that's always sufficient for the synchronous parsers, but it does nothing to prevent poorly implemented async templating libraries.

@simov

simov Jan 8, 2013

Contributor

I don't even know how it's possible to make a synchronous parser in node .. and that's why I'm curious why most of the engines don't have a callback for it's compile/render functions.

@ForbesLindesay

ForbesLindesay Jan 8, 2013

Contributor

The only thing that would force you to make your template parsing async is if you allow users to do async work inside their templates. Node.js can do pretty much anything synchronously if you want it to (making http web requests takes a bit of work). Most of the libraries just transform strings to functions so there's no real reason for there to be any async work there.

P.S. The only two that actually operate asyncronously are dust and qejs the rest just trick you into thinking they do by providing a node style apie when in fact their syncronous (for an example take a look at the source code for the render function for jade no Async there P.S. it also nicely demonstrates a template library that suffers from the problem finished would fix.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment