Add code comment to explain conditional throw on unCaught exceptions

[markstos]

It wasn't clear at first why you would throw sometimes but others here, so I researched and added a comment to leave a hint for the next code reader.

[markstos]

Also, I didn't see in the README or CONTRIBUTING docs how to file an "issue".

It appears that the test suite has no test for the case for your uncaught exception handling when there are multiple handlers. It's an important case to check because in one case you re-throw the error and in the other case you don't.

(If the test is there but I missed it, perhaps the test name could use a more explicit label).

This case is important for people who may be combining the newrelic module with another one that also registers the same event handler, like the bugsnag module.

It appears you may have an unstated expectation that if there are multiple listeners for this event that the other other event will fire /after/ the newrelic event and that /it/ throws an error and shut's down.

If the other event ran first and shut down the process, the newrelic process would never fire. This is never supposed to happen because you advise to always load the newrelic module first, but a local code comment about that expectation could still be helpful. Second, what happens if the other module behaves in a similar way and /also/ choose not to shutdown the process after the uncaught exception is handled, like New Relic does? Test coverage could and code comments or docs could help clarify your intent for the case of when the unCaughtException event has multiple listeners.

[NatalieWolfe]

Hi Mark,

Thanks for the PR! More comments are always good and we'll try to roll this into the next release.

Also, I didn't see in the README or CONTRIBUTING docs how to file an "issue".

This is the correct way to file an issue with the agent. :) We should make that more clear in the contributing doc though.

It appears that the test suite has no test for the case for your uncaught exception handling when there are multiple handlers. It's an important case to check because in one case you re-throw the error and in the other case you don't.

(If the test is there but I missed it, perhaps the test name could use a more explicit label).

The test case is in test/integration/core/exceptions.tap.js and is titled "caught uncaught exceptions." Our tests are definitely not the most straightforwardly organized, but it is all there.

It appears you may have an unstated expectation that if there are multiple listeners for this event that the other other event will fire /after/ the newrelic event and that /it/ throws an error and shut's down.

The order of the other handlers doesn't matter to us. If another handler takes that error and then rethrows it your server will crash and you'll know about the error from that, the same as the case were we are the only error handler. Our check for that listener count is simply an attempt at maintaining the normal behavior of your server if NR were not installed.

[markstos]

Thanks for the response.

I've done some testing now with a "Hello World" script testing to see what happens when both New Relic and BugSnag are loaded. It appears that both NR and BugSnag both handle the exception as expected and the program exits-- all good.

• It would be helpful to update the label for the "caught uncaught exceptions" test to clearly label which test is testing the "uncaught exception" case for the "only one listener" case which one is covering the "multiple listeners" case.
• It also be helpful to document your use of "process._fatalException". When Googling for this, there are hardly any results. It appears to be undocumented in modern Node.js. So I assumed it was support for ancient Node.js versions. More digging turned up that it's part of the newer 'domains' feature added in Node.js 0.8, and is actually the code path triggered when I run the code in Node.js 4.4.x. I presume it's there because the domain code path gives richer or more reliable data than the 'uncaughtException' code path, which is also supported by modern Node.js versions.

Reference: http://www.slideshare.net/domenicdenicola/domains-20010482 ( Some slides reference how process._fatalException is used as part of the domains feature ).

[NatalieWolfe]

We use _fatalException because wrapping it will not potentially change the behavior of the server. The uncaughtException event should crash the server if there are no listeners for it, which is why we have to do that check for the special case where we are the only listener.

I agree with you regarding comments though. More comments are always good, and PRs are accepted. :)