search

meteor / promise

ES6 Promise polyfill with Fiber support
MIT License
63 stars 15 forks source link

Prevent infinite loop in newer versions of Node (#18). #19

Closed corwin-of-amber closed 7 years ago

corwin-of-amber commented 7 years ago

An attempt to solve an issue with Node 7 and apparently also 6. Not thoroughly tested.

benjamn commented 7 years ago

After thinking about this some more, I think it might be going a little too far, since it means that no callback functions passed to fiberPool.run(...).then will be wrapped.

That includes the native resolver functions that are causing the problems, which is why this PR solves those problems, but it also includes promises returned by Promise.asyncApply, which is used for server-side async/await support in Meteor. I think this might be the source of your occasional out-of-fiber warnings, but I'm not sure.

Your comment in #18 inspired me to see if we could avoid wrapping the native resolver functions:

The fix from pull request #11 does not help in this case because they are created afresh every time.

While this is a true statement, I think there's another way to detect them (along with other native functions): https://github.com/meteor/promise/commit/3ab6dcba8159a70008db4e7f19dec5f81285c99a

I'm confident that native functions never need to run in a Fiber. I'm somewhat less confident they never need the dynamics from the Fiber that spawned them, since they could potentially call our version of .then, which calls cloneFiberOwnProperties(Fiber.current) to capture the dynamic state of the current Fiber.

That functionality would have been lost with the _meteorPromiseAlreadyWrapped marking strategy, too, and the tests of dynamics are passing, so I'm not too worried.

corwin-of-amber commented 7 years ago

I agree that preventing all fiberPool.run(...).then calls from spawning fibers may be too general. But if this is the case, why not simply mark just the one call site that is inside wrapCallback, right before returning it? Testing for a native function looks like it is... well, a matter of taste. It uses a proxy for the problem rather than addressing the problem itself (which may be ok).

Regarding dynamics: I have no idea what they mean. I see that you have removed them from the call to wrapCallback, so I am just assuming they are not important.

benjamn commented 7 years ago

why not simply mark just the one call site that is inside wrapCallback, right before returning it?

Can you make that change, and rebase this PR?

corwin-of-amber commented 7 years ago

Can you make that change, and rebase this PR?

Done. It does indeed seem that more t()s and u()s get wrapped, but at least there is no infinite loop. isNativeFunction is not needed now so you can remove it if you merge this.