Skip to content
This repository has been archived by the owner on Apr 22, 2023. It is now read-only.

domain: move error handling directly into instance #6096

Merged
merged 1 commit into from
Aug 23, 2013
Merged

domain: move error handling directly into instance #6096

merged 1 commit into from
Aug 23, 2013

Conversation

trevnorris
Copy link

Instead of doing all the domain handling in core, allow the domain to
set an error handler that'll take care of it all. This way the domain
error handling can be abstracted enough for any user to use it.

@trevnorris
Copy link
Author

@isaacs just need a quick approval.

This is going to add some new functionality that we're going to pretend doesn't exist. For example, you can do the following:

process.domain = {
  _errorHandler: function errorHandler(er) {
    return true;
  },
  enter: function enter() { },
  exit: function exit() { }
}

setTimeout(function() {
  throw new Error('freak yeah!');
});

And... it'll catch all errors. Basically another uncaughtException.

This is a first step at working towards #6011, and since it can be done separate I'd like to get it in now to lighten the review load necessary from the other.

@bnoordhuis
Copy link
Member

LGTM in the sense that I don't see anything obviously wrong. Isaac should probably sign off on it as well though.

@trevnorris
Copy link
Author

I'm going to infer @isaacs is good with this. From IRC:

Aug 21 16:13:51 <trevnorris>  isaacs: do you mind a change like this: https://github.com/trevnorris/node/compare/domain-api-redo-again-sucka
Aug 21 16:14:16 <trevnorris>  isaacs: hm, here. this is w/o the multi-context stuff: https://github.com/trevnorris/node/commit/5764104
Aug 21 16:15:08 <isaacs>      trevnorris: that seems fine

Instead of doing all the domain handling in core, allow the domain to
set an error handler that'll take care of it all. This way the domain
error handling can be abstracted enough for any user to use it.
@trevnorris trevnorris merged commit 467e00e into nodejs:master Aug 23, 2013
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants