Sequence of .then() calls with Promise.all about native-promise-only

[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.