async-wrap: no way to catch errors without changing the throw origin

[AndreasMadsen]

Consider some long stack trace module. It can use async_wrap to manage the the callSite objects correctly and use the v8 Error hooks to modify the .stack property. However because v8 sets the .stack property in a lazy way, it is necessary to do a try {} finally {} around the callback.

See https://github.com/AndreasMadsen/trace/blob/master/trace.js#L53 for an example with the tracing module.

An okay solution solution is to use the uncaughtException event like this:

var asyncWrap = process.binding('async_wrap');

// Enable asyncWrap and call init hook function on async initialization
var asyncHooksObject = {};
var kCallInitHook = 0;
asyncWrap.setupHooks(
  asyncHooksObject,
  asyncFunctionInitialized,
  asyncCallbackBefore,
  asyncCallbackAfter);
asyncHooksObject[kCallInitHook] = 1;

function asyncFunctionInitialized() {}

function asyncCallbackBefore() {
  process.once('uncaughtException', asyncCallbackError);
}

function asyncCallbackError(error) {
  // Set stack by v8 magic
  error.stack;

  // changes throw origin, should not be necessary
  throw error;
}

function asyncCallbackAfter() {
  process.removeListener('uncaughtException', asyncCallbackError);
}

setTimeout(function () {
  badluck();
}, 10);

However this changes the throw origin:

/Users/Andreas/Sites/node_modules/trace/test.js:26
  throw error;
        ^
ReferenceError: badluck is not defined
    at null._onTimeout (/Users/Andreas/Sites/node_modules/trace/test.js:34:3)
    at Timer.listOnTimeout (timers.js:90:15)

If uncaughtException isn't used then this is the error:

/Users/Andreas/Sites/node_modules/trace/test.js:34
  badluck();
  ^
ReferenceError: badluck is not defined
    at null._onTimeout (/Users/Andreas/Sites/node_modules/trace/test.js:34:3)
    at Timer.listOnTimeout (timers.js:90:15)

This is much more informative. It would be really nice if async_wrap or some other mechanism allowed something similar to the old tracing.addAsyncListener({ error: handler }) behaviour. Such that the throw origin can be preserved.

issue tracking: AndreasMadsen/trace#12
issue tracking: nodejs/diagnostics#7

[trevnorris]

It is on my list to reimplement the error handling that previously existed in async listener. It was removed for expediency of landing the what was already massive patch. I've assigned to myself and will prioritize finishing this implementation.

[trevnorris]

Do you want to be able to recover from these errors, or just be notified of them on the way to crashing?

[AndreasMadsen]

In the long-stack-trace tool I never want to recover from the error. However I think it would be best if given an error one could choose to recover or just be notified (and let it propagate to uncaughtException).

I don't think it is necessary to know what handle that called the callback (like in the old tracing module). That can be determined with the pre and post from async_wrap. That being said, it is an option.

The basic idea is really just:

diff --git a/src/node.js b/src/node.js
index 7cfd2c0..90cb446 100644
--- a/src/node.js
+++ b/src/node.js
@@ -214,7 +214,11 @@
     process._fatalException = function(er) {
       var caught;

-      if (process.domain && process.domain._errorHandler)
+      if (process.officalFatalExceptionHandler) {
+        caught = process.officalFatalExceptionHandler(er);
+      }
+
+      if (!caught && process.domain && process.domain._errorHandler)
         caught = process.domain._errorHandler(er) || caught;

       if (!caught)

Though it might need to be more modular so that more handlers can be attached.

[trevnorris]

I'm fine with starting out by allowing to be notified without the option to recover. That would be a relatively easy PR to land, and without much fuss.

As for error recovery, we'll need to do more exploration as to what that entails and what problems that may cause. I don't want it to simply sit in the same basket as uncaughtException where we basically tell users not to use it.

[AndreasMadsen]

I'm totally fine with just getting the notification. I personally don't have any good use case for error recovery, maybe that would be a good agenda item for the tracing-wg.

[trevnorris]

+1. and I can start on the notification PR.

[Qard]

Recovery is probably more post-mortem than tracing, but I can think of a few people that'd be interested in discussing it. I know @piscisaureus probably has some thoughts related to https://github.com/strongloop/zone.

[Fishrock123]

@trevnorris what's the status of this?

[AndreasMadsen]

We are waiting for use cases from actual users.

[Qard]

As an APM provider, I don't care about recovery, just intercepting and reporting. There's an actual user case from me. 😸

[trevnorris]

@Qard So a callback that sits just below unhandledException that allows you to see the error, but not do anything about it, is about what you're looking for?

[Qard]

In an ideal world, I'd like to be able to spot and read the stack string of errors thrown in the system, but let them keep flowing upwards. Even a native-only interface would be fine with me, if it meant the possibility of not interfering with post-mortem debugging. I don't want to prevent the process from crashing when a crash is expected.

[bnoordhuis]

Is this still relevant? If not, can we close?

[AndreasMadsen]

I think we reached the conclusion that it isn't a async_wrap problem, we simply need a try-finally version of uncaughtException.

[bnoordhuis]

Okay, thanks. Does it need to stay open?

[AndreasMadsen]

We don't have an issue for the "try-finally version of uncaughtException", so either create that or keep this open.

[trevnorris]

I'd say open an issue to support try-finally. It's still possible a third party native module could call Function::Call() directly w/o a stack over it, and so async hooks wouldn't be able to observe the exception.

[jasnell]

Closing due to lack of additional follow up in the past 2 years