illegal hardware exception #6

Activity

1. sam-github commented on Oct 27, 2016

   

   sam-github

   on Oct 27, 2016

   ContributorAuthor

   More actions

   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*) ()
   
2. rnchamberlain commented on Oct 28, 2016

   

   rnchamberlain

   on Oct 28, 2016

   Last edited by rnchamberlain

   Contributor

   More actions

   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.
3. sam-github commented on Oct 28, 2016

   

   sam-github

   on Oct 28, 2016

   ContributorAuthor

   More actions

   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).
4. rnchamberlain

   self-assigned this

   on Feb 24, 2017
5. rnchamberlain commented on Feb 24, 2017

   

   rnchamberlain

   on Feb 24, 2017

   Contributor

   More actions

   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
6. richardlau commented on Mar 10, 2017

   

   richardlau

   on Mar 10, 2017

   Member

   More actions

   We replace Node.js' ShouldAbortOnUncaughtException callback with our own. Should we duplicate the logic?
7. rnchamberlain commented on Mar 15, 2017

   

   rnchamberlain

   on Mar 15, 2017

   Contributor

   More actions

   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
8. rnchamberlain commented on Mar 23, 2017

   

   rnchamberlain

   on Mar 23, 2017

   Contributor

   More actions

   @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
9. rnchamberlain

   closed this as completedon Apr 3, 2017