track active domain for nextTick for node.js 0.10.xx

[xaka]

No description provided.

[benjamingr]

Promises make domains worthless anyway.

[xaka]

I'd not say so. Right now libraries like knex do not work as expected in
some situations because of that issue with nextTick and I don't know when
the fix will land in next 0.10.xx so we have to do at least something.

BTW. Why do you say worthless?
On Feb 28, 2014 9:00 AM, "benjamingr" [email protected] wrote:

Promises make domains worthless anyway.

Reply to this email directly or view it on GitHubhttps://github.com//pull/124#issuecomment-36371430
.

[benjamingr]

See #51

[tokafish]

Although promises do a better job of error handling than domains, you might be using domains for other purposes. For instance, using them for a request context.

Are there any disadvantages to using setTimeout instead of nextTick?

[benjamingr]

Yes, they do completely different things. If anything setImmediate can be used but that's still different - see stackoverflow.com/questions/15349733/setimmediate-vs-nexttick

On 28 בפבר 2014, at 19:31, Thomas Fisher [email protected] wrote:

Although promises do a better job of error handling than domains, you might be using domains for other purposes. For instance, using them for a request context.

Are there any disadvantages to using setTimeout instead of nextTick?

—
Reply to this email directly or view it on GitHub.

[xaka]

@wless1 is right, if i were using bluebird directly, i wouldn't mind stay away from domains, but in my case bluebird sits in the middle of multiple layers of code and 3rd party libraries, and because of that issue with nextTick, layers below bluebird no longer work with layers above it. This PR assures that bluebird plays well with being integrated into the big project. The actual problem is in node.js it self and it's fixed in latest unstable version and probably will be fixed in the next stable version, but for now i strongly believe that it's bluebird and other libraries responsibility to provide a workaround.

[petkaantonov]

Are there any disadvantages to using setTimeout instead of nextTick?

I would easily say performance - setTimeout requires the use of timers.

[benjamingr]

ty

On Sun, Mar 2, 2014 at 7:22 PM, Petka Antonov [email protected]:

Closed #124 #124.

Reply to this email directly or view it on GitHubhttps://github.com//pull/124
.

[xaka]

Why it's been closed? Any comments at least?

[petkaantonov]

The explanation comment was posted right before closing:

Are there any disadvantages to using setTimeout instead of nextTick?

I would easily say performance - setTimeout requires the use of timers.

[xaka]

Well, fair enough, but at the same time we have very interesting situation here. What's worse? Slower library or library that doesn't work at all as soon as you or another library start using domains.
Again, as I mentioned before, the issue with nextTick is fixed in 0.11.xx and will land in 0.12.xx I believe, but until then what options do I have? Domains are part of node.js.

BTW. It's possible to implement the fix for domains in bluebird using nextTick. We just have to track the active domain in schedule.js. Will you consider it as an appropriate solution? It shouldn't affect the performance.

[petkaantonov]

@xaka If you can truly do it without affecting performance, then why not. But as @benjamingr says, domains are useless and people not using them should not pay anything.

[xaka]

@petkaantonov I've updated the patch. Please take a look and let me know what do you think of it. https://github.com/xaka/bluebird/compare/patch-1.

[xaka]

BTW. Domains are very useful for things like http://andreypopp.github.io/domain-context/. It's just the question of size and type of your project.

[petkaantonov]

Looks good to me, just needs to include a test. You can see how previous node-only tests are done by grepping for isNodeJS in test directory.

[xaka]

Could you please re-open PR so i can keep posting the updates. I'll change the title and commit message to not confuse with the initial proposal. Thank you!

P.S. If you want me to open another PR and keep this one closed, i don't mind. It's your call.

[xaka]

@petkaantonov The test has been added. Please take a look when you have a chance.

[benjamingr]

How do the benchmarks look? Does the extra check have any impact when not using domains?

[xaka]

@benjamingr extra check for Node.js version happens only once at compilation time so it doesn't affect the performance at all. Another check for active domain happens at runtime, but as it's very simple IF condition without additional computation, I'd say we've lost only few cycles.

P.S. To all, as you understand, this fix is for 0.10.xx version only, so my question is do we care about any version prior to 0.10.xx?

[oleksiyk]

Is it necessary to call require("domain") each time?

[xaka]

No, there is no reason to do so. I've updated the patch :)

[petkaantonov]

I will publish 1.0.8 later today that includes this patch

[xaka]

@petkaantonov i feel so bad and embarrassed now, but after digging into node.js code and doing more research, i was asked to provide a test case for broken nextTick, and i've found out that situation is even worse. The reality is that nextTick implementation (reference to function) is replaced with another one as soon as you do require('domain') in node.js. What does it mean? You never ever should cache a reference to that function and node.js documentation is pretty silent about it until you get caught. Now, the patch that was provided here is correct, it fixes the actual problem, but it can be better and can be applied to all node.js versions.

The improved fix would not cache nextTick function and call it at the time when schedule is called.

schedule = function schedule(fn) {
  process.nextTick(fn);
}

And there is no need to check for node.js version anymore.

[xaka]

I've updated the patch. You can see the new version if you re-open the issue or go directly to my fork.
I've also sent a PR to node.js to improve the documentation and consider using a wrapper for nextTick so people don't get caught again.

[benjamingr]

@xaka it seems according to your NodeJS pr that this might get fixed soon in Node :) That's really good news.

[xaka]

@benjamingr i'm trying really hard to escalate it :) hehe. Thanks to @tjfontaine who agreed to jump on it vey quickly. When community acts like this it makes me happy.

[xaka]

@benjamingr aaannnddd it's been fixed for 0.10.xx!

[benjamingr]

@xaka awesome!

Does this mean we can revert this change in BB once it lands in a stable .10.xx?

[xaka]

Sure thing! I would repatch it with my recent update though which makes
more sense now, but don't think it's fun to release after release. Not my
call anyway :)

@xaka https://github.com/xaka awesome!

Does this mean we can revert this change in BB once it lands in a stable
.10.xx?

Reply to this email directly or view it on
GitHubhttps://github.com//pull/124#issuecomment-36591680
.

[TimBeyer]

This PR broke browserify.
I made a PR to fix it in #129

[petkaantonov]

@llambda The patch is here 6e02e47

[petkaantonov]

The issue was that a reference was cached instead of loading it over and over again at runtime:

var cached = process.nextTick;
function callCached() {
    //What if process.nextTick has been changed by now?
    //the variable cached will be out of sync
    cached();
}

vs

function callFresh() {
    //What if process.nextTick has been changed by now?
    //It will call the fresh one
    process.nextTick();
}