parser-error vs user-error

[gjohnson]

I noticed that not just parser errors get handled nicely and emitted (if anyone is listening) but actually any exception that may occur within a "data" event handler. Whether this is an issue or not boils down to opinion, but might be worth to note it somewhere in the docs that error handling is not just limited to server/socket/parser errors.

spark.on('data', function (data) {
  throw new Error('oh noes!');
});

[3rd-Eden]

I didn't even realise that it would capture user thrown errors. Not sure if can be seen as a bug or as a feature, as I can that this can be useful but also really annoying.

I'm just going to label this as a bug for now. The quickest fix would be to force parsing to be async using a process.nextTick/setimmediate/setTimeout

Note: We could use https://github.com/NobleJS/setImmediate for this.

[gjohnson]

Yeah, emitting data on the next tick does the trick.

gjohnson@ace4052

[3rd-Eden]

Good to know that it's working, but not sure if that would be the optimal solution for this. I think it makes more sense to update the parsers https://github.com/3rd-Eden/primus/blob/master/parsers/json.js#L11 so they call the fn callback after the try / catch statement. This would also increase performance as V8 won't be de-optimising transformation handling.

[gjohnson]

Ohhhhh I see now... Good call, I'll go ahead and fix them that way. I don't really see the need to make the fn call async in this case now though. This what your thinking?

exports.decoder = function decoder(data, fn) {
  try { data = JSON.parse(data); }
  catch (e) { return fn(e); }
  fn(undefined, data);
}

Though, I guess you could also drop fn from the catch statement too... Need to test it, unless you know off the top of your head.

exports.decoder = function decoder(data, fn) {
  var err = undefined;
  try { data = JSON.parse(data); }
  catch (e) { err = e; }
  fn(err || undefined, data);
};

[3rd-Eden]

Yes, your first example was exactly what I was thinking. The second example would also work and can be written cleaner as well:

exports.decoder = function decoder(data, fn) {
  var err; // err is always undef when not nothing is assigned to it.

  try { data = JSON.parse(data); }
  catch (e) { err = e; }

  fn(err, data);
};

[gjohnson]

Ah with this fix, it shows an actual failed test when using binary-pack. When checking for \u2028 and \u2029 in https://github.com/3rd-Eden/primus/blob/master/spark.js#L206, currently it always assumes strings and uses packet.indexOf.

Want to just ignore anything that's not a string? Seems silly to cast the buffer right back to a string... Meh?

[3rd-Eden]

@gjohnson whoops, yes I think a simple typeof check would be sufficient to fix that.

[3rd-Eden]

Closing this. It was fixed in #43