Fix double resolving of misbehaving promises that returned rejections.

[ForbesLindesay]

This is intended to fix issue #75. It'll need someone to code review and make sure I've done this right, but it seems to work and make sense, providing I've correctly understood the intended behaviour.

[domenic]

Any chance of getting a test with this? Not sure if it should go in spec/q-spec.js or test/ or both though... @kriskowal?

[ForbesLindesay]

As I understand it (from reading the code, not from seeing any spec) when you call promise.promiseSend('when',onResolved, onRejected) it gives your implementation a callback to call when there's an error, which would trigger the bottom of the 3 cases, but if you return a promise which resolves to an error, it calls both the top two cases (before my fix)

[kriskowal]

@ForbesLindesay This looks like a good fix to me.

Let’s make tests to verify the progression.

@domenic I would like to move in the direction of using Jasmine and eventually removing the previous CommonJS testing scaffold. However, we can’t do that until continuous integration with Travis on Node makes use of the Jasmine specs. Maybe we can even use PhantomJS with Travis to do continuous integration for WebKit.

[danfuzz]

Since this rejection function and the other rejection function are now identical, does it make sense to refactor it such that it's just passing (say) a direct reference to a suitably-modified _rejected?

[kriskowal]

You have prompted me to wonder why I’m sending nested when messages at all. All the tests pass with a single layer.

Rather than factor out the common rejection, I think I will factor out the nested "when".

Good eye.

[ForbesLindesay]

I have to confess to being confused, but without the nested when, it would've 'resolved' to a rejected promise in @danfuzz 's example (see issue #75)?

[danfuzz]

Yeah, I am similarly confused. I've been poking and prodding at the code today to try to really get what's going on. Still not there…

[danfuzz]

Hey there. I've been trying to work my way through this code a bit more, and I've begun to wonder if the if (done) checks, while necessary to guarantee the desired property at the level of Q.when()'s implementation, are also hiding a bug or deficiency in the implementation of makePromise.promiseSend(). What I'm seeing is that method will always make an extra resolved() call in cases where the callback it calls issues a rejection.

For example, take this code:

var rej = Q.reject(new Error("umm"));
Q.when(rej, function () {}, function () {});

The call to Q.when() causes (on q.js line 875) a call to be made to makePromise.promiseSend(), passing it two function arguments.

makePromise.promiseSend() (on line 545) ends up calling the reject()'s when handler (line 683), which (a) calls the rejector it was handed and (b) returns whatever that rejector returns (in this case undefined). The rejector will be the second function argument that was passed in back on line 875.

Back in makePromise.promiseSend() the result of that call (that is, undefined) gets saved as result which then (line 553) becomes the argument to a call to the resolved argument. This resolver will be the first function argument passed in back on line 875. It is at this point that the if (done) check prevents any more work from being done.

In the original bug I filed (issue #75), a similar pattern of double calls ends up happening, except on the functions passed into the inner promiseSend() of the when() implementation.

What I'm wondering is if makePromise.promiseSend() should have known better than to call both of its argument functions. As I said, it's clear that the if (done) checks are handy to help prevent external abuse, but what's happening here is a purely internal call pattern.

Thanks again. I'm learning a lot from all this.

[domenic]

@kriskowal Want to take another look at this area?

[ForbesLindesay]

Still hadn't been resolved as far as I know? It doesn't directly affect me, but we should probably look to get this fixed.

[domenic]

@danfuzz After taking some time to dig in to this, I think you are right that makePromise.promiseSend should not be calling resolve unconditionally. Have you had time to hack on the source long enough to make that work?

[domenic]

OK so I basically merged this fix. But I still think there is some crazy stuff going on inside the when and promiseSend implementations.

I tried to remove a nesting level as per Kris's earlier comment but that broke all the tests. I wonder if fc2ba18 was the reason they were succeeding for Kris.

Anyway, @kriskowal, @ForbesLindesay, and @danfuzz, would love to hear your thoughts on the deeper issues and possible fixes. For now though a bugfix seemed prudent.

[danfuzz]

@domenic I'm afraid the last research I did on the topic was a couple of months ago, with the results posted here.

[ForbesLindesay]

beyond working out that this change would fix it, I'm not really that sure what's going on, I think some work may need to go into refactoring at some point :s