New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rejecting with a promise is not fully tested #59

Open
Ziriax opened this Issue Apr 28, 2014 · 7 comments

Comments

Projects
None yet
4 participants
@Ziriax

Ziriax commented Apr 28, 2014

For learning promises, I made my own little implementation, see code below. I'm a Javascript newbie, so Douglas, don't be too hard on me please ;-)

For simplicity, it uses process.nextTick, so it only works in nodejs for now.

However, all 872 specs of your test suite pass, whether or not I 'adopt the state' of a 'thenable' passed as an argument to the reject method. See the single comment in the code.

What is the correct behavior?

Thanks a lot,
Peter Verswyvelen

var deferred = (function() {

    function Deferred() {
        this.promise = {
            then: this.then.bind(this),
        };
        this.thens = [];
    }

    function call_thens(state, value, thens) {
        thens.forEach(function (then) {
            var cfn = then[state],
                next_value = value,
                next_state = state;
            if (typeof cfn === "function") {
                try {
                    next_value = cfn(value);
                    next_state = "done";
                } catch (error) {
                    next_value = error;
                    next_state = "fail";
                }
            }

            then.next.transit(next_state, next_value);
        });
    }

    function then_transit(id, state, value) {
        if (this.state === id) {
            delete this.state;
            this.transit(state, value);
        }
    }

    var next_then_id = 0;

    Deferred.prototype = {
        asap: function () {
            process.nextTick(call_thens.bind(
                     this, this.state, this.value, this.thens));
            this.thens = [];
        },

        switchTo: function(state, value) {
            this.value = value;
            this.state = state;
            this.asap();
        },

        transit: function (state, value) {
            if (typeof this.state === "undefined") {
                // All tests succeeds with or without the state === "done" check?
                if (state === "done" && 
                     (typeof value === "function" || 
                         (typeof value === "object" && value !== null))) {
                    try {
                        if (value === this.promise)
                            throw new TypeError();

                        var then = value.then;
                        if (typeof then === "function") {
                            this.promise.then = then.bind(value);
                            var id = this.state = ++next_then_id;
                            try {
                                then.call(value,
                                    then_transit.bind(this, id, "done"),
                                    then_transit.bind(this, id, "fail"));
                            } catch (error) {
                                if (this.state === id)
                                    this.switchTo("fail", error);
                            } finally {
                                return;
                            }
                        }
                    } catch (error) {
                        value = error;
                        state = "fail";
                    }
                }

                this.switchTo(state, value);
            }
        },

        resolve: function (value) {
            this.transit("done", value);
        },

        reject: function (value) {
            this.transit("fail", value);
        },

        then: function (done, fail) {
            var then = {
                next: new Deferred(),
                done: done,
                fail: fail
            }

            this.thens.push(then);

            if (this.state)
                this.asap();

            return then.next.promise;
        }
    }

    return function () {
        return new Deferred();
    }
})();

var promisesAplusTests = require("promises-aplus-tests");

promisesAplusTests({
    deferred: deferred
}, {
    reporter: "spec"
});

@domenic

This comment has been minimized.

Show comment
Hide comment
@domenic

domenic Apr 28, 2014

Member

I don't understand the state === "done" check enough to comment, really.

Member

domenic commented Apr 28, 2014

I don't understand the state === "done" check enough to comment, really.

@briancavalier

This comment has been minimized.

Show comment
Hide comment
@briancavalier

briancavalier Apr 28, 2014

Member

@Ziriax It will probably be hard for us to comment, since we don't understand your code nearly as well as you do. If you could provide some sort of minimal test case that shows a potential problem, such as a particular test not matching a part of the Promises/A+ spec, that'd be extremely helpful.

Member

briancavalier commented Apr 28, 2014

@Ziriax It will probably be hard for us to comment, since we don't understand your code nearly as well as you do. If you could provide some sort of minimal test case that shows a potential problem, such as a particular test not matching a part of the Promises/A+ spec, that'd be extremely helpful.

@Ziriax

This comment has been minimized.

Show comment
Hide comment
@Ziriax

Ziriax Apr 28, 2014

Sorry for not making my question clear enough. I don't understand my own code that much either ;-) I tweaked it until all the specs passed, some of them were really hard to get right.

Let me try to explain my question in detail, with an example.

Consider the following code:

var def1 = deferred();
var prom1 = def1.promise;

var def2 = deferred();
var prom2 = def2.promise;

prom1.then(function (x) {
    console.log("done:" + x);
}, function (x) {
    console.log("fail:" + x);
});

def1.reject(prom2);
def2.resolve("foo");

What should be the correct output?

If I keep my promise implementation as initially pasted, I get the output:

fail:[object Object]

So prom1 is rejected with prom2 as-is, not doing any further resolution.

However, if I modify my transit method, removing the state === "done" check, I get the output

done: foo

In this case prom1 is rejected with prom2, but prom1 adopts the state of prom2, so eventually gets fulfilled...

Only one of the two behaviors can be correct, but I don't know which one.

So this means that I have two different implementations, both of them passing all the tests, but with different behavior.

The test suite doesn't seem to dictate the correct behavior, or at least not in the context of my own simple promise implementation.

What confused me in the A+ spec was

2.3.3.3.2: If/when rejectPromise is called with a reason r, reject promise with r.

It's not clear to me what to do when the reason r is another promise?

Thanks a lot,
Peter Verswyvelen

Ziriax commented Apr 28, 2014

Sorry for not making my question clear enough. I don't understand my own code that much either ;-) I tweaked it until all the specs passed, some of them were really hard to get right.

Let me try to explain my question in detail, with an example.

Consider the following code:

var def1 = deferred();
var prom1 = def1.promise;

var def2 = deferred();
var prom2 = def2.promise;

prom1.then(function (x) {
    console.log("done:" + x);
}, function (x) {
    console.log("fail:" + x);
});

def1.reject(prom2);
def2.resolve("foo");

What should be the correct output?

If I keep my promise implementation as initially pasted, I get the output:

fail:[object Object]

So prom1 is rejected with prom2 as-is, not doing any further resolution.

However, if I modify my transit method, removing the state === "done" check, I get the output

done: foo

In this case prom1 is rejected with prom2, but prom1 adopts the state of prom2, so eventually gets fulfilled...

Only one of the two behaviors can be correct, but I don't know which one.

So this means that I have two different implementations, both of them passing all the tests, but with different behavior.

The test suite doesn't seem to dictate the correct behavior, or at least not in the context of my own simple promise implementation.

What confused me in the A+ spec was

2.3.3.3.2: If/when rejectPromise is called with a reason r, reject promise with r.

It's not clear to me what to do when the reason r is another promise?

Thanks a lot,
Peter Verswyvelen

@domenic

This comment has been minimized.

Show comment
Hide comment
@domenic

domenic Apr 28, 2014

Member

So this means that I have two different implementations, both of them passing all the tests, but with different behavior.

That means you found a bug in the test coverage; nice!

It's not clear to me what to do when the reason r is another promise?

You should follow the spec, and reject promise with r.

Member

domenic commented Apr 28, 2014

So this means that I have two different implementations, both of them passing all the tests, but with different behavior.

That means you found a bug in the test coverage; nice!

It's not clear to me what to do when the reason r is another promise?

You should follow the spec, and reject promise with r.

@briancavalier

This comment has been minimized.

Show comment
Hide comment
@briancavalier

briancavalier Apr 28, 2014

Member

It's not clear to me what to do when the reason r is another promise?

Reject is verbatim. IOW, you shouldn't attempt to resolve r, but rather use r as the reason verbatim, even if r is a promise.

Member

briancavalier commented Apr 28, 2014

It's not clear to me what to do when the reason r is another promise?

Reject is verbatim. IOW, you shouldn't attempt to resolve r, but rather use r as the reason verbatim, even if r is a promise.

@briancavalier

This comment has been minimized.

Show comment
Hide comment
@briancavalier

briancavalier Apr 28, 2014

Member

jinx again! :)

Member

briancavalier commented Apr 28, 2014

jinx again! :)

@juandopazo

This comment has been minimized.

Show comment
Hide comment
@juandopazo

juandopazo Apr 29, 2014

Nice catch!

juandopazo commented Apr 29, 2014

Nice catch!

@domenic domenic changed the title from Troubled by my own 'fully compliant' promise implementation to Rejecting with a promise is not fully tested Jul 14, 2014

@domenic domenic added the undertested label Jul 14, 2014

xiaody pushed a commit to xiaody/in-promise that referenced this issue Jul 13, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment