BackburnerJS / backburner.js

A rewrite of the Ember.js run loop as a generic microlibrary
MIT License
392 stars 80 forks source link

Revert extra try/catch wrapping of `run.join`. #277

Closed rwjblue closed 6 years ago

rwjblue commented 6 years ago

4c26eedcb added try/catch wrapping to Backburner.prototype.join in the scenario where it is actually joining an existing instance. This seemed completely valid on the surface (who would want to introduce error swallowing?!?!?!), but unfortunately it actually introduced a regression.

In the case where run.join is called without already being within an existing instance, it was already properly handling try/catch wrapping and calling the provided onError. In fact, the test added in ed3dd0a demonstrates exactly that! The test was intending to cover the new logic, but it actually only covers the error handling behaviors of Backburner.prototype.run itself.

In the case where run.join is called from within an existing instance, the new logic introduced error swallowing behaviors when the onError callback itself did not rethrow the error. This is because run.join from within an existing instance is already invoked from within Queue.prototype.invokeWithOnError.

The added try/catch wrapping that was added in 4c26eedcb caused the regression reported in https://github.com/BackburnerJS/backburner.js/issues/274 (and https://github.com/emberjs/ember.js/issues/15864), due to the usage of run.bind to wrap the rejection handler combined with the fact that Ember configures RSVP to handle all promise resolution within a Backburner queue which essentially created this situation:

let bb = new Backburner(['errors'], {
  onError(error) { console.log(error.message) }
});

bb.run(() => {
  try {
    // RSVP runs the then callbacks here
    // in the example case, the callbacks were also
    // wrapped in `run.bind` (which uses run.join internally)

    // this simulates the run.bind callback from the example
    bb.join(() => {
      throw new Error('foo');
    });

    // after 4c26eedcb the bb.join above no longer bubbles errors
    // because the registered onError does not rethrow
  } catch(e) {
    // RSVP relies on this error catching to know that a promise
    // should be properly rejected
  }
});

This PR adds a number of additional tests for various combinations of Backburner.prototype.join, having an onError setup, joining an existing instance, creating a new instance.

Fixes #274 Closes #276 Addresses emberjs/ember.js#15864