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/async-hooks/test-callback-error.js always produces core file #29286

Closed
trevnorris opened this issue Aug 23, 2019 · 13 comments
Closed

test/async-hooks/test-callback-error.js always produces core file #29286

trevnorris opened this issue Aug 23, 2019 · 13 comments
Labels
async_hooks Issues and PRs related to the async hooks subsystem. linux Issues and PRs related to the Linux platform. test Issues and PRs related to the tests.

Comments

@trevnorris
Copy link
Contributor

trevnorris commented Aug 23, 2019

  • Version: master (f39ad8a), v12.x (b4e670d) back to v12.0.0
  • Platform: All
  • Subsystem: async_hooks, test

The test test/async-hooks/test-callback-error.js always produces a core file when run.

Example reproduction:

$ ls core
ls: cannot access 'core': No such file or directory
$ ./node test/async-hooks/test-callback-error.js
start case 1
end case 1: 74.522ms
start case 2
end case 2: 79.682ms
start case 3
end case 3: 9.263ms
$ ls core
core

Expected: Successful tests should not produce core files.

The issue comes from case 3 above, which runs:

$ ./node test/async-hooks/test-callback-error.js --abort-on-uncaught-exception test_callback_abort

The problem is test_callback_abort always has an uncaught exception. Here I've changed the test to always print the output from case 3 child's stderr:

$ ./node test/async-hooks/test-callback-error.js
start case 1
end case 1: 51.322ms
start case 2
end case 2: 47.906ms
start case 3
end case 3: 12.985ms
Error: test_callback_abort
    at ActivityCollector.<anonymous> (node/test/async-hooks/test-callback-error.js:27:45)
    at ActivityCollector.oninit node/test/common/index.js:373:15)
    at ActivityCollector._init (node/test/async-hooks/init-hooks.js:192:10)
    at emitInitNative (internal/async_hooks.js:134:43)
    at emitInitScript (internal/async_hooks.js:341:3)
    at new AsyncResource (async_hooks.js:156:7)
    at Object.<anonymous> (node/test/async-hooks/test-callback-error.js:29:5)
    at Module._compile (internal/modules/cjs/loader.js:936:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:947:10)
    at Module.load (internal/modules/cjs/loader.js:790:32)
 1: 0x9837a0 node::Abort() [node]
 2: 0x9edaf9  [node]
 3: 0xb129ad v8::internal::FunctionCallbackArguments::Call(v8::internal::CallHandlerInfo) [/usr/local/bin/node]
 4: 0xb120bb  [node]
 5: 0xb117ed  [node]
 6: 0x123c579  [node]

(note: the above output is from v12.9.0)

Even though, ironically, running the test directly doesn't cause the core file:

$ ./node test/async-hooks/test-callback-error.js --abort-on-uncaught-exception test_callback_abort
assert.js:373
    throw err;
    ^

AssertionError [ERR_ASSERTION]: The expression evaluated to a falsy value:

  assert.ok(!arg)

    at Object.<anonymous> (node/test/async-hooks/test-callback-error.js:34:8)
    at Module._compile (internal/modules/cjs/loader.js:936:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:947:10)
    at Module.load (internal/modules/cjs/loader.js:790:32)
    at Function.Module._load (internal/modules/cjs/loader.js:703:12)
    at Function.Module.runMain (internal/modules/cjs/loader.js:999:10)
    at internal/main/run_main_module.js:17:11 {
  generatedMessage: true,
  code: 'ERR_ASSERTION',
  actual: false,
  expected: true,
  operator: '=='
}
@juanarbol
Copy link
Member

@trevnorris I could not replicate that behavior, I checked out to f39ad8a91f, re-compiled node bin; no core file generated.

Screen Shot 2019-08-25 at 6 26 09 PM

@Trott
Copy link
Member

Trott commented Aug 25, 2019

I also could not replicate on macOS (but did not try very hard so figured it was likely I was doing something wrong, which may still end up being true....)

@addaleax addaleax added async_hooks Issues and PRs related to the async hooks subsystem. linux Issues and PRs related to the Linux platform. test Issues and PRs related to the tests. labels Aug 25, 2019
@addaleax
Copy link
Member

I think all that we need to do is to move the test to test/abort, right?

Even though, ironically, running the test directly doesn't cause the core file:

It does when the command line args are in the right order, i.e. with ./node --abort-on-uncaught-exception test/async-hooks/test-callback-error.js test_callback_abort instead of ./node test/async-hooks/test-callback-error.js --abort-on-uncaught-exception test_callback_abort it crashes as expected.

I also could not replicate on macOS (but did not try very hard so figured it was likely I was doing something wrong, which may still end up being true....)

I think it should generate a core file everywhere – the process does fail with SIGABRT and the test checks that – assuming ulimit -c and the core dump paths are set accordingly.

@cjihrig
Copy link
Contributor

cjihrig commented Aug 25, 2019

FWIW, I am able to generate a core file on macOS by running the test after ulimit -c unlimited.

@Trott
Copy link
Member

Trott commented Aug 25, 2019

assuming ulimit -c and the core dump paths are set accordingly.

sigh Yep, forgot I had disabled core dumps entirely. Sorry for the noise. You'd never know I've been using Unix-like operating systems for 30 years.

@juanarbol
Copy link
Member

I could work on this with some guidance!

@trevnorris
Copy link
Contributor Author

It does when the command line args are in the right order [...]

@addaleax Thanks for confirming that.

@bnoordhuis
Copy link
Member

@juanarbol I can see two ways of fixing the test:

  1. Move it to test/abort
  2. Change the test to re-execute itself with ulimit -c 0 so it never writes a core dump

My preference is for (2).

grep 'ulimit -c 0' test/ will find you some examples. There's even a helper function for it, common.childShouldThrowAndAbort().

@juanarbol
Copy link
Member

juanarbol commented Sep 4, 2019

@bnoordhuis I've a question, how can I append 'ulimit -c 0 && ' to fork? I tried by changing execPath param, I did not worked; could I use spawn instead? (I really prefer option number 2) Or maybe continue with option 1...

@bnoordhuis
Copy link
Member

@juanarbol see e.g. test/pummel/test-abort-fatal-error.js for an example.

@juanarbol
Copy link
Member

@bnoordhuis Sorry, I could not patch this issue, I do not understand very good child process and async hooks, sorry again; maybe later I'll help with this kind of issues, again, thanks for the help and the examples.

PS: I tried to move to abort, It didn't worked

bnoordhuis added a commit to bnoordhuis/io.js that referenced this issue Sep 6, 2019
The test spawns a subprocess with the `--abort-on-uncaught-exception`
flag and expects it to terminate with a SIGABRT signal.

On systems where core dumps are enabled, that actually generates an
unnecessary core dump. Set `ulimit -c 0` before spawning the subprocess.

Fixes: nodejs#29286
@Trott Trott closed this as completed in 8217990 Sep 9, 2019
@trevnorris
Copy link
Contributor Author

Thank you for the fix.

@bnoordhuis
Copy link
Member

My pleasure, Trevor. :-)

targos pushed a commit that referenced this issue Sep 20, 2019
The test spawns a subprocess with the `--abort-on-uncaught-exception`
flag and expects it to terminate with a SIGABRT signal.

On systems where core dumps are enabled, that actually generates an
unnecessary core dump. Set `ulimit -c 0` before spawning the subprocess.

Fixes: #29286

PR-URL: #29478
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
BridgeAR pushed a commit that referenced this issue Sep 25, 2019
The test spawns a subprocess with the `--abort-on-uncaught-exception`
flag and expects it to terminate with a SIGABRT signal.

On systems where core dumps are enabled, that actually generates an
unnecessary core dump. Set `ulimit -c 0` before spawning the subprocess.

Fixes: #29286

PR-URL: #29478
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
async_hooks Issues and PRs related to the async hooks subsystem. linux Issues and PRs related to the Linux platform. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants