src: adjust windows abort behavior to make sense #13947

jkantr · 2017-06-27T16:39:25Z

Raising SIGABRT is handled in the CRT in windows, calling _exit()
with ambiguous code "3" by default.

This adjustment to the abort behavior gives a more sane exit code
on abort, by calling _exit() directly with code 134.

I added a test I used initially in reference to recreating #12271 but it may be moot to merge.

Fixes: #12271
Refs: https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/abort

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

Affected core subsystem(s)

src

refack · 2017-06-27T16:45:13Z

Conservatively marking as semver-major.
Should it be considered semver-patch, since previous behaviour could be considered erroneous ?

refack · 2017-06-27T16:47:14Z

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

@@ -21,7 +21,7 @@ function run(flags, signals) {
  child.on('exit', common.mustCall(function(code, sig) {
    if (common.isWindows) {
      if (signals)
-        assert.strictEqual(code, 3);
+        assert.strictEqual(code, 3221225477);


Could you replace this with 0xC0000005 (better google results)

refack · 2017-06-27T16:47:29Z

test/common/index.js

  if (exports.isWindows)
-    expectedExitCodes = [3221225477, 3];
+    expectedExitCodes = [3221225477, 134];


Same 0xC0000005

richardlau · 2017-06-27T16:51:19Z

@nodejs/platform-windows

refack · 2017-06-27T16:52:01Z

@jkantr One request, add 134 as an example in the last note in https://github.com/nodejs/node/blob/master/doc/api/process.md#exit-codes

refack · 2017-06-27T16:53:00Z

/cc @nodejs/release how do we decide the semver level?

richardlau · 2017-06-27T17:00:51Z

test/parallel/test-windows-failed-heap-allocation.js

+}
+
+// --max-old-space-size=3 is the min 'old space' in v8, explodes fast
+const cmd = `"${process.argv[0]}" --max-old-space-size=3 "${process.argv[1]}"`;


This could use process.execPath and __filename.

richardlau · 2017-06-27T17:05:37Z

test/parallel/test-windows-failed-heap-allocation.js

+  fn(2);
+}
+
+// --max-old-space-size=3 is the min 'old space' in v8, explodes fast


Small nit: v8 -> V8. This will help distinguish between Node.js 8 and V8 JavaScript engine.

richardlau · 2017-06-27T17:12:11Z

test/parallel/test-windows-failed-heap-allocation.js

+
+// --max-old-space-size=3 is the min 'old space' in v8, explodes fast
+const cmd = `"${process.argv[0]}" --max-old-space-size=3 "${process.argv[1]}"`;
+exec(`${cmd} heapBomb`, (err) => {


Please wrap the callback with common.mustCall().

richardlau · 2017-06-27T17:15:12Z

test/parallel/test-windows-failed-heap-allocation.js

+const cmd = `"${process.argv[0]}" --max-old-space-size=3 "${process.argv[1]}"`;
+exec(`${cmd} heapBomb`, (err) => {
+  const msg = `Wrong exit code of ${err.code}! Expected 134 for abort`;
+  assert.strictEqual(err.code, 134, msg);


Might be worth a short comment explaining why common.nodeProcessAborted() is not used.

gireeshpunathil · 2017-06-29T09:54:14Z

#node --v8-options | grep "abort_on_uncaught_exception"
  --abort_on_uncaught_exception (abort program (dump core) when an uncaught exception is thrown)
#

Will this change imply the phrase dump core will not be honored? I have no evidence that Windows dumps core on abort (Windows internals has been bit grey for me) but just wondering what is the difference with and without the flag in Windows, if a core is not produced.

If we cannot prove either way (ability / inability to configure Windows to collect dump on abort) I suggest we document on the lines of :
--abort_on_uncaught_exception does not dump in Windows.
It may be the current behavior as well, but now by exiting, we are closing out all the possibilities.

Programs that embed node and programs that spawn node may have static assumptions about the current behavior of abort / exit code, so this change warrants semver-major I guess.

refack · 2017-06-29T12:30:22Z

If we cannot prove either way (ability / inability to configure Windows to collect dump on abort) I suggest we document on the lines of :

This change will stop windows from even attempting a core dump. It could be restored with an explicit call to MiniDumpWriteDump

gireeshpunathil · 2017-06-29T13:30:52Z

@refack - right, so the question is: do we follow the doc (abort program with dump on uncaught exception) or make this change and amend the doc to that effect.

My point is: if Windows never dumped a core on aborted node before, or we don't know of anyone reporting that as an issue, I think we are good with this change, with the doc reflecting the change. If we get issues that require dump inspection, we can always bring an in-process dumping feature through MiniDumpWriteDump. Thoughts?

refack · 2017-06-29T13:33:09Z

/cc @nodejs/post-mortem
@billti @AndrewPardoe does the VS team have an opinion on node core dumps (In Windows)?

bzoz · 2017-06-29T14:08:45Z

On Windows libuv disables WER and thus also dumping cores (src/win/core.c:177). From a quick search it looks like it is not enabled anywhere else in the codebase.

gireeshpunathil · 2017-06-29T14:21:31Z

thanks @bzoz . And that piece of code is around there since 2011, so we are good in terms of core dump. in terms of doc - while it looks clean to state this fact explicitly, I am fine with this change as it is.

gireeshpunathil · 2017-06-29T14:34:09Z

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

gireeshpunathil · 2017-06-30T05:48:26Z

test failures seem unrelated:
pi3-raspbian-jessie: not ok 67 parallel/test-fs-readdir-ucs2
win2008r2: not ok 177 parallel/test-icu-data-dir
both of which passed later

FYI - @refack

tniessen · 2017-06-30T09:30:58Z

@gireeshpunathil Yes, these are known problems, even though the latter should be fixed now, ref #13986.

refack · 2017-06-30T10:16:02Z

I've found this black magic: https://docs.microsoft.com/en-us/windows-hardware/drivers/debugger/setting-and-clearing-flags-for-silent-process-exit
Which gave me:

refack · 2017-06-30T10:37:38Z

RE last massage: black magic still works after this PR 👍
Fresh CI: https://ci.nodejs.org/job/node-test-commit/10840/

ping @nodejs/ctc PTAL, this needs another review

gireeshpunathil · 2017-06-30T11:52:34Z

@refack - great, that is a good news! Where do I set the Silent Exit Flag? gflags.exe? I could not locate it in any of my Win 7/10 systems. Is it an external download?

refack · 2017-06-30T16:43:36Z

Where do I set the Silent Exit Flag? gflags.exe?

gflags.zip

The "good" gflags.exe is part of the 8.1 SDK - C:\Program Files (x86)\Windows Kits\8.1\Debuggers\x64
The one that comes with the 10 SDK has no GUI.

refack · 2017-07-09T18:23:36Z

CI to make sure it's still fresh: https://ci.nodejs.org/job/node-test-pull-request/9052/
@nodejs/ctc need another review, PATL

refack · 2017-07-25T15:27:41Z

@nodejs/ctc this needs another review, PTAL

Trott · 2017-07-25T18:34:22Z

Aside: The wait to get CTC reviews for this may be a sign that we need more Windows representation on CTC.

mhdawson

As long as its SemVer Major seems good to me.

refack · 2017-08-01T18:57:52Z

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

refack · 2017-08-02T00:15:12Z

@jkantr after the long wait we got the 2 CTC approval, but a failing test came up could you take a look?

not ok 1 async-hooks/test-callback-error
  ---
  duration_ms: 0.631
  severity: fail
  stack: |-
    start case 1
    end case 1: 131.106ms
    start case 2
    end case 2: 130.250ms
    start case 3
    end case 3: 6.412ms
    assert.js:42
      throw new errors.AssertionError({
      ^
    
    AssertionError [ERR_ASSERTION]: 134 === 3
        at ChildProcess.child.on (c:\workspace\node-test-binary-windows\RUN_SUBSET\0\VS_VERSION\vcbt2015\label\win10\test\async-hooks\test-callback-error.js:104:14)
        at emitTwo (events.js:125:13)
        at ChildProcess.emit (events.js:213:7)
        at maybeClose (internal/child_process.js:929:16)
        at Process.ChildProcess._handle.onexit (internal/child_process.js:212:5)
  ...

Trott · 2017-08-07T23:37:47Z

This has two CTC approvals, etc. Any reason this shouldn't land?

refack · 2017-08-07T23:43:09Z

There's still one failing test, but I've talked to @jkantr about it...

refack · 2017-08-08T01:44:00Z

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

refack · 2017-08-08T02:37:26Z

All CI fails are known flakes or infra problems

Raising SIGABRT is handled in the CRT in windows, calling _exit() with ambiguous code "3" by default. This adjustment to the abort behavior gives a more sane exit code on abort, by calling _exit directly with code 134. PR-URL: nodejs#13947 Fixes: nodejs#12271 Refs: https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/abort Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Timothy Gu <timothygu99@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

refack · 2017-08-08T02:48:33Z

Landed in 80ebb42
@jkantr congrats on being GitHub promoted from

to

refack · 2017-08-08T02:49:42Z

Extra sanity test on master: https://ci.nodejs.org/job/node-test-commit-linuxone/7813/

nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Jun 27, 2017

refack added process Issues and PRs related to the process subsystem. semver-major PRs that contain breaking changes and should be released in the next major version. labels Jun 27, 2017

refack approved these changes Jun 27, 2017

View reviewed changes

richardlau reviewed Jun 27, 2017

View reviewed changes

refack approved these changes Jun 27, 2017

View reviewed changes

refack added the windows Issues and PRs related to the Windows platform. label Jun 27, 2017

refack self-assigned this Jun 27, 2017

richardlau approved these changes Jun 27, 2017

View reviewed changes

TimothyGu approved these changes Jun 28, 2017

View reviewed changes

tniessen approved these changes Jun 28, 2017

View reviewed changes

jasnell approved these changes Jun 29, 2017

View reviewed changes

gireeshpunathil approved these changes Jun 29, 2017

View reviewed changes

refack mentioned this pull request Jul 6, 2017

src: document --abort-on-uncaught-exception #13931

Merged

2 tasks

Trott mentioned this pull request Jul 16, 2017

Node.js Foundation Core Technical Committee (CTC) Meeting 2017-07-19 nodejs/CTC#154

Closed

Trott mentioned this pull request Jul 31, 2017

Node.js Foundation Core Technical Committee (CTC) Meeting 2017-08-02 nodejs/CTC#160

Closed

mhdawson approved these changes Aug 1, 2017

View reviewed changes

refack mentioned this pull request Aug 7, 2017

src: Node implementation of v8::Platform #14001

Closed

3 tasks

jkantr force-pushed the win32-heap-abort-code branch from 042bf12 to f4186dc Compare August 8, 2017 01:37

refack force-pushed the win32-heap-abort-code branch from f4186dc to 8f6b762 Compare August 8, 2017 02:44

refack force-pushed the win32-heap-abort-code branch from 8f6b762 to 80ebb42 Compare August 8, 2017 02:46

refack merged commit 80ebb42 into nodejs:master Aug 8, 2017

Trott removed the ctc-review label Aug 13, 2017

This was referenced Sep 11, 2017

v8.5.0 proposal #15308

Merged

test: run 'abort' suite on Windows #15056

Merged

MylesBorins mentioned this pull request Sep 11, 2017

deps: update V8 to 6.1 #14730

Closed

5 tasks

jasnell mentioned this pull request Oct 30, 2017

Proposal: 2017-10-31, Version 9.0.0 (Current) #15135

Merged

ofrobots mentioned this pull request Sep 24, 2018

trace_event: destroy platform before tracing #22938

Closed

2 tasks

refack removed their assignment Oct 20, 2018

refack mentioned this pull request Jan 15, 2019

Windows CI failures: parallel/test-trace-events-fs-sync #25512

Closed

src: adjust windows abort behavior to make sense #13947

src: adjust windows abort behavior to make sense #13947

Conversation

jkantr commented Jun 27, 2017

Checklist

Affected core subsystem(s)

refack commented Jun 27, 2017

refack Jun 27, 2017

Choose a reason for hiding this comment

refack Jun 27, 2017

Choose a reason for hiding this comment

richardlau commented Jun 27, 2017

refack commented Jun 27, 2017

refack commented Jun 27, 2017

richardlau Jun 27, 2017

Choose a reason for hiding this comment

richardlau Jun 27, 2017

Choose a reason for hiding this comment

richardlau Jun 27, 2017

Choose a reason for hiding this comment

richardlau Jun 27, 2017

Choose a reason for hiding this comment

gireeshpunathil commented Jun 29, 2017

refack commented Jun 29, 2017

gireeshpunathil commented Jun 29, 2017

refack commented Jun 29, 2017 • edited Loading

bzoz commented Jun 29, 2017

gireeshpunathil commented Jun 29, 2017

gireeshpunathil commented Jun 29, 2017

gireeshpunathil commented Jun 30, 2017

tniessen commented Jun 30, 2017

refack commented Jun 30, 2017

refack commented Jun 30, 2017

gireeshpunathil commented Jun 30, 2017

refack commented Jun 30, 2017 • edited Loading

refack commented Jul 9, 2017

refack commented Jul 25, 2017

Trott commented Jul 25, 2017

mhdawson left a comment

Choose a reason for hiding this comment

refack commented Aug 1, 2017

refack commented Aug 2, 2017

Trott commented Aug 7, 2017

refack commented Aug 7, 2017

refack commented Aug 8, 2017

refack commented Aug 8, 2017

refack commented Aug 8, 2017

refack commented Aug 8, 2017

refack commented Jun 29, 2017 •

edited

Loading

refack commented Jun 30, 2017 •

edited

Loading