promise chain execution affected by whether an error handler is registered

kbullaughey commented 6 years ago

I originally posted issue emberjs/ember.js#15864 to ember.js, but when I found I could explain the strange behavior purely based on the version of backburner used, perhaps the issue should be here.

The crux of it is that which callbacks are called in a promise chain is affected by whether I've called Ember.onerror or not. It seems that somehow backburner is behaving differently depending on if a callback has been registered, even if it's an empty callback.

If you prefer me to migrate the issue here, I'm happy to. Here's a repo where I distill and illustrate the problem: https://github.com/kbullaughey/ember-effect-of-onerror

kbullaughey commented 6 years ago

The commit that explains this difference is 4eb0bceeda2b and it relates to issue #246.

It seems that in the presence of an error handler thrown errors are caught but not re-thrown. It seems if the main purpose of registering an error handler through Ember.onerror() is for logging, you'd want the behavior to be the same whether or not a handler is registered. I can fix this problem by re-throwing the error as follows:

let onError = getOnError(this.options);

if (onError) {
  try {
    return method.apply(target, args);
  } catch (error) {
    onError(error);
    throw error;  // ADDED THIS LINE
  }
} else {
  return method.apply(target, args);
}

}

Although in the discussion of #246, catching the error in this way in join() was done to make it behave the same as run(). I don't know enough about backburner to know what is the desired behavior.

Thoughts?

kbullaughey commented 6 years ago

To me it seems swallowing errors when a global error handler is registered is dangerous.

For example, suppose we have methods step1, step1Success, step1Fail, and step2 and put these into a promise chain:

step1().then(bind(null, step1Success), bind(null, step1Fail)).then(step2);

If the promise returned by step1 rejects, causing the step1Fail reject handler to be called, and then this handler in turn throws an exception, step2 will be called. But this will only happen if an global error handler was registered. In the absence of such a handler, step2 will not be called.

If the methods were not part of the runloop:

step1().then(step1Success, step1Fail).then(step2);

Then if step1 rejects, and step1Fail throws an exception, then the step2 will not be executed, just like if there is no global error handler registered.

Thus the behavior with a global error handler is different than you'd expect from either an ordinary promise chain or a promise chain involving the runloop but with no global error handler.