Infinite loop interaction between q.js and when.js #106

Closed
briancavalier opened this Issue Aug 10, 2012 · 11 comments

Comments

Projects
None yet
4 participants
@briancavalier

I ran into this while using volo, which uses q, with when.js's when.map() to "volo add" (fetch from github and install) a bunch of AMD dependencies. I narrowed it down to an infinite loop that seems to be cause by an interaction between q's when() and/or promiseSend implementation and when.js's internal _reduce(), but I haven't been able to nail it down further than that.

Here's a fairly minimal test case that reproduces the problem using node v0.6.14 on OS X 10.8--not sure yet if it happens in other environments, but given that it seems to be related to when.js and q level code, I'm pretty sure it would. Interestingly, using when.js's delay() instead of q's seems to work ok.

Maybe we can put our heads together and figure out what's going on here. Thanks!

@briancavalier

This comment has been minimized.

Show comment Hide comment
@briancavalier

briancavalier Aug 10, 2012

I noticed that master is ahead of what's in npm (specifically that the double-when message in q.when() was removed), so I tried npm installing directly from the github url, and retried the test. I still get the infinite loop and CPU/mem spike.

I haven't dug any deeper yet, but I may this weekend. Any insight you guys have would be much appreciated.

I noticed that master is ahead of what's in npm (specifically that the double-when message in q.when() was removed), so I tried npm installing directly from the github url, and retried the test. I still get the infinite loop and CPU/mem spike.

I haven't dug any deeper yet, but I may this weekend. Any insight you guys have would be much appreciated.

@domenic

This comment has been minimized.

Show comment Hide comment
@domenic

domenic Aug 10, 2012

Collaborator

No real ideas off the top of my head, but FWIW this is definitely on my weekend to-do list.

Collaborator

domenic commented Aug 10, 2012

No real ideas off the top of my head, but FWIW this is definitely on my weekend to-do list.

@briancavalier

This comment has been minimized.

Show comment Hide comment
@briancavalier

briancavalier Aug 10, 2012

Thanks, Domenic.

Thanks, Domenic.

@briancavalier

This comment has been minimized.

Show comment Hide comment
@briancavalier

briancavalier Aug 13, 2012

I still don't know what's going on, but I was able to trim down the test case. Now it shows the 4 possible combinations of .then() chaining using when.resolve and q.resolve. It seems to be some sort of weird interaction between the two since q + q works, and when + when works, but not when + q or q + when.

I still don't know what's going on, but I was able to trim down the test case. Now it shows the 4 possible combinations of .then() chaining using when.resolve and q.resolve. It seems to be some sort of weird interaction between the two since q + q works, and when + when works, but not when + q or q + when.

@briancavalier

This comment has been minimized.

Show comment Hide comment
@briancavalier

briancavalier Aug 13, 2012

I think I've narrowed it down a bit further. At first, I thought the nextTick() in q.when around 906 was unnecessary ... interestingly, commenting it out avoids the loop. But, after running Q's unit tests and looking more at the code, I see why it's there.

So, I tried a different angle. Here's another test case where it seems like using q.when() and deferred.promise.promiseSend() should produce equivalent behavior, but they done. I don't know if that assumption is valid, though. I tried some other combinations (using when.js's defer or when(), etc. etc.), but the loop seems to happen when I use q.when().

@domenic, have you found any clues?

I think I've narrowed it down a bit further. At first, I thought the nextTick() in q.when around 906 was unnecessary ... interestingly, commenting it out avoids the loop. But, after running Q's unit tests and looking more at the code, I see why it's there.

So, I tried a different angle. Here's another test case where it seems like using q.when() and deferred.promise.promiseSend() should produce equivalent behavior, but they done. I don't know if that assumption is valid, though. I tried some other combinations (using when.js's defer or when(), etc. etc.), but the loop seems to happen when I use q.when().

@domenic, have you found any clues?

@briancavalier

This comment has been minimized.

Show comment Hide comment
@briancavalier

briancavalier Aug 13, 2012

Oops, forgot the test case link, added above as well

Oops, forgot the test case link, added above as well

@kriskowal

This comment has been minimized.

Show comment Hide comment
@kriskowal

kriskowal Aug 23, 2012

Owner

@briancavalier I’ve reproduced and came to the same conclusion. There is a cycle where Q and When are taking turns coercing eachother’s promises with their resolve methods. As part of this process, they both call .then on the other.

when can break the cycle by attempting to normalize the promise to a fulfilled value with promise.valueOf(). I’ll send a patch. I imagine Q could do something similar, but I don’t see an implementation of valueOf or nearer.

Owner

kriskowal commented Aug 23, 2012

@briancavalier I’ve reproduced and came to the same conclusion. There is a cycle where Q and When are taking turns coercing eachother’s promises with their resolve methods. As part of this process, they both call .then on the other.

when can break the cycle by attempting to normalize the promise to a fulfilled value with promise.valueOf(). I’ll send a patch. I imagine Q could do something similar, but I don’t see an implementation of valueOf or nearer.

@domenic

This comment has been minimized.

Show comment Hide comment
@domenic

domenic Aug 23, 2012

Collaborator

Does this mean Promises/A implementations cannot interop without valueOf/nearer implementations?

On Aug 23, 2012, at 19:37, "Kris Kowal" <notifications@github.commailto:notifications@github.com> wrote:

@briancavalierhttps://github.com/briancavalier I’ve reproduced and came to the same conclusion. There is a cycle where Q and When are taking turns coercing eachother’s promises with their resolve methods. As part of this process, they both call .then on the other.

when can break the cycle by attempting to normalize the promise to a fulfilled value with promise.valueOf(). I’ll send a patch. I imagine Q could do something similar, but I don’t see an implementation of valueOf or nearer.


Reply to this email directly or view it on GitHubhttps://github.com/kriskowal/q/issues/106#issuecomment-7987962.

Collaborator

domenic commented Aug 23, 2012

Does this mean Promises/A implementations cannot interop without valueOf/nearer implementations?

On Aug 23, 2012, at 19:37, "Kris Kowal" <notifications@github.commailto:notifications@github.com> wrote:

@briancavalierhttps://github.com/briancavalier I’ve reproduced and came to the same conclusion. There is a cycle where Q and When are taking turns coercing eachother’s promises with their resolve methods. As part of this process, they both call .then on the other.

when can break the cycle by attempting to normalize the promise to a fulfilled value with promise.valueOf(). I’ll send a patch. I imagine Q could do something similar, but I don’t see an implementation of valueOf or nearer.


Reply to this email directly or view it on GitHubhttps://github.com/kriskowal/q/issues/106#issuecomment-7987962.

@kriskowal

This comment has been minimized.

Show comment Hide comment
@kriskowal

kriskowal Aug 23, 2012

Owner

@domenic I suspect so. I’m favoring valueOf since the default implementation is harmless.

Owner

kriskowal commented Aug 23, 2012

@domenic I suspect so. I’m favoring valueOf since the default implementation is harmless.

@kriskowal kriskowal closed this in facbb77 Aug 23, 2012

@briancavalier

This comment has been minimized.

Show comment Hide comment
@briancavalier

briancavalier Aug 24, 2012

Thanks, Kris, I really appreciate it. I'll try out the patch tonight with the volo use case where I first encountered the problem. If all goes well, I'll do a new release tomorrow.

I've actually resisted implementing valueOf (or nearer) because I felt it encourages people, especially those new to promises, to poll instead of observing with when/then. This might be a reason to implement it.

Interestingly, I haven't run into this problem when dealing with other promise impls (I've used when pretty extensively with Dojo Deferred pre-1.8 and, gasp, $.Deferred, among a few others). Those implementations don't coerce/assimilate (Dojo 1.8 does), though, which is probably the reason it all works out.

So, it seems like Promises/A can interoperate without valueOf, but perhaps only if there's at least one implementation in the mix that doesn't assimilate in order to break the cycle?

I'll certainly give more thought to adding valueOf to when.js.

Thanks, Kris, I really appreciate it. I'll try out the patch tonight with the volo use case where I first encountered the problem. If all goes well, I'll do a new release tomorrow.

I've actually resisted implementing valueOf (or nearer) because I felt it encourages people, especially those new to promises, to poll instead of observing with when/then. This might be a reason to implement it.

Interestingly, I haven't run into this problem when dealing with other promise impls (I've used when pretty extensively with Dojo Deferred pre-1.8 and, gasp, $.Deferred, among a few others). Those implementations don't coerce/assimilate (Dojo 1.8 does), though, which is probably the reason it all works out.

So, it seems like Promises/A can interoperate without valueOf, but perhaps only if there's at least one implementation in the mix that doesn't assimilate in order to break the cycle?

I'll certainly give more thought to adding valueOf to when.js.

@ForbesLindesay

This comment has been minimized.

Show comment Hide comment
@ForbesLindesay

ForbesLindesay Aug 24, 2012

Collaborator

If you force non-valueOf methods to resolve in the next turn of the event loop that can be very expensive when promises are resolved synchronously. I like to build as much as possible of my libraries to be OK with being given promises, so often they're given resolved promises, or plain old values.

Collaborator

ForbesLindesay commented Aug 24, 2012

If you force non-valueOf methods to resolve in the next turn of the event loop that can be very expensive when promises are resolved synchronously. I like to build as much as possible of my libraries to be OK with being given promises, so often they're given resolved promises, or plain old values.

campadrenalin pushed a commit to DJDNS/when.js that referenced this issue Aug 9, 2014

Fix infinite coercion with Q
Some promises, particularly Q promises, provide a valueOf method that
attempts to synchronously return the fulfilled value of the promise, or
returns the unresolved promise itself.  Attempting to break a fulfillment
value out of a promise appears to be necessary to break cycles between
Q and When attempting to coerce each-other's promises in an infinite loop.
For promises that do not implement "valueOf", the Object#valueOf is harmless.
See: kriskowal/q#106
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment