Skip to content
This repository has been archived by the owner on Jan 5, 2023. It is now read-only.

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

Closed
nestradaOBS opened this issue Feb 3, 2014 · 4 comments

Comments

@nestradaOBS
Copy link

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
Copy link
Member

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
Copy link
Member

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

@johnyanarella
Copy link
Member

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().)

karfau pushed a commit to karfau/promise-as3 that referenced this issue Feb 3, 2014
…all future execution of scheduled callbacks.

* Revised `nextTick()` to perform safer next tick scheduling - i.e. where a scheduled callback can throw an Error without affecting other scheduled calls.
* Modified `log()` and `done()` to use this safer `nextTick()` implementation to rethrow errors.
* Moved the optimized CallbackQueue to be an internal class used only by Consequence, where its underlying assumption (that execute() will not throw an error) is valid.
* Cloned additional CallbackQueue performance optimizations from promise.coffee and Deft JS - ensuring that only a single Array instance is used, rather than allocating new Arrays (via push(), etc.) whenever new callbacks are added.

Fixes CC-Archived#29
(cherry picked from commit a33c977)
@nestradaOBS
Copy link
Author

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!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants