Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

isNodeJS check #645

Closed
domarmstrong opened this issue Feb 13, 2015 · 10 comments
Closed

isNodeJS check #645

domarmstrong opened this issue Feb 13, 2015 · 10 comments

Comments

@domarmstrong
Copy link

Hi, in asap.js you are checking whether the environment isNodeJS.

if (typeof process !== "undefined" && process.nextTick) {
    // Node.js before 0.9. Note that some fake-Node environments, like the
    // Mocha test runner, introduce a `process` global without a `nextTick`.
    isNodeJS = true;
}

We do have a fake environment with browserify where process.nextTick is used and this check is causing errors to be thrown synchronously causing issues.

One suggestion from ksmeltzer was to use process.title === 'node'. The problem is we have window in both envs and process etc..

@ghost
Copy link

ghost commented Feb 13, 2015

Or Object.prototype.toString.call(process) == '[object process]'...

@kriskowal
Copy link
Owner

Do these environments also provide a bad process.nextTick?

@kriskowal
Copy link
Owner

Come back and re-open this if process.nextTick is bad on these fake Node environments. The isNode test just means that Q will use process.nextTick and check whether process.domain exists when necessary. It should not have any harmful side effects.

@domarmstrong
Copy link
Author

The problem is this in flush:

           try {
                task();

            } catch (e) {
                if (isNodeJS) {
                    // In node, uncaught exceptions are considered fatal errors.
                    // Re-throw them synchronously to interrupt flushing!

                    // Ensure continuation if the uncaught exception is suppressed
                    // listening "uncaughtException" events (as domains does).
                    // Continue in next event to avoid tick recursion.
                    if (domain) {
                        domain.exit();
                    }
                    setTimeout(flush, 0);
                    if (domain) {
                        domain.enter();
                    }

                    throw e;

                } else {
                    // In browsers, uncaught exceptions are not fatal.
                    // Re-throw them asynchronously to avoid slow-downs.
                    setTimeout(function() {
                       throw e;
                    }, 0);
                }
            }

It thinks that the browser isNodeJS and is therefore throwing errors synchronously instead of async as it would otherwise and flush is not completing.

@slorber
Copy link

slorber commented Mar 24, 2015

@kriskowal as mentionned in the linked issue I got a problem with this too.
In case of some error, Q completely stops working, producing important problems in production (SPA needing F5 refresh, as not a single promise can resolve anymore)

@dominataa is right, the problem seems to be related to the synchronous exception rethrow in case of isNodeJs = true, in case of a Browserify context.

This would be nice to be able to disable the "fail fast flushing behavior" used for NodeJS (In node, uncaught exceptions are considered fatal errors. Re-throw them synchronously to interrupt flushing!).

@slorber
Copy link

slorber commented Mar 24, 2015

This problem appeared between releases Browserify 8.0.3 and 8.1.0. Q works fine with older versions of Browserify

@slorber
Copy link

slorber commented Mar 24, 2015

I've made a PR for node-process/browserify that solves this problem.
See defunctzombie/node-process#37

@calvinmetcalf
Copy link

can confirm Q does some funky stuff in browserify because it thinks it's in node

@pho3nixf1re
Copy link

This happens with Webpack as well. It's killing our test suite and causing early exits instead of just test failures. It is definitely a bug with the flush in Q and not with the shim's in browserify and webpack.

@kriskowal
Copy link
Owner

This has been addressed by @kahnjw in v1.2.1. Thanks for the tips.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants