support for domains #163

Merged
merged 1 commit into from Apr 29, 2013

Projects

None yet

2 participants

@erlichmen
Contributor

Support for domains so that async method will preserve the caller process.domain.

@erlichmen erlichmen support for domains
made bind_to_domain private
1769698
@defunctzombie
Collaborator

@ncb000gt Looks good to you? LGTM.

There is a slight code tweak I would make to make this simpler:

var bind_to_domain = function(cb) {
    return (process.domain) ? process.domain.bind(cb) : cb;
};

But I will make that after I merge this.

@defunctzombie defunctzombie merged commit d1576b1 into kelektiv:master Apr 29, 2013

1 check passed

default The Travis build passed
Details
@defunctzombie
Collaborator

After thinking about this some more I have reverted this change. This is not the correct way to support domains. Every library that does callbacks would need to do this. In my opinion this silly and more work for developers with no reason. This is something you should do in your own code or should be handled by node core at lower levels.

I believe there is a MakeCallback thing c/c++ modules are supposed to use if they want to support domains. That would be the proper way to do it and not like this.

@erlichmen
Contributor

I agree with you that its a pain to support domain by every package author (not really everyone, but a lot), but its a bigger pain to support it by EVERY nodejs developer everywhere in your code.

Yes, domains should be more backed into the core framework but until then than I think its more pragmatic to support it in the package level. Such support mean that when it will be finally be backed in you won't have to change your (the package user) code.

@defunctzombie
Collaborator

@erlichmen I do not buy into the way domains are handled right now. There are also a number of undocumented things and various edge cases. Given that this module is very low level and simple, I am hesitant to make such a decision at this time when the presented change could just be implemented in your app.

@erlichmen
Contributor

Your call, and yes that code felt a little out of place. lets hope that domains will be fully supported by the core because they are pretty darn useful.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment