CC-Archived / promise-as3

Promises/A+ compliant implementation in ActionScript 3.0
168 stars 55 forks source link

If an error is thrown in otherwise() handler, future promises are broken #29

Closed nestradaOBS closed 10 years ago

nestradaOBS commented 10 years ago

I've encountered what I'm sure is a bug regarding error handling in promises. I'm using them with robotlegs which uses a VigilenceExtension which basically throws errors whenever a logger logs an error or a warning. Whether or not that is a good thing, I've discovered that whenever an error is thrown in a promise it cripples any other promises created afterwards. Here is a test case which IMO should pass :

package {

    import com.codecatalyst.promise.Deferred;
    import com.codecatalyst.promise.Promise;

    import org.flexunit.async.Async;
    import org.hamcrest.assertThat;
    import org.hamcrest.object.equalTo;

    public class PromiseTest {

        [Test(async)]
        public function ifFirstPromiseThrowsErrorInOtherwiseSecondPromiseIsUnaffected() : void {

            var results : Object = {};

            // First promise
            var def1 : Deferred = new Deferred();
            var promise1 : Promise = def1.promise;
            promise1.otherwise(function (error : *) : void {
                results.error = error;
                throw new Error();
            }).done();

            Async.delayCall(this, function () : void {
                def1.reject('error');
            }, 250);

            // Second promise
            var def2 : Deferred = new Deferred();
            var promise2 : Promise = def2.promise;
            promise2.then(function (value : *) : void {
                results.result = value;
                validate();
            }).done();

            Async.delayCall(this, function () : void {
                def2.resolve('This is a result');
            }, 500);

            var validate : Function = Async.asyncHandler(this, function () : void {
                assertThat(results.error, equalTo('error'));
                assertThat(results.result, equalTo('This is a result'));
            }, 5000);
        }
    }
}

In order to get this to pass, I simply need to comment out the throw new Error() in the 1st promise's otherwise, but of course the purpose of the test is to be able to handle errors. Any help would be much appreciated? Are there any unit tests planned for this library?

johnyanarella commented 10 years ago

See #11 regarding unit tests. @karfau is porting the Promises/A+ specification test suite to: https://github.com/karfau/promises-tests-as3. We will be restructuring the repo soon to introduce FlexUnit tests for Promise-AS3 specific utility methods, etc. that would not be covered by the standard validation suite.

While working on a JavaScript variation of this same code for Deft JS (where I do have extensive unit tests), I discovered this same issue. The problem relates to recent optimizations in nextTick() / CallbackQueue and their use in Promise::log() and Promise::done().

I'll port the associated fix for this back later this afternoon and will post an update here when it is available. Thanks for reporting this issue!

johnyanarella commented 10 years ago

When you get a chance, could you grab the latest and verify this fixes the issue in your environment, too? Thanks!

johnyanarella commented 10 years ago

NOTE: it was the done() call - which schedules an error to be rethrown - not the otherwise() call that was triggering the issue. (By removing the throw statement in your rejection handler, the promise returned by that then() call is resolved with the rejection handler's return value, so done() did not end up rethrowing an error.)

It was also possible for log() to trigger the same issue. (Or any call that throws an error that was scheduled via the previous implementation of nextTick().)

nestradaOBS commented 10 years ago

Works like a charm! Thanks for the quick fix! I'm currently building from source but it would be nice to have some versioning though ;) Anyhow thanks again and great work!