Sequence of .then() calls with Promise.all

[smikes]

I have cleaned up one of the tests that shows a difference in behavior between NPO and v8 -- since this is the version of v8 which is known not to pass the Promises/A+ suite, I was initially inclined to suspect that this is reacting to a problem in v8. Now I am not so sure, and I would appreciate guidance. (/cc @domenic @allenwb )

Here is the test:

it("should should execute 'then' methods in sequence", function(done) {
    var p1 = Promise.resolve(100),
        p2 = Promise.resolve(200);

    var sequencer = [1];

    // Point "A"

    p1.then(function afterOne(resolved) {
        assert.deepEqual([1], sequencer);
        sequencer.push(2);
    }).catch(done);

    // Point "B"

    Promise.all([p1, p2]).then(function afterAll(resolved) {
        assert.deepEqual([1, 2], sequencer);
        sequencer.push(3);
    }).catch(done);

    // Point "C"

    p2.then(function afterTwo(resolved) {
        assert.deepEqual([1,2,3], sequencer);
        sequencer.push(4);
    }).then(done).catch(done);

    // Point "D"
});

My expectation is that the three functions passed to then will be enqueued and eventually executed in sequence: ( afterOne, afterAll, afterTwo ). However, I am concerned that my expectations are coloring my reasoning as I read the spec.

According to 25.4.4.1 step 9.o all invokes then on each of the promises in its argument. (Technically there's an intermediate step where Promise.resolve is called on each argument, but since p1 and p2 were constructed by the same Promise constructor as Promise.all, Promise.resolve just returns them without constructing new promises.)

According to 25.4.4.3 step 12, when then is called on a promise in the state "fulfilled", a task to call its onFulfilled handlers is enqueued immediately.

Both p1 and p2 are constructed by Promise.resolve() so they begin in the "fulfilled" state.

I would expect the state of the "PromiseTasks" queue to evolve as follows:

   Point A: []   // empty queue
   Point B: [ afterOne ]
   Point C: [ afterOne, resolveElement1, resolveElement2 ]
   Point D: [ afterOne, resolveElement1, resolveElement2, afterTwo ]

After control flow leaves the test function, I would expect tasks to be consumed from the "PromiseTasks" queue in FIFO order. The question is, what happens when resolveElement2 finishes executing?

According to 25.4.4.1.1 step 10, when remainingElementsCount.[[value]] has dropped to zero, the parent Promise created by Promise.all should have its promiseCapability.[[Resolve]] function called. This will eventually trigger a call to the only function in its onFulfilled slot, namely afterAll.

The question is, can resolveElement2 synchronously call afterAll (since it is running in the PromisesTask handler, not from user code) or must it enqueue the call to afterAll? This is detectable by user code as above, where the numbers pushed to sequencer distinguish between the call order of (afterOne, afterAll, afterTwo) vs. (afterOne, afterTwo, afterAll)

I started writing this thinking that my test was correct, and afterAll should be called before afterTwo. Midway through writing this, I flip-flopped and started to think that afterTwo should be called before afterAll. Now I am merely confused.

Comments very much appreciated.

[domenic]

The question is, can resolveElement2 synchronously call afterAll (since it is running in the PromisesTask handler, not from user code) or must it enqueue the call to afterAll?

You can never synchronously call a promise handler; they are always called via TriggerPromiseReactions which enqueues tasks to call them.

[smikes]

Just to make that clear, you're saying that when resolveElement2 discovers that the the parent promise is resolvable and calls [[Resolve]], this will cause a call to afterAll to be enqueued, and the correct observed call sequence should be (afterOne, afterTwo, afterAll)

And that this sequence is required by the spec.

(edited to add: I'm not trying to be pedantic, I just want to make sure I have the observables right, because my reasoning could be in error about the other things -- including whether a synchronous call is what causes this.)

[domenic]

Yes. You can also check against the reference implementation, if that would help.

[smikes]

Thanks.

I can see that the sequencing behavior of v8 faithfully reproduces the sequencing behavior of the reference implementation. Changed the tests to reflect expected sequencing behavior.

[getify]

So the conclusion is that NPO has a bug in Promise. all, correct?

[smikes]

Yes, and also Promise.race.

I think it's actually the scheduler, but I haven't gone into the code yet.

[getify]

I'll jump on it.

[getify]

should be fixed now I think. I was being too aggressive with perf optimizations ☺

[smikes]

That causes the Promise.all tests to pass, but not the Promise.race tests. I glanced at the NPO code and didn't see the problem, so my natural inclination is to blame the tests. Will do some more checking.

[getify]

The test I used for race(..) was to take the test for all(..) you provided and just swap in race(..). If that's not the same test, can you please let me know what's different and I can check it out to see?

[smikes]

Hm.. what version of node are you running? I just noticed that for NPO the race(...) tests succeed in 0.11.13 and fail in 0.10.29, which is not a happy result. I am only able to test v8 Promises using node 0.11.13.

This is the command I am using to run the race tests:
node test/cli.js test_adapter.js --grep "25.4.4.3 Promise.race with 2-element array"

And the result:

12 passing (86ms)
1 pending
2 failing

1) 25.4.4.3 Promise.race with 2-element array should fulfill immediately with first fulfilled promise in array:
  Error: done() invoked with non-Error: 2
  at /home/smikes/src/github/native-promise-only/node_modules/mocha/lib/runnable.js:198:38
  at notifyIsolated (/home/smikes/src/github/native-promise-only/lib/npo.src.js:120:19)
  at MakeDef.notify (/home/smikes/src/github/native-promise-only/lib/npo.src.js:106:4)
  at schedule (/home/smikes/src/github/native-promise-only/lib/npo.src.js:72:7)
  at MakeDef.reject (/home/smikes/src/github/native-promise-only/lib/npo.src.js:188:3)
  at Object.publicReject [as reject] (/home/smikes/src/github/native-promise-only/lib/npo.src.js:277:13)
  at notifyIsolated (/home/smikes/src/github/native-promise-only/lib/npo.src.js:116:11)
  at MakeDef.notify (/home/smikes/src/github/native-promise-only/lib/npo.src.js:106:4)
  at schedule (/home/smikes/src/github/native-promise-only/lib/npo.src.js:72:7)
  at MakeDef.reject (/home/smikes/src/github/native-promise-only/lib/npo.src.js:188:3)

2) 25.4.4.3 Promise.race with 2-element array should reject immediately when second rejects:
 Error: Unexpected resolve 1
  at /home/smikes/src/github/native-promise-only/test/tests/race.js:151:10
  at notifyIsolated (/home/smikes/src/github/native-promise-only/lib/npo.src.js:120:19)
  at MakeDef.notify (/home/smikes/src/github/native-promise-only/lib/npo.src.js:106:4)
  at Object.drain [as _onImmediate] (/home/smikes/src/github/native-promise-only/lib/npo.src.js:60:11)
  at processImmediate [as _immediateCallback] (timers.js:336:15)

[getify]

NPO doesn't run in 0.11.13, because it's a polyfill, and in 0.11, Promise already exists, so it just skips over itself. So I believe you're probably testing the native Promise.

I am running 10.29, and my race(..) test passes. I haven't looked at your branch yet. Can you past a minimal breaking test in 10.29 here so I can diagnose?

[smikes]

Oh. Hahahaha. Of course.

I will create a small breaking test.

[smikes]

Confirmed that node version doesn't matter by modifying the test_adapter to remove global.Promise before loading NPO. I am surprised that node 0.11 has global.Promise even if I don't pass --harmony on the command line.

Anyway, a failing test:

it("should reject immediately when second rejects", function(done) {
    var resolveP1, rejectP2,
        p1 = new Promise(function(resolve, reject) {
            resolveP1 = resolve;
        }),
        p2 = new Promise(function(resolve, reject) {
            rejectP2 = reject;
        });

    Promise.race([p1, p2]).then(function(resolved) {
        throw new Error("Unexpected resolve " + resolved);
    }, function(rejected) {
        assert.equal(rejected, 2);
    }).then(done).catch(done);

    rejectP2(2);
    resolveP1(1);
});

[smikes]

And the other failing test:

it("should fulfill immediately with first fulfilled promise in array", function(done) {
    var resolveP1, rejectP2,
        p1 = new Promise(function(resolve, reject) {
            resolveP1 = resolve;
        }),
        p2 = new Promise(function(resolve, reject) {
            rejectP2 = reject;
        });

    rejectP2(2);
    resolveP1(1);

    Promise.race([p1, p2]).then(function(resolved) {
        assert.equal(resolved, 1);
    }).then(done).catch(done);
});

[getify]

@smikes

See promises-aplus/promises-tests#61

Notice that I reduced the test cases to actually have nothing to do with all(..) and race(..). It's actually about scheduling sequencing between different promises, which is a very interesting (and troubling) case indeed.

Based on my analysis, I believe your first test case is correct (which I've diagnosed in NPO and have a fix for), but I believe your second test case is incorrect.

If I'm correct, and your second test should be 2 instead of 1, that means that this has actually found another bug in v8, because v8 results in 1.

Moreover, that linked issue thread raises the question that these are bugs which should have tests in promises/a+, because I believe that spec (and its test suite) should handle and enforce implied inter-promise scheduling semantics, not just intra-promise semantics.

So, your all(..) and race(..) tests, which are _also_ checking sequencing semantics, may or may not be appropriate for this test suite, depending on whether or not promises/a+ adds tests to enforce inter-promise sequencing. OTOH, they may be useful only for testing all(..) and race(..) independently of any promise sequencing. Not really sure. My brain hurts now.

[update: my bad, see message below]

[smikes]

My brain hurts now.

Neuralgia is a known side effect of open-source developmen

I will port your simplified tests to @domenic 's reference implementation and note the results.

[getify]

OK, so both @smikes' test cases were correct, and I was misunderstanding how to think about scheduling vs. resolution/rejection. I have corrected NPO in v0.7.0-a to use the correct scheduling behavior (at least, I believe), and it passes both tests as shown above.

[smikes]

Ah, schneaky. The "race" was between scheduling the execution and subsequently adding a handler to the execution chain.

[getify]

I do think we should have tests of the sequencing (like my simplified versions) and tests with race(..) and all(..) (like your versions). It would trouble me if we made assumptions about one set covering the other or vice versa.

[smikes]

Agreed. I can add those easily. They exercise the scheduler directly; while the race(...) test depends on scheduling, formally what it's testing is the mechanism of single settlement on an aggregate promise.