promise chain execution affected by whether an error handler is registered

[kbullaughey]

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]

The commit that explains this difference is 4eb0bce 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]

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.

[rwjblue]

Awesome digging @kbullaughey! Thank you for tracking this down so well.

I do believe that #246 does do the correct thing (run and run.join should have the same error semantics).

[rwjblue]

Grr, accidentally submitted that comment before finishing (sorry).

I totally agree that there is a bug here, I was just working out how to detangle things in Ember-land around emberjs/ember.js#14898 which I believe is generally related to the thing you are hitting here.

[kbullaughey]

I don't necessarily suggest accepting my pull request. I believe it may introduce an inconsistency between run() and join().

[rwjblue]

Thanks for the continued investigation here @kbullaughey. I have been digging at this and related problems most of the day. Should hopefully have some PR's up by EOW...

[rwjblue]

I submitted #277 to address this, mind reviewing?

[kbullaughey]

It all looks good to me. And I can confirm that it solves the problems I was having.

Though one question, this test has two assertions but assert.expect(1) and I the onError method is not called. Is this intentional? Maybe there's dead code here?

[rwjblue]

Ya, mind fixing that up in a separate PR? Definitely seems confusing at least, most likely a copy paste error...