Skip to content

Conversation

@tcr
Copy link
Member

@tcr tcr commented Oct 13, 2014

On master, require('domain') throws an error. This patch

  1. Implements node-libs whitelisting in gyp instead of in the Makefile. No files need to be copied, just added / replaced in libcolony.gyp See Whitelist additions #502
  2. Implements Array.prototype.lastIndexOf
  3. Implements uncaughtException (again?)
  4. Implements process-level domain support for thrown errors
  5. Adds tests for domain module

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If process.domain is set, don't fire an uncaughtException.

@kevinmehall
Copy link
Member

r+ on the first two commits, if you want to break those out to a new PR.

Is there a reason to merge this before Domain support is implemented enough to actually work? Without support for hooking async events, this is basically a broken try-catch. In addition to Node-defined things, like setTimeout and net, we have to think about how it should interact with hardware IO.

Because of how it works, Domain support has to invade every other part of the system. I'm not entirely convinced we want to go down this rabbit hole just yet. Is there something blocking on this, or even a compelling use case?

@tcr
Copy link
Member Author

tcr commented Oct 13, 2014

This PR would unblock code which relies on but not extensively uses the domain module (see kevinswiber's issues from July). I feel this comes up with some frequency in HTTP libraries.

This provides a starting point for other contributors to submit patches against if they want to build out domain functionality. Since this won't impact any code that doesn't use the domain module, it's effectively opt-in.

And a weak technical argument: since the domain error case presumes you're going to do graceful system shutdown, even implementing domain support improperly will, in the worst case, abort the VM on error, which on Tessel frees all allocated resources anyway.

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.

3 participants