Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

test: common.nodeProcessAborted() does not catch aborts in Windows #12823

Closed
gireeshpunathil opened this issue May 4, 2017 · 5 comments
Closed
Labels
test Issues and PRs related to the tests. windows Issues and PRs related to the Windows platform.

Comments

@gireeshpunathil
Copy link
Member

  • Version: v7.8.0 for example.
  • Platform: Windows (10, for example)
  • Subsystem: test (common.js)

Looks like common.nodeProcessAborted() (test/common.js) does not cover Windows aborts comprehensively. Results from MAC and Windows on a sample code show the difference:

'use strict';
const common = require('../common');
const assert = require('assert');
const spawn = require('child_process').spawn;

if (process.argv[2] === 'child') {
  process.abort();
} else {
  const child = spawn(process.argv[0], [process.argv[1], 'child']);
  child.on('exit', common.mustCall((code, signal) => {
    console.log('child exited with code: ' + code + ' and signal: ' + signal);
    assert(common.nodeProcessAborted(code, signal),
           'process should have aborted, but did not');
  }));
}

MAC:

child exited with code: null and signal: SIGABRT

Windows:

child exited with code: 3 and signal: null
assert.js:81
  throw new assert.AssertionError({
  ^
AssertionError: process should have aborted, but did not
    at ChildProcess.child.on.common.mustCall (D:\gireesh\parser\node\test\parallel\foo.js:12:5)
    at ChildProcess.<anonymous> (D:\gireesh\parser\node\test\common.js:452:15)
    at emitTwo (events.js:106:13)
    at ChildProcess.emit (events.js:194:7)
    at Process.ChildProcess._handle.onexit (internal/child_process.js:208:12)

@gireeshpunathil
Copy link
Member Author

ref: #12288

@mscdex mscdex added test Issues and PRs related to the tests. windows Issues and PRs related to the Windows platform. labels May 4, 2017
@gireeshpunathil
Copy link
Member Author

gireeshpunathil commented May 4, 2017

This patch

@@ -548,7 +548,7 @@ exports.nodeProcessAborted = function nodeProcessAborted(exitCode, signal) {
   // On Windows, v8's base::OS::Abort triggers an access violation,
   // which corresponds to exit code 3221225477 (0xC0000005)
   if (exports.isWindows)
-    expectedExitCodes = [3221225477];
+    expectedExitCodes = [3221225477, 3];

   // When using --abort-on-uncaught-exception, V8 will use
   // base::OS::Abort to terminate the process.

resolves the issue. If experts agree, I can fully test and bring it through a PR.

@Trott
Copy link
Member

Trott commented May 4, 2017

/cc @misterdjules

@misterdjules
Copy link

The change looks good to me.

We might want to add a comment that an exit code of 3 is the expected exit code for a program that terminates by calling the C runtime library's abort(), compared to a program that terminates by calling V8's base::OS::Abort which can trigger different behaviors depending on the compiler and other things.

We could add a link to the documentation of the abort function on the MSDN too, but maybe that's overkill and that link could change.

Basically anything that doesn't make people wonder where the value 3 comes from is good.

@refack
Copy link
Contributor

refack commented Jun 4, 2017

Fixed by #12856

@refack refack closed this as completed Jun 4, 2017
@refack refack removed their assignment Oct 24, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test Issues and PRs related to the tests. windows Issues and PRs related to the Windows platform.
Projects
None yet
Development

No branches or pull requests

5 participants