promises prevent domain propagation

[vkurchatkin]

Test case:

var domain = require('domain').create();

function asyncStuff(cb) {
  new Promise(function(resolve) {
    setImmediate(resolve);
  }).then(cb);
}

domain.run(function() {

  asyncStuff(function() {
    setImmediate(function() {
      throw new Error;
    });
  });

});

domain.on('error', function() {
  console.log('caught');
});

[vkurchatkin]

This is not really a problem when using bare promises as they handle errors themselves. The problem arises when a callback-style interface is implemented with promises under the hood. If such a function is exported by a third party module, the result will be totally unexpected as implementation details are unknown to the user.

One of possible solutions is to instruct package authors to bind callback to current domain if they use promises as a part of implementation:

function asyncStuff(cb) {
  new Promise(function(resolve) {
    setImmediate(resolve);
  }).then(process.domain ? process.domain.bind(cb) : cb);
}

[city41]

For what it's worth, when swapping the new native promises (using Node 0.11.13) out for bluebird, then the domain error event handler fires as expected. When using native Promises, the error is uncaught

[othiym23]

I haven't looked too deeply into now V8's native promises are implemented, but it would not surprise me if it turned out to be necessary to patch them to work with domains. Even if the TC decides that domains should be deprecated, if 0.12 is going to ship with native Promises, somebody should probably fix them to work with domains. If I have some time (a big if these days), I'll take a look at fixing this.

[vkurchatkin]

Here is a solution (not really) using debug object (node --expose_debug_as=debug):

var domains = {};
debug.Debug.setListener(function (event, state, data) {
  if (event != debug.Debug.DebugEvent.AsyncTaskEvent) return;

  var type = data.type();

  if (type === 'enqueue')
    domains[data.id()] = process.domain;
  else if (type === 'willHandle')
    domains[data.id()].enter();
  else if (type === 'didHandle') {
    domains[data.id()].exit();
    delete domains[data.id()];
  }

});

@othiym23 patching v8 to call node-specific code is probably out of the question

[othiym23]

@vkurchatkin I was anticipating that if it's not something that can be handled in pure JS, it might require making some domain-specific tweaks to the Node C++ layer (which is already done for MakeCallback / MakeDomainCallback). So, not floating a patch on V8, but doing some work in C++. Between the current domain hacks and @trevnorris's work on AsyncWrap, it's not unprecedented.

[vkurchatkin]

@othiym23 unfortunately, we don't have any control of how promise handlers are executed, aside from debug events.

[bnoordhuis]

It's like @vkurchatkin says, promise handlers run from the micro task queue and that's not under node's control.

Another complication is that exceptions thrown from micro tasks are swallowed, they never propagate up the stack. I can think of only two ways to work around that: the debug listener hack or monkey-patching the Promise object.

Neither are very appealing: debug events have a lot of overhead while monkey-patching is a very un-core thing to do (and likely has unforeseen consequences.)

[othiym23]

Either Node core should go ahead and monkeypatch the Promises global (and / or figure out how to assimilate the microtask queue into AsyncWrap / get them working with domains) or just go ahead and start the process of deprecating domains.

I would really like to see there be a solid, generic error-handling facility in Node core, because the current error-handling story is deeply confusing to just about everyone not directly on the Node core team. I agree, though, that at this point, domains may be a bigger problem than the one they're trying to solve. Aside from domains, the best thing we've got is @piscisaureus's work on Zones, and while that's not bad, it's still too limited to be a general-purpose solution (and I think it has this problem with V8's Promises as well).

[vkurchatkin]

I don't think it's possible to monkeypatch native promises, because we need to capture domain on promise creation or resolve/reject call, it's not something you can monkeypatch (global.Promise = require('some-promise-lib') is not a solution)

[piscisaureus]

I'm quite sure that if we are willing to put some effort into it, the v8 team would care and be amenable to our suggestions. Remember that the Promise API is quite new.

@othiym23
It's difficult to define what a better error handling API should do. You have any suggestions?
As for zones, being a better way to handle errors is clearly within scope. I am aware of the Promise issue (and the fact that a Zone should itself be a promise), and I know how they can be integrated. But there are higher priorities first.

[vkurchatkin]

@piscisaureus I propose something like this:

diff --git a/deps/v8/include/v8.h b/deps/v8/include/v8.h
index ef0bda6..27d9839 100644
--- a/deps/v8/include/v8.h
+++ b/deps/v8/include/v8.h
@@ -4170,6 +4170,17 @@ class V8_EXPORT Isolate {
   typedef void (*UseCounterCallback)(Isolate* isolate,
                                      UseCounterFeature feature);

+  enum MicrotaskEventType {
+    kEnqueueMicrotask,
+    kBeforeMicrotask,
+    kAfterMicrotask
+  };
+
+  typedef void (*MicrotaskEventCallback)(Isolate* isolate,
+                                         MicrotaskEventType type, void** data);
+
+  void SetMicrotaskEventCallback(MicrotaskEventCallback callback);
+

   /**
    * Creates a new isolate.  Does not change the currently entered
diff --git a/deps/v8/src/api.cc b/deps/v8/src/api.cc
index 4a63459..6642e39 100644
--- a/deps/v8/src/api.cc
+++ b/deps/v8/src/api.cc
@@ -6688,6 +6688,10 @@ void Isolate::SetUseCounterCallback(UseCounterCallback callback) {
   reinterpret_cast<i::Isolate*>(this)->SetUseCounterCallback(callback);
 }

+void Isolate::SetMicrotaskEventCallback(MicrotaskEventCallback callback) {
+  reinterpret_cast<i::Isolate*>(this)->SetMicrotaskEventCallback(callback);
+}
+

 void Isolate::SetCounterFunction(CounterLookupCallback callback) {
   i::Isolate* isolate = reinterpret_cast<i::Isolate*>(this);
diff --git a/deps/v8/src/isolate.cc b/deps/v8/src/isolate.cc
index 215296d..01641c1 100644
--- a/deps/v8/src/isolate.cc
+++ b/deps/v8/src/isolate.cc
@@ -1474,7 +1474,8 @@ Isolate::Isolate()
       num_sweeper_threads_(0),
       stress_deopt_count_(0),
       next_optimization_id_(0),
-      use_counter_callback_(NULL) {
+      use_counter_callback_(NULL),
+      microtask_event_callback_(NULL) {
   id_ = base::NoBarrier_AtomicIncrement(&isolate_counter_, 1);
   TRACE_ISOLATE(constructor);

@@ -2275,7 +2276,17 @@ void Isolate::EnqueueMicrotask(Handle<Object> microtask) {
     heap()->set_microtask_queue(*queue);
   }
   DCHECK(queue->get(num_tasks)->IsUndefined());
-  queue->set(num_tasks, *microtask);
+
+  void* data = NULL;
+  if (microtask_event_callback_) {
+    microtask_event_callback_(reinterpret_cast<v8::Isolate*>(this), v8::Isolate::kEnqueueMicrotask, &data);
+  }
+
+  Handle<FixedArray> task = factory()->NewFixedArray(2);
+  Handle<Object> data_object = v8::FromCData(this, data);
+  task->set(0, *microtask);
+  task->set(1, *data_object);
+  queue->set(num_tasks, *task);
   set_pending_microtask_count(num_tasks + 1);
 }

@@ -2303,15 +2314,29 @@ void Isolate::RunMicrotasks() {
     for (int i = 0; i < num_tasks; i++) {
       HandleScope scope(this);
       Handle<Object> microtask(queue->get(i), this);
-      if (microtask->IsJSFunction()) {
+      if (microtask->IsFixedArray()) {
+        Handle<FixedArray> microtask_record = Handle<FixedArray>::cast(microtask);
+        Handle<Object> microtask_function_obj(microtask_record->get(0), this);
         Handle<JSFunction> microtask_function =
-            Handle<JSFunction>::cast(microtask);
+            Handle<JSFunction>::cast(microtask_function_obj);
+        Handle<Object> data_object(microtask_record->get(1), this);
+        void* data = v8::ToCData<void*>(*data_object);
+
+        if (microtask_event_callback_) {
+          microtask_event_callback_(reinterpret_cast<v8::Isolate*>(this), v8::Isolate::kBeforeMicrotask, &data);
+        }
+
         SaveContext save(this);
         set_context(microtask_function->context()->native_context());
         Handle<Object> exception;
         MaybeHandle<Object> result = Execution::TryCall(
             microtask_function, factory()->undefined_value(),
             0, NULL, &exception);
+
+        if (microtask_event_callback_) {
+          microtask_event_callback_(reinterpret_cast<v8::Isolate*>(this), v8::Isolate::kAfterMicrotask, &data);
+        }
+
         // If execution is terminating, just bail out.
         if (result.is_null() &&
             !exception.is_null() &&
@@ -2346,6 +2371,11 @@ void Isolate::CountUsage(v8::Isolate::UseCounterFeature feature) {
   }
 }

+void Isolate::SetMicrotaskEventCallback(v8::Isolate::MicrotaskEventCallback callback) {
+  DCHECK(!microtask_event_callback_);
+  microtask_event_callback_ = callback;
+}
+

 bool StackLimitCheck::JsHasOverflowed() const {
   StackGuard* stack_guard = isolate_->stack_guard();
diff --git a/deps/v8/src/isolate.h b/deps/v8/src/isolate.h
index 9ef6fc7..bb0968f 100644
--- a/deps/v8/src/isolate.h
+++ b/deps/v8/src/isolate.h
@@ -1099,6 +1099,7 @@ class Isolate {

   void EnqueueMicrotask(Handle<Object> microtask);
   void RunMicrotasks();
+  void SetMicrotaskEventCallback(v8::Isolate::MicrotaskEventCallback callback);

   void SetUseCounterCallback(v8::Isolate::UseCounterCallback callback);
   void CountUsage(v8::Isolate::UseCounterFeature feature);
@@ -1326,6 +1327,8 @@ class Isolate {

   v8::Isolate::UseCounterCallback use_counter_callback_;

+  v8::Isolate::MicrotaskEventCallback microtask_event_callback_;
+
   friend class ExecutionAccess;
   friend class HandleScopeImplementer;
   friend class IsolateInitializer;
diff --git a/src/node.cc b/src/node.cc
index db22ea9..deb394f 100644
--- a/src/node.cc
+++ b/src/node.cc
@@ -932,6 +932,42 @@ void SetupAsyncListener(const FunctionCallbackInfo<Value>& args) {
       FIXED_ONE_BYTE_STRING(args.GetIsolate(), "_setupAsyncListener"));
 }

+void MicrotaskEventCb(Isolate* isolate, v8::Isolate::MicrotaskEventType type, void** data) {
+  Environment* env = Environment::GetCurrent(isolate);
+  HandleScope handle_scope(isolate);
+
+  if (type == v8::Isolate::kEnqueueMicrotask) {
+    if (env->in_domain()) {
+      Handle<Object> domain = env->domain_array()->Get(0).As<Object>();
+      *data = new BaseObject(env, domain);
+    }
+    return;
+  }
+
+  if (!*data) return;
+
+  BaseObject* domain_wrap = static_cast<BaseObject*>(*data);
+  Handle<Object> domain = domain_wrap->object();
+
+
+  if (type == v8::Isolate::kBeforeMicrotask) {
+    Local<Function> enter =
+      domain->Get(env->enter_string()).As<Function>();
+
+    if (enter->IsFunction()) {
+      enter->Call(domain, 0, NULL);
+    }
+  } else {
+    Local<Function> exit =
+      domain->Get(env->exit_string()).As<Function>();
+
+    if (exit->IsFunction()) {
+      exit->Call(domain, 0, NULL);
+    }
+
+    domain_wrap->persistent().Reset();
+  }
+}

 void SetupDomainUse(const FunctionCallbackInfo<Value>& args) {
   Environment* env = Environment::GetCurrent(args.GetIsolate());
@@ -967,6 +1003,8 @@ void SetupDomainUse(const FunctionCallbackInfo<Value>& args) {
       kExternalUint32Array,
       domain_flag->fields_count());

+  env->isolate()->SetMicrotaskEventCallback(MicrotaskEventCb);
+
   // Do a little housekeeping.
   env->process_object()->Delete(
       FIXED_ONE_BYTE_STRING(args.GetIsolate(), "_setupDomainUse"));

pretty sketchy, but seems to work

/cc @bnoordhuis @othiym23 @trevnorris

[bnoordhuis]

@vkurchatkin It's not a bad approach but I suggest you discuss it with the V8 people first. One obvious issue is that you add an extra allocation in Isolate::EnqueueMicrotask() and that's a hot function. (Or it can be, when you use observers.)

[piscisaureus]

I think I would prefer an approach where v8 doesn't call micro-tasks itself. Instead I'd like them to just call some c++ callback (as specified by the embedder) when the microtask is scheduled, and then let the embedder take care of actually calling the microtask.

Later addition: the reason is that @vkurchatkin's approach, while okay, looks a lot like the old async-listener. But we don't know what async-listener will become in the future, and it'd be not so great if we have to go back to the v8 team by that time and request yet another api change.

[trevnorris]

I think the fact that all errors are swallowed by the micro-task queue makes anything short of monky-patching V8 impossible. Any upstream changes to V8 definitely aren't going to land any time soon. Even async listener will suffer from the same. Since _fatalException() won't even run if a Promise throws.

In short, I'm not sure. Monkey-patching seems like an horrible idea, but I don't see any other solutions.

[bnoordhuis]

A change was landed today in v8/v8@cb0694e that notifies the embedder of uncaught exceptions in Object.observe() callbacks. It doesn't do quite what we want but maybe it can function as a starting point.

(Caveat emptor: the tests use V8::AddMessageListener() but that's been superseded by Isolate::AddMessageListener().)

[vkurchatkin]

@trevnorris promises don't throw exceptions out of a microtask, so no problem here. At some point we need to integrate Isolate::SetPromiseRejectCallback, but that is whole other story.

[trevnorris]

@vkurchatkin Seems that API doesn't exist in 3.28. We'll have to upgrade to 3.29 to support that.

[vkurchatkin]

@piscisaureus haven't seen your update. This is intentional, eventually this should work with async listeners as well. I hoped that @trevnorris could comment

[trevnorris]

@vkurchatkin Let's address async listener support after things get working with the code we directly control. :)

[ajklein]

@vkurchatkin Hi there, @rafaelw pointed me at this thread. From a v8 point-of-view #8648 (comment) looks pretty invasive. If Node wants to have this much control over execution, I tend to lean towards something more like @piscisaureus's suggestion of letting the embedder get a callback every time a task is enqueued, though that may not be a good long-term solution as ECMAScript's execution model gets more fleshed out in ES7.

But I know little about Node, so it's possible that there's some way to support what Domains need without a change to V8's API, which would of course be the most ideal from my point of view.

[vkurchatkin]

@ajklein I was thinking about abstract class (MicrotaskQueue or JobQueue or something like that) that can be implemented to handle all microtask related operations. v8 would provide default implementation which could be overriden per isolate.

[trevnorris]

@vkurchatkin Still isn't there a problem that V8's Microtask runner swallows all errors?

[vkurchatkin]

@trevnorris it's not a problem for promises since they don't throw inside of microtasks

[roberttaylortech]

Hoping someone will take note and check out the issues I referenced -- and add anything useful please! :)

• Execution using/within node domains not properly supported, partial fix implemented here in? tj/co#239
• My long post here: deprecate domains node#66
• Session middleware fix: Context gets lost in Express after using session middleware othiym23/node-continuation-local-storage#29

[ORESoftware]

+1 this seems to still be a problem

[syrnick]

How about removing domains in node 7?

With the current state of affairs domains are absolutely unusable. It's impossible to guarantee that a library that you use doesn't use native promises somewhere deep inside and breaks the domain propagation. Unless you do something like global.Promise = require("bluebird"); and the library doesn't find a different way to grab the native promise constructor. As native promises make its way into the user-facing methods like nodejs/node#5020, it'll be impossible to guarantee that no code uses native promises.

[the1mills]

Has anyone successfully monkey patched native promises to work with domains?

[ORESoftware]

this works:

const then = Promise.prototype.then;
Promise.prototype.then = function (fn1, fn2) {

  if (process.domain) {
    fn1 = fn1 && process.domain.bind(fn1);
    fn2 = fn2 && process.domain.bind(fn2);
  }

  return then.call(this, fn1, fn2);
};

process.on('uncaughtException', function(err){
    console.log(' This is uncaught => ', err);
});


const domain = require('domain');

const d = domain.create();

d.on('error', function(err){
  console.error(' => Domain caught => ',err);
});


d.run(function(){

  Promise.resolve().then(function(){
     process.nextTick(function(){
       throw new Error('rah');
     });

  });

});

be curious if there's more we should patch though

[ChrisCinelli]

Thanks @ORESoftware.

This seems to fix it for me:

const then = Promise.prototype.then;
Promise.prototype.then = function (fn1, fn2) {

  if (process.domain) {
    fn1 = fn1 && process.domain.bind(fn1);
    fn2 = fn2 && process.domain.bind(fn2);
  }

  return then.call(this, fn1, fn2);
};

But what about the catch and the constructor?

[ORESoftware]

@ChrisCinelli sure thing, let me know if you see anything wrong with that patch, it seems to work fine, but ya never know. By the way I think there is something in the works at Node core to achieve this at more granular level - in V8 itself - nodejs/node#12489

[the1mills]

I think in the catch and constructor you'd do essentially the same thing, let me whip something up.

[ORESoftware]

Yeah Promise.prototype.catch should be straightforward, but patching the Promise constructor is a bit trickier (Promise.resolve needs to work too, etc.). I am not sure if touching the constructor is a great idea but I will experiment.

const katch = global.Promise.prototype.catch;
global.Promise.prototype.catch = function (fn1) {
  if (process.domain) {
    fn1 = fn1 && process.domain.bind(fn1);
  }
  return katch.call(this, fn1);
};

Bluebird Promises should already propagate domains, so I don't believe there is a need to patch Promise = require('bluebird');... I have always been surprised that Bluebird chose to propagate domains considering how performance focused Bluebird has always been...weird.

The one thing that's sort of caveat here is that process.domain.bind() doesn't seem to complain if the argument is not a function, which is strange.

[ORESoftware]

An apparent leaky Promise - to model a monkey patch after https://github.com/nodejs/node/blob/ca480719199d2ff38223aff8e301aced25d7e6f1/deps/v8/test/mjsunit/regress/regress-5476.js

[ORESoftware]

For the constructor, you could try this:

const PromiseConstructor = global.Promise;
global.Promise = class PatchedPromise extends PromiseConstructor {
  constructor(executor) {
    if (process.domain) {
      executor = executor && process.domain.bind(executor);
    }
    super(executor); // call native Promise constructor
  }
};

and transpile it how you will/must. Note that the vanilla JS version works, but if you try to transpile the TypeScript version of this - there is a bug - microsoft/TypeScript#15294 (comment)

[ORESoftware]

Maybe I am incorrect (someone please correct me), but it seems like to get domains to work with promises, it's enough to do this:

process.on('unhandledRejection', (reason: any, p: Promise<any>) => {

  if (p && p.domain) {
      p.domain.emit('error', reason);
      return;
  }

  if(process.domain){
      process.domain.emit('error', reason);
      return;
   }

    // oh well, do something else
});

It appears that only on recent versions of Node.js, will promises have a domain pinned to them, so the first if statement will only evaluate to true on newer versions. But process.domain should always work anyway?