Don't catch errors thrown in callbacks

[charlierudolph]

Currently oboe catches errors thrown in callbacks and triggers the fail event

/** 
*  wrap a callback so that if it throws, Oboe.js doesn't crash but instead
*  handles it like a normal error
*/
function protectedCallback( callback ) {
  return function() {
    try {      
      return callback.apply(oboeApi, arguments);   
    } catch(e)  {

      // An error occured during the callback, publish it on the event bus 
      oboeBus(FAIL_EVENT).emit( errorReport(undefined, undefined, e));
    }      
  }   
}

I agree that oboe.js should not crash if there is an error thrown in a callback. However, I disagree that the error should be caught and passed to the fail event handler. Firstly, catching errors in functions it receives as arguments should not be a responsibility of oboe. Secondly, the fail event handler should just be for unsuccessful requests

Thus I suggest the code be updated to the following:

/** 
*  execute the callback in its own event loop so that if it throws, Oboe.js doesn't crash
*/
function protectedCallback( callback ) {
  return function() {
    setTimeout(callback.bind(oboeApi, arguments));     
  }   
}

This way errors in callbacks do not cause oboe to crash and errors thrown by callbacks are not caught.

I am happy to implement this change, but want to get feedback before I spend time updating the tests.

[jimhigson]

I haven't tested but I suspect the proposed change could be quite inefficient. The reason is that with a long JSON there can be many callbacks in one JS frame. I'm not sure what the implications are of setting (say) 100 callbacks to call right away but I strongly suspect this will slow things down quite a bit.

There may be another way to safely stop the error from crashing oboe.

What is the situation where catching the error causes a problem? Of course you can still catch your own errors.

If the problem is that errors aren't being reported, maybe there could be a default behaviour that if an error occurs and there is no error handler it is put to console.error something like 'uncaught error in blah callback: (err)'

[charlierudolph]

The biggest problem for me is that oboe catching errors makes it very hard to debug. I never suspected oboe caught my callback errors and when I heard from users that my website wasn't working but there were no errors, I had no idea what was going on. Secondly, if you want to start logging errors thrown by callbacks, you have to complicate the logic you have for handling request failures.

My number one solution would be oboe simply doesn't catch callback errors. Just remove the protection entirely. Let oboe js crash if there is an error in the user callback. Its the users fault anyway and it will be much easier to debug. Since you can't catch errors in the fail callback no matter what, I think this makes the most sense.

A second solution I could make do with is to have a new event, callbackError, so that the fail event is solely for requests failing. On callbackError I would just throw the error anyway so it would be my own way of implementing my number one solution.

[charlierudolph]

@jimhigson any more thoughts?

[goloroden]

I made the same experience as @charlierudolph, hence I'd also like to see a solution that is better debuggable.

[charlierudolph]

@jimhigson how about instead of catching errors and calling the fail event, it simply catches them and throws them in their own event loop to prevent oboe from crashing. Therefore in the base case, there is no loss to performance and when there are errors they will just be reported to the user as they would expect.

/** 
*  wrap a callback so that if it throws, throw the error in a different event loop so Oboe.js doesn't crash
*/
function protectedCallback( callback ) {
  return function() {
    try {      
      return callback.apply(oboeApi, arguments);   
    } catch(e)  {
      setTimeout(function() { 
        throw e 
      });
    }      
  }   
}

Happy to implement if you agree

[ryan-williams]

+1, I love Oboe so far but this behavior (catching errors) is really confusing. I can't figure out where the original error is coming from. Seems like the thrown object is just an array with a string in it?

{ statusCode: undefined,
  body: undefined,
  jsonBody: undefined,
  thrown: [ReferenceError: id is not defined] }

[ryan-williams]

(the above is what I'm seeing from logging the argument to the .fail callback via https://github.com/baryon/tracer.

After removing the .fail callback, oboe is swallowing all thrown errors, from what I can tell? Very unexpected/undesired.

[ryan-williams]

In case others are struggling with this, it seems like simply re-throwing e.thrown in a fail handler behaves effectively as if the error had not been caught by Oboe in the first place, e.g.:

      oboe(c).node('!', function(e) {
        handleEvent(e);
      }).fail(function(e) {
        throw e.thrown;
      });

This throws with the stack trace of the original error, allowing me to debug.

[aldipower]

+1 This behavior makes it impossible to debug my application. I had the situation of a ReferenceError that was thrown in a component of my app that has nothing to do with the oboe-request at all. It seems that oboe catches all my errors on application wide level. :unamused: Obeo is definitely not responsible for this! And this could lead to horrible bugs, if the developer is not aware of this behavior.

[jimhigson]

Yes, I'm in agreement. Struggling to find the time to change this behaviour, but agree on changing it. This has annoyed me also when using Oboe.js in various projects. Hopefully I'll have a bit of down time coming up between contracts soon.

On Sat, Jun 20, 2015 at 3:15 PM, Felix Gertz notifications@github.com wrote:

+1 This behavior makes it impossible to debug my application. I had the situation of a ReferenceError that was thrown in a component of my app that has nothing to do with the oboe-request at all. It seems that oboe catches all my errors on application wide level. [image: :unamused:] Obeo is definitely not responsible for this! And this could lead to horrible bugs, if the developer is not aware of this behavior.

— Reply to this email directly or view it on GitHub https://github.com/jimhigson/oboe.js/issues/62#issuecomment-113771572.

[ryan-williams]

@jimhigson thanks, good to know!

@aldipower does the workaround I posted above help you? just want to make sure I'm not missing anything as I depend on it in the meantime :)

[pierluigi]

+1 for @charlierudolph 's solution