Skip to content
This repository has been archived by the owner on Jun 18, 2021. It is now read-only.

illegal hardware exception #6

Closed
sam-github opened this issue Oct 27, 2016 · 7 comments
Closed

illegal hardware exception #6

sam-github opened this issue Oct 27, 2016 · 7 comments
Assignees

Comments

@sam-github
Copy link
Contributor

Is this expected?

core/nodereport (master % u=) % node demo/exception.js                  
exception.js: Node running
exception.js: Go to http://<machine>:8080/ or http://localhost:8080/

Writing Node.js report to file: NodeReport.20161027.100601.28911.001.txt
Node.js report completed
Uncaught #<UserException>

FROM
Server.my_listener (/home/sam/w/core/nodereport/demo/exception.js:15:5)
emitTwo (events.js:106:13)
Server.emit (events.js:191:7)
HTTPParser.parserOnIncoming [as onIncoming] (_http_server.js:546:12)
HTTPParser.parserOnHeadersComplete (_http_common.js:99:23)
zsh: illegal hardware instruction (core dumped)  node demo/exception.js

System info:

  • linux ubuntu x64
  • node 6.9.1, npm 3.10.8
  • nodereport from git 7cd3f76

I got this when I was trying to verify whether nodereport after reporting an uncaught exception, would let the exception continue to propagate and cause node to exit (as it should).

@sam-github
Copy link
Contributor Author

Its fairly reproduceable. I ran with gdb, too:

Node.js report completed
Uncaught #<UserException>

FROM
Server.my_listener (/home/sam/w/core/nodereport/demo/exception.js:15:5)
emitTwo (events.js:106:13)
Server.emit (events.js:191:7)
HTTPParser.parserOnIncoming [as onIncoming] (_http_server.js:546:12)
HTTPParser.parserOnHeadersComplete (_http_common.js:99:23)

Thread 1 "node" received signal SIGILL, Illegal instruction.
0x000000000132de89 in v8::base::OS::Abort() ()
(gdb) bt
#0  0x000000000132de89 in v8::base::OS::Abort() ()
#1  0x0000000000d4e30a in v8::internal::Isolate::Throw(v8::internal::Object*, v8::internal::MessageLocation*) ()
#2  0x0000000000eaf423 in v8::internal::Runtime_Throw(int, v8::internal::Object**, v8::internal::Isolate*) ()

@rnchamberlain
Copy link
Contributor

rnchamberlain commented Oct 28, 2016

Yes, nodereport sets --abort_on_uncaught_exception, and uses isolate->SetAbortOnUncaughtExceptionCallback() to intercept the exception. So the behaviour you are seeing should be the same as when running node with --abort_on_uncaught_exception. On Linux the v8::base::OS::Abort()code intentionally aborts and triggers a core dump using an illegal instruction. The nodereport setCoreDump(yes/no) API should let you switch to node behaviour without --abort_on_uncaught_exception. Maybe that should be the default.

@sam-github
Copy link
Contributor Author

I think normal node behaviour should be the default, yes. If people want abort on uncaught exception, they can set that flag on the command line.

But... it sounds like the mechanism you are using won't allow that. Have you considered using process.once('uncaughtException', .. (in c++) to catch uncaught exceptions? That would allow you to re-raise them, I think, and for whatever the "normal" thing is to happen (normal means uncaught exceptions usually print stack traces and exit with 1, unless --abort_on_uncaught_exception is set in which case it aborts, or unless some user has unwisely added an event listener for uncaughtException, in which case they are responsible for handling it).

I'm pretty surprised node aborts with an illegal instruction and not abort(3).

@rnchamberlain
Copy link
Contributor

This still needs more work:

  • on node 4 and 6, with node-report enabled, the uncaught exception prints a stack trace but node continues rather than exiting with rc=1

  • the demos need a refresh to reflect the node-report defaults change, plus improvement, e.g buttons to trigger the required events rather than browser refresh

@richardlau
Copy link
Member

We replace Node.js' ShouldAbortOnUncaughtException callback with our own. Should we duplicate the logic?

@rnchamberlain
Copy link
Contributor

There is a complicated story behind ShouldAbortOnUncaughtException() in node.cc, see:
nodejs/node#3036
nodejs/node#3654

The logic was added to fix behaviour when domains are used. It prevents the --abort-on-uncaught-exception option from triggering an abort when domains are used to catch errors. In practice, domains are deprecated, and the call to TopDomainHasErrorHandler(env) has a simple check, which means that the logic is a no-op now:

if (!env->using_domains())
    return false;

So I've simply omitted that code in the node-report callback

@rnchamberlain
Copy link
Contributor

@sam-github latest node-report v2.1.2 has the fix to the exception handler, and the improved demos, hopefully we can close this now

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants