Guard against exceptions in the setup steps.

[winding-lines]

When there are exceptions in the setup steps (for example needs component missing)
the implementation is not properly capturing those exceptions.
This fix uses the slighlty longer syntax for creating promises which
wraps the call in a function and allows RSVP to properly capture
the exception. When using the shorter RSVP.resolve() form the exception
happens outside (before) the RSVP invocation and cannot be captured.

[chadhietala]

Wouldn't this just work.

  return Ember.RSVP.resolve(step.call(context)).then(nextStep, function(err) {
   throw err;
  })

[winding-lines]

@chadhietala your proposal does not work. You can rewrite both the original code and your proposal as the following:

var current = step.call(context); // throws exception, promise code never reached
return Ember.RSVP.resolve(current).then(..)

The call to step.call() will happen before the promise, even though it is written on the same line. With the proposed change the promise is active before the throwing call and can setup a try-catch to catch the exception.

Note that promise.resolve() creates a new function anyways so there is no change in performance or memory allocation.

[rwjblue]

Yes, this is roughly what I was thinking also (though with .catch instead of two args to .then).

[winding-lines]

@rwjblue a .catch() won't work either. The issue is with capturing the exception and translating it to a rejected promise. The .catch() only changes how that promise is handled later on.

You can play with the following code to better understand what I mean. The commented code is my proposal, the uncommented code is the current approach.

var RSVP = require('rsvp');

function throws() {
  throw Error("a");
}


/*
var promise = new RSVP.Promise(function(resolve, reject) {
  resolve(throws());
});
*/

var promise = RSVP.resolve(throws());

promise.then(function(value) {
  console.log( "then -> " + value );
}, function(value) {
  console.log( "failure -> " + value );
});

[rwjblue]

We should use RSVP.resolve().then(function() { return step.call(); })

[winding-lines]

@rwjblue yes that would also work, however you are 'wasting' a resolve().

[rwjblue]

Ya,
makes sense

[rwjblue]

This looks good, can you add a test for this?

[winding-lines]

@rwjblue I've added the test. Without the improved exception handling the whole test loop breaks when the new test is hit - it's test 14 below. What we are seeing in production is that the CI builds 'hang'.

With the new code the processing continues after test 14. In normal operation the test itself would fail instead of their setup. More importantly for us there is no 'hanging'.

[winding-lines]

Let me know if I should collapse all the commits in one.

[rwjblue]

Thanks for your work on this!

[winding-lines]

Thanks for merging!

On Jul 6, 2015, at 5:32 AM, Robert Jackson [email protected] wrote:

Thanks for your work on this!

—
Reply to this email directly or view it on GitHub.