src: honor --abort_on_uncaught_exception flag #2776

evanlucas · 2015-09-09T16:47:03Z

Fix regression introduced in 0af4c9e
that ignores the --abort-on-uncaught-exception flag. Prior to that
commit, the flag was passed through to v8. After that commit, the
process just calls exit(1).

The tests added in 0af4c9e all are still passing.

There probably is a better way of doing this, but this is working for me, so...

cjihrig · 2015-09-09T16:52:48Z

Is it possible to add a test to prevent the regression from happening again?

evanlucas · 2015-09-09T16:56:54Z

That was something I wasn't sure about. Would that require certain configurations on different systems? Or is there a particular exit code on an abort?

cjihrig · 2015-09-09T17:02:24Z

I'm not sure, but it looks like the tests in 0af4c9e expect an exit code of 0.

evanlucas · 2015-09-09T17:57:39Z

Aren't those explicitly catching the errors in the domain though?

cjihrig · 2015-09-09T18:17:40Z

Ah, right. I'm just guessing here, but can you do anything like detect SIGABRT or check the process output.

evanlucas · 2015-09-09T18:20:36Z

Yes, I believe I can actually. Will update shortly

evanlucas · 2015-09-09T18:43:17Z

Ok, added a test that checks the signal in the error message of child_process.exec

evanlucas · 2015-09-10T13:26:07Z

CI: https://ci.nodejs.org/job/node-test-pull-request/272/

cjihrig · 2015-09-10T14:47:45Z

The CI doesn't seem to be happy with this.

evanlucas · 2015-09-10T14:57:17Z

Yea, will look into today

evanlucas · 2015-09-10T20:28:31Z

Ok, updated CI: https://ci.nodejs.org/job/node-test-pull-request/274/

Not sure whats up with the windows exit code?? Getting 3221226505...

https://ci.nodejs.org/job/node-test-commit-windows/602/nodes=win2012r2/tapResults/

bnoordhuis · 2015-09-10T20:44:40Z

@evanlucas That's error code 0xC0000409 or STATUS_STACK_BUFFER_OVERRUN.

evanlucas · 2015-09-10T21:14:21Z

@bnoordhuis is that what should occur when aborted on windows?

bnoordhuis · 2015-09-10T21:48:09Z

I don't think so. The exit code on abort is normally 3.

bnoordhuis · 2015-09-10T21:55:17Z

test/parallel/test-abort-uncaught-exception.js

+  exec(cmd, function(err, stdout, stderr) {
+    assert(err);
+    // darwin only provides the signal on an abort
+    // linux only provides the exit code (134) on an abort


This isn't right. Both platforms raise SIGABRT on abort() but SIGABRT has a different signal number on Linux than on OS X, 6 vs. 22. When a process terminates after a signal, the exit code is 128 + signo, so 134 on Linux and 150 on OS X.

Sorry, I take that back - it's 6 on OS X as well, 22 is another platform. The point remains that both platforms should indicate that the process was killed by a signal.

The CI croaked when I had it that way. On ubuntu, signal was null when abort occurred. On OS X, the exit code was null (at least when run as a child process) and the signal was provided

I think the issue here is that you're using exec() instead of spawn(). The former runs the command through a shell and that may interfere with signal propagation.

evanlucas · 2015-09-10T22:44:44Z

ok, switched to spawn as that should fix the issues. New CI: https://ci.nodejs.org/job/node-test-pull-request/275/

Fishrock123 · 2015-09-11T14:26:15Z

Windows still having issues:

https://ci.nodejs.org/job/node-test-commit-windows/605/nodes=win2012r2/tapTestReport/test.tap-1/

# 
# assert.js:89
# throw new assert.AssertionError({
# ^
# AssertionError: null === 'SIGABRT'

evanlucas · 2015-09-11T14:27:36Z

Yea, I saw that. I guess it makes sense that the signal on Windows should be null?

bnoordhuis · 2015-09-11T14:30:45Z

Yes, libuv doesn't translate the exit code to a signal number.

Fishrock123 · 2015-09-11T14:37:35Z

test/parallel/test-abort-uncaught-exception.js

+  child.on('exit', function(code, sig) {
+    if (!common.isWindows) {
+      assert.strictEqual(sig, signal);
+    }


SHould we still check something for windows? Otherwise, maybe produce skip output?

I would think we should. I am just not sure what to check at this point on windows.

If we are ignoring this assertion for windows, why don't we skip the test for windows?

thefourtheye · 2015-09-14T01:11:56Z

test/parallel/test-abort-uncaught-exception.js

+  if (flags)
+    args.unshift(flags);
+
+  var child = spawn(node, args);


evanlucas · 2015-09-14T11:28:15Z

Ok, should be fixed now. CI: https://ci.nodejs.org/job/node-test-pull-request/294/

evanlucas · 2015-09-14T11:46:33Z

Looks like win2012 is still unhappy...

cjihrig · 2015-09-16T14:09:25Z

LGTM if the CI is happy now

bnoordhuis · 2015-09-16T14:46:02Z

src/node.cc

+      raise(SIGABRT);
+#else
+      abort();
+#endif


Special-casing abort() here and only here is pretty meh IMO. At the very least add a comment but personally, I'd add an ABORT() macro to src/util.h and update all calls to abort() in src/ in one fell swoop.

mscdex · 2015-09-16T14:57:55Z

LGTM

evanlucas · 2015-09-16T15:15:23Z

Ok, updated with @bnoordhuis's suggestion.

evanlucas · 2015-09-16T15:27:03Z

CI: https://ci.nodejs.org/job/node-test-pull-request/315/

trevnorris · 2015-09-16T19:12:33Z

test/parallel/test-abort-uncaught-exception.js

+  throw new Error('child error');
+} else {
+  run('', null);
+  run('--abort-on-uncaught-exception', 'SIGABRT');


@bnoordhuis will this generate core files that stick around every time tests run?

if you have core dumps enabled, then it will

That'll be a problem for a lot of devs, like myself. A test is nice, but allowing them to abort isn't so nice.

I agree. Any suggestions on how to handle?

Unfortunately no. I've had cases like this in the past as well, but never had a good solution. Possibly add it to another folder in test/?

evanlucas · 2015-09-17T20:21:36Z

I moved the test to test/abort per @trevnorris. PTAL

trevnorris · 2015-09-17T20:22:37Z

LGTM

Windows 8+ compiled in Release mode exits with code 0xC0000409 when abort() is called. This prevents us from being able to reliably verify an abort exit code (3) on windows. PR-URL: nodejs#2776 Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Brian White <mscdex@mscdex.net> Reviewed-by: Trevor Norris <trev.norris@gmail.com>

Fix regression introduced in 0af4c9e that ignores the --abort-on-uncaught-exception flag. Prior to that commit, the flag was passed through to v8. After that commit, the process just calls exit(1). PR-URL: nodejs#2776 Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Brian White <mscdex@mscdex.net> Reviewed-by: Trevor Norris <trev.norris@gmail.com>

skomski · 2015-09-17T20:28:25Z

Would it not be better to use an inline function with a unique name like NodeAbort rather than an unsafe macro with a name where it's not clear if it's defined by node?

evanlucas · 2015-09-17T20:40:31Z

@skomski could you be more specific on what is making this macro unsafe?

It is also implemented similarly to the other macros in src/util.h

skomski · 2015-09-17T22:19:25Z

@evanlucas I would always prefer a inline function over a macro if its possible and the name ABORT is not unique which makes it even more unsafe and unclear who provides it. It's not a special case like CHECK.
More from style guide: https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Preprocessor_Macros.

It's simply unnecessary to use a macro in this case.

Windows 8+ compiled in Release mode exits with code 0xC0000409 when abort() is called. This prevents us from being able to reliably verify an abort exit code (3) on windows. PR-URL: #2776 Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Brian White <mscdex@mscdex.net> Reviewed-by: Trevor Norris <trev.norris@gmail.com>

Fix regression introduced in 0af4c9e that ignores the --abort-on-uncaught-exception flag. Prior to that commit, the flag was passed through to v8. After that commit, the process just calls exit(1). PR-URL: #2776 Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Brian White <mscdex@mscdex.net> Reviewed-by: Trevor Norris <trev.norris@gmail.com>

bnoordhuis · 2015-09-25T12:31:19Z

I've added land-on-v3.x and land-on-v4.x labels. Remove them if that's wrong but please remove them from #3058 as well.

mscdex added the c++ Issues and PRs that require attention from people who are familiar with C++. label Sep 9, 2015

evanlucas force-pushed the fixabrt branch from f754eca to bc28f05 Compare September 9, 2015 18:42

evanlucas force-pushed the fixabrt branch from bc28f05 to 7c3bc64 Compare September 10, 2015 19:50

bnoordhuis reviewed Sep 10, 2015
View reviewed changes

evanlucas force-pushed the fixabrt branch from 7c3bc64 to 3ba0bac Compare September 10, 2015 22:38

evanlucas force-pushed the fixabrt branch from 3ba0bac to e92bd00 Compare September 11, 2015 14:34

Fishrock123 reviewed Sep 11, 2015
View reviewed changes

Fishrock123 mentioned this pull request Sep 13, 2015

Release proposal: v4.1.0 #2844

Closed

7 tasks

thefourtheye reviewed Sep 14, 2015
View reviewed changes

test/parallel/test-abort-uncaught-exception.js

if (flags)

args.unshift(flags);

var child = spawn(node, args);

Copy link

Contributor

thefourtheye Sep 14, 2015

Choose a reason for hiding this comment

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

const :D

evanlucas force-pushed the fixabrt branch from e92bd00 to 2bdcc40 Compare September 14, 2015 11:24

bnoordhuis reviewed Sep 16, 2015
View reviewed changes

evanlucas force-pushed the fixabrt branch from 7557983 to ee510fe Compare September 16, 2015 15:14

evanlucas force-pushed the fixabrt branch from ee510fe to 4cadbe4 Compare September 16, 2015 19:05

trevnorris reviewed Sep 16, 2015
View reviewed changes

evanlucas force-pushed the fixabrt branch from 4cadbe4 to 9f448cc Compare September 17, 2015 19:54

evanlucas mentioned this pull request Sep 17, 2015

Account for core dumps on CI machines nodejs/build#196

Closed

evanlucas added 2 commits September 17, 2015 15:25

evanlucas force-pushed the fixabrt branch from 9f448cc to 921f2de Compare September 17, 2015 21:21

evanlucas merged commit 921f2de into nodejs:master Sep 17, 2015

evanlucas deleted the fixabrt branch September 17, 2015 21:21

trevnorris mentioned this pull request Sep 20, 2015

Release Proposal: v4.1.1 #2943

Closed

rvagg mentioned this pull request Sep 22, 2015

Release proposal: v4.1.1 #2995

Merged

misterdjules mentioned this pull request Sep 24, 2015

node does not abort at the right time when using --abort-on-uncaught-exception #3035

Closed

bnoordhuis added land-on-v3.x labels Sep 25, 2015

jasnell added the lts-landed-on-v4.x label Oct 7, 2015

MylesBorins removed the land-on-v4.x label Nov 16, 2015

src: honor --abort_on_uncaught_exception flag #2776

src: honor --abort_on_uncaught_exception flag #2776

Conversation

evanlucas commented Sep 9, 2015

cjihrig commented Sep 9, 2015

evanlucas commented Sep 9, 2015

cjihrig commented Sep 9, 2015

evanlucas commented Sep 9, 2015

cjihrig commented Sep 9, 2015

evanlucas commented Sep 9, 2015

evanlucas commented Sep 9, 2015

evanlucas commented Sep 10, 2015

cjihrig commented Sep 10, 2015

evanlucas commented Sep 10, 2015

evanlucas commented Sep 10, 2015

bnoordhuis commented Sep 10, 2015

evanlucas commented Sep 10, 2015

bnoordhuis commented Sep 10, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evanlucas commented Sep 10, 2015

Fishrock123 commented Sep 11, 2015

evanlucas commented Sep 11, 2015

bnoordhuis commented Sep 11, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evanlucas commented Sep 14, 2015

evanlucas commented Sep 14, 2015

cjihrig commented Sep 16, 2015

Choose a reason for hiding this comment

mscdex commented Sep 16, 2015

evanlucas commented Sep 16, 2015

evanlucas commented Sep 16, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evanlucas commented Sep 17, 2015

trevnorris commented Sep 17, 2015

skomski commented Sep 17, 2015

evanlucas commented Sep 17, 2015

skomski commented Sep 17, 2015

bnoordhuis commented Sep 25, 2015