Skip to content

Commit

Permalink
Coerce thenables on next tick
Browse files Browse the repository at this point in the history
  • Loading branch information
@kriskowal
kriskowal committed Feb 12, 2013
1 parent e3a927d commit 3f32632
Showing 1 changed file with 7 additions and 1 deletion.
8 changes: 7 additions & 1 deletion q.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -745,7 +745,13 @@ function resolve(value) {
*/
function coerce(promise) {
var deferred = defer();
promise.then(deferred.resolve, deferred.reject, deferred.notify);
nextTick(function () {
try {
promise.then(deferred.resolve, deferred.reject, deferred.notify);
} catch (exception) {
deferred.reject(exception);
}
});
return deferred.promise;
}

Expand Down

7 comments on commit 3f32632

@briancavalier
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting. What's the reason? It seems like trapping the exception in the current tick would be sufficient, since deferred will guarantee next tick for resolution, rejection, and progress updates. It also seems like the call to thenable.then and the prior isPromise check during assimilation should be done in the same tick (as each other) to prevent interleaving code from adding/removing a then method.

@rkatic
Copy link
Collaborator

@rkatic rkatic commented on 3f32632 Feb 25, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, I "feel" that Q(fakeThenable) should throw, and not to return a rejection.
What about moving deferred.resolve in a try..catch (in when) instead?

@domenic
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@briancavalier I am somewhat confused by this myself as well, but @erights insists this is necessary and managed to convince me---I just forgot how. (I think he pointed out the interleaving problem you give as well, however.) @kriskowal probably remembers better.

@rkatic one of the primary use cases for Q assimilation is passing untrusted objects or thenables, and getting back a trusted Q promise. If these untrusted objects were malicious and wanted to crash our program or interrupt the control flow of the current turn by throwing, we should not allow this; we do so by using nextTick and transforming throws into rejections.

@briancavalier
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm, thanks @domenic. If you happen to remember, I'd def be interested to know the reasoning, as it's not obvious to me yet (but I'll keep noodling on it). Maybe @erights or @kriskowal could chime in quickly?

@rkatic
Copy link
Collaborator

@rkatic rkatic commented on 3f32632 Feb 26, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@domenic I am aware that the exception should be caught to propagate exceptions, that's why I am asking what about catching in when, instead on coercing itself.

@briancavalier
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@domenic I think this is why, but maybe there are better/other reasons.

var when = require('../when');

var i = 0;
var p = when({
    then: function(cb) {
        cb(i);
        // vs
        // setTimeout(function() { cb(i); });
    }
});

i++;

p.then(console.log);

@rkatic
Copy link
Collaborator

@rkatic rkatic commented on 3f32632 Feb 26, 2013 via email

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please sign in to comment.