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

Node v0.8 - problem on handeling errors #12

Closed
wants to merge 2 commits into from
Closed

Node v0.8 - problem on handeling errors #12

wants to merge 2 commits into from

Conversation

rkatic
Copy link
Collaborator

@rkatic rkatic commented Jul 24, 2013

This seems to make tests pass in Node v0.8 too.

However, at the moment, I am not convinced this is a really safe fix.

Opinions?

@domenic
Copy link
Collaborator

domenic commented Jul 24, 2013

Hmm, I believe what this does is make the error "escape" the domain, since at the time process was created, no domain was active. (Event emitters are always associated to the domain that was active when they were created.)

I guess that's what you're trying to do already, by nulling out the current domain. But presumably the semantics are different between the 0.8 and 0.10 version of domains. I am not sure exactly how.

Very interesting. I am tentatively +1. You could also try removing the null-out-and-restore-the-active-domain code, which should in theory work in combination with this approach. If we can get rid of that, it seems like a pretty nice win.

The one downside is the stack trace is now even less useful, but not by that much.

@rkatic
Copy link
Collaborator Author

rkatic commented Jul 25, 2013

Hmm, I believe what this does is make the error "escape" the domain, since at the time process was created, no domain was active. (Event emitters are always associated to the domain that was active when they were created.)

Not in this case. process is the exception to the rule.

I guess that's what you're trying to do already, by nulling out the current domain. But presumably the semantics are different between the 0.8 and 0.10 version of domains. I am not sure exactly how.

Negative. Not trying to null current domain on thrown exception, only on scheduling flushing since flush calls should not interfere with domains of tasks.
Still not sure about differences between the 0.8 and 0.10 version, but domain.enter() and domain.exits() seems have not been changed.

Very interesting. I am tentatively +1. You could also try removing the null-out-and-restore-the-active-domain code, which should in theory work in combination with this approach. If we can get rid of that, it seems like a pretty nice win.

As I said above, this fix is not just another approach to do what null-out-and-restore-the-active-domain code does.

@rkatic
Copy link
Collaborator Author

rkatic commented Jul 25, 2013

Still not sure why this fix works, and I am not comfortable with it. Also this will probably easily break after process.on("error", function () {...}).
Will investigate further, but probably not in next days.

@rkatic rkatic mentioned this pull request Jul 27, 2013
@rkatic
Copy link
Collaborator Author

rkatic commented Jul 27, 2013

See #13.

@rkatic rkatic closed this Jul 27, 2013
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

Successfully merging this pull request may close these issues.

2 participants