async_hooks: simplify fatalError #14051

AndreasMadsen · 2017-07-03T09:05:16Z

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)

async_hooks

Simplify fatalError by implementing it like ClearFatalExceptionHandlers.

On my Mac I now get a SIGILL instead of SIGABRT. Very odd.

/cc @nodejs/async_hooks

Implement fatalError like ClearFatalExceptionHandlers.

AndreasMadsen · 2017-07-03T09:28:13Z

Let's see how platform specific that SIGILL issue is.

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

AndreasMadsen · 2017-07-03T10:13:04Z

Looks like the SIGILL issue isn't Mac specific. It is very odd.

refack · 2017-07-03T13:53:42Z

lib/async_hooks.js

@@ -60,19 +60,11 @@ async_wrap.setupHooks({ init,
                        destroy: emitDestroyN });

 // Used to fatally abort the process if a callback throws.
-function fatalError(e) {


Why is everyone trying to kill this 😨
I like it as a way to bail from a callback or a Promise.catch
Ref: #13839 (comment)

Just because it is useful for Promise.catch doesn't mean that is useful here.

Probably. I'll copy it to test/common/

Letting an error bubble with try {} finally {} have always been the standard way of handling throws.

Except what async_hooks is doing isn't standard. This is a guard to prevent an abort from C++ if it detects stack corruption that can likely result from the user try catching/Promise wrapping the call, and to get useful core dumps. It also doesn't follow how this is handled in C++ (https://github.com/nodejs/node/blob/v8.1.3/src/env-inl.h#L144-L160). I'm a big -1 on this.

By "exporting FatalException" do you mean expose it to JS?

If anything I'd like to export the code in Environment::AsyncHooks::pop_ids() and use that as the uniform way to exit the process.

EDIT: Except it should show a JS stack instead of a C++ stack.

By "exporting FatalException" do you mean expose it to JS?

Yes. At the very least I would like a unified way of making a fatal exception. Currently, there are a couple of different implementations floating around in nodecore. It makes things like --abort-on-uncaught-exception difficult to test.

@AndreasMadsen

At the very least I would like a unified way of making a fatal exception.

Sounds good to me.

So area this is currently a WIP?

AndreasMadsen · 2017-07-03T14:06:27Z

I have reduced the SIGILL failture to:

const assert = require('assert');
const { spawnSync } = require('child_process');

const child = spawnSync(process.execPath, [
  '--abort-on-uncaught-exception', '-e', 'throw new Error();'
]);
assert.strictEqual(child.signal, 'SIGABRT');
assert.strictEqual(child.code, null);

AndreasMadsen · 2017-07-03T19:44:59Z

Blocked by #14060

trevnorris · 2017-07-03T21:37:01Z

lib/async_hooks.js

-      }
+  for (var i = 0; i < active_hooks_array.length; i++) {
+    if (typeof active_hooks_array[i][before_symbol] === 'function') {
+      active_hooks_array[i][before_symbol](asyncId);


Allowing an exception to bubble then be handled by a Promise or similar means that it's possible that not all hook callbacks execute, but users that have those unexecuted callbacks won't know that it hasn't been called. Consider the following:

const map = new Map(); createHook({ before(id) { map.set(id, /* some object */) }, after(id) { map.delete(id) }, }).enable();

You've just introduced a vector for a memory leak.

BTW If you're touching this you could use for...of (supposed to be tiny bit faster on TF&I)
Ref: #13419 (comment)

BridgeAR · 2017-07-09T13:17:59Z

lib/async_hooks.js

-  if (async_hook_fields[kAfter] > 0)
-    emitAfterN(asyncId);
+  if (async_hook_fields[kAfter] > 0) {
+    // Remove prepear for a fatal exception in case an error occurred


s/prepear/prepare

@AndreasMadsen is the comment a TODO? Otherwise I'm not sure I understand what It means.

BridgeAR · 2017-07-09T13:18:05Z

lib/async_hooks.js

  }

  pushAsyncIds(asyncId, triggerAsyncId);

  if (async_hook_fields[kBefore] === 0)
    return;
-  emitBeforeN(asyncId);
+
+  // Remove prepear for a fatal exception in case an error occurred


s/prepear/prepare

refack · 2017-07-09T22:49:52Z

lib/async_hooks.js

-  if (async_hook_fields[kAfter] > 0)
-    emitAfterN(asyncId);
+  if (async_hook_fields[kAfter] > 0) {
+    // Remove prepear for a fatal exception in case an error occurred


@AndreasMadsen is the comment a TODO? Otherwise I'm not sure I understand what It means.

refack · 2017-07-09T23:00:26Z

lib/async_hooks.js

-      }
+  for (var i = 0; i < active_hooks_array.length; i++) {
+    if (typeof active_hooks_array[i][before_symbol] === 'function') {
+      active_hooks_array[i][before_symbol](asyncId);


BTW If you're touching this you could use for...of (supposed to be tiny bit faster on TF&I)
Ref: #13419 (comment)

refack · 2017-07-09T23:06:58Z

lib/async_hooks.js

+    init(asyncId, type, triggerAsyncId, resource);
+    didThrow = false;
+  } finally {
+    if (didThrow) clearFatalExceptionHandlers();


I'm not sure it's correct to move this line from the N variant to the S variant?

refack · 2017-07-09T23:22:04Z

@trevnorris just for explicitness what's the meaning of the S and N suffixes? I'm assuming N is for Native... What's the S?

AndreasMadsen · 2017-07-10T06:36:46Z

@refack As I tried to tell @BridgeAR, there is no point reviewing this as it is now. As we need to do something drastically different anyway.

What's the S?

I think it is for Script. I plan to make a rename PR in the near future.

AndreasMadsen · 2017-07-10T06:44:00Z

Clearly, there is a lot of confusion so I will just close this PR.

trevnorris · 2017-07-12T20:35:03Z

I think it is for Script. I plan to make a rename PR in the near future.

Yup, that's it. Thanks for the PR to make it more explicit.

async_hooks: simplify fatalError

17aafd7

Implement fatalError like ClearFatalExceptionHandlers.

nodejs-github-bot added the async_hooks Issues and PRs related to the async hooks subsystem. label Jul 3, 2017

AndreasMadsen added dont-land-on-v4.x labels Jul 3, 2017

AndreasMadsen added 2 commits July 3, 2017 12:42

[squash] use try finally, for better stack traces

278b163

[squash] ensure that uncaughtException listeners are removed

610d365

AndreasMadsen mentioned this pull request Jul 3, 2017

async_hooks: move statement #14054

Closed

4 tasks

refack reviewed Jul 3, 2017

View reviewed changes

AndreasMadsen added 2 commits July 3, 2017 21:42

[squash] async_hooks c++ already have a try finally for each hook

1d3f69b

[squash] add frindly comment explaining _events interaction

527a4f6

trevnorris reviewed Jul 3, 2017

View reviewed changes

BridgeAR requested changes Jul 9, 2017

View reviewed changes

refack reviewed Jul 9, 2017

View reviewed changes

AndreasMadsen added the wip Issues and PRs that are still a work in progress. label Jul 10, 2017

AndreasMadsen closed this Jul 10, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

async_hooks: simplify fatalError #14051

async_hooks: simplify fatalError #14051

AndreasMadsen commented Jul 3, 2017

AndreasMadsen commented Jul 3, 2017

AndreasMadsen commented Jul 3, 2017

refack Jul 3, 2017 •

edited

Loading

AndreasMadsen Jul 3, 2017

refack Jul 3, 2017

AndreasMadsen Jul 3, 2017

trevnorris Jul 3, 2017

trevnorris Jul 3, 2017

trevnorris Jul 3, 2017 •

edited

Loading

AndreasMadsen Jul 4, 2017

trevnorris Jul 5, 2017

refack Jul 9, 2017

AndreasMadsen commented Jul 3, 2017 •

edited

Loading

AndreasMadsen commented Jul 3, 2017

trevnorris Jul 3, 2017

refack Jul 9, 2017

BridgeAR Jul 9, 2017

refack Jul 9, 2017

BridgeAR Jul 9, 2017

refack Jul 9, 2017

refack Jul 9, 2017

refack Jul 9, 2017

refack commented Jul 9, 2017

AndreasMadsen commented Jul 10, 2017 •

edited

Loading

AndreasMadsen commented Jul 10, 2017

trevnorris commented Jul 12, 2017

async_hooks: simplify fatalError #14051

async_hooks: simplify fatalError #14051

Conversation

AndreasMadsen commented Jul 3, 2017

Checklist

Affected core subsystem(s)

AndreasMadsen commented Jul 3, 2017

AndreasMadsen commented Jul 3, 2017

refack Jul 3, 2017 • edited Loading

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

Choose a reason for hiding this comment

trevnorris Jul 3, 2017 • edited Loading

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

AndreasMadsen commented Jul 3, 2017 • edited Loading

AndreasMadsen commented Jul 3, 2017

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

refack commented Jul 9, 2017

AndreasMadsen commented Jul 10, 2017 • edited Loading

AndreasMadsen commented Jul 10, 2017

trevnorris commented Jul 12, 2017

refack Jul 3, 2017 •

edited

Loading

trevnorris Jul 3, 2017 •

edited

Loading

AndreasMadsen commented Jul 3, 2017 •

edited

Loading

AndreasMadsen commented Jul 10, 2017 •

edited

Loading