similar to the other comment, this would read better as MarkAsDisposed().

@jasnell @trevnorris This isn't called for disposed domains.

Won't this only work if the class instance is wrapped in a v8::HandleScope? Otherwise it's possible the v8::Local<> handle could be GC'd before the instance is closed.

Not sure what you mean by wrapped, but yes, there needs to be a valid HandleScope for the object’s lifetime. That is already ensured because you can’t pass the v8::Local argument to its constructor without one, though.

The lifetime of private_ should be within the same v8::HandleScope, correct? In that case should be able to use RAII and not need to allocate a new instance for this.

The intention here is that userland code won’t have access to the fields of InternalCallbackScope, or become dependent on the class layout ABI-wise.

Semantically, InternalCallbackScope and CallbackScope are identical, and their lifetimes are tied together.

this probably stems from a lack of C++ knowledge, but why does passing Environment* here properly cast to the AsyncCallbackScope* that is callback_scope_?

the AsyncCallbackScope* that is callback_scope_

callback_scope_ is a AsyncCallbackScope, not a pointer – it’s a normal call to its constructor

The assumption here is that env->using_domains() is always true if object_->IsObject(). Mind adding the check above this to make sure that's always the case?

Hm? Not quite seeing that. The reverse assumption is present here, i.e. that if object_->IsObject() is true then env->using_domains() is true, but there’s already a CHECK for that, just below this line in the diff.

That assumption is already found in the previous code, and I do find it odd. I think we should either always or never do a CHECK(object->IsObject()); (but that would be semver-major).

Wish I could go back and take a screenshot of what I was seeing b/c the code I thought I wrote this comment for isn't there anymore. Nm me.

Haha! I found it. Was from one of your previous commits:

+InternalCallbackScope::InternalCallbackScope(Environment* env,
+                                             Local<Value> object,
+                                             const async_context& asyncContext)
+  : env_(env),
+    async_context_(asyncContext),
+    object_(object->IsObject() ? object.As<Object>() : Local<Object>()),
+    callback_scope_(env) {

(I was specifically referring to assigning object_. Since that's no longer there, no need to think about it. :)

Ah, right. :)

The name failed_ doesn't properly represent the return value for DomainEnter(). If the domain callback truly failed then the application will FatalError(). A more appropriate name for this would be disposed_, or similar.

But, like MakeCallback, this has a lot more failure modes than just trying to enter a disposed domain? There are other cases in which failed_ is set and in which MakeCallback previously bailed out.

Like, if you have suggestions that make sense across the board, I'm all for it. Or we could split this into different boolean values, but that might unnecessarily increase complexity.

what are the states? would a simple bit mask with a couple flags suffice?

@jasnell Currently and previously, those are:

• Domain enter/exit callback was acting on a disposed domain
• The invoked callback itself threw
• Calling process.nextTick() afterwards threw

would a simple bit mask with a couple flags suffice?

@jasnell Just to be clear, we would be setting different bits in a bit mask, but would never inspect those values, only care whether one was set. Am I understanding correctly that that's what you're suggesting?

So failed_ handles all failure cases. I see.

The reason the domain callbacks wrap the async wrap callbacks is that if the domain is disposed then MakeCallback() can return immediately. There's no cleanup that needs to be performed. Though I guess because of the way the code is now structured that fact does need to propagate.

I don't see how pushed_ids_ in Close() could be false. Mind clarifying?

pushed_ids_ is only true if the InternalCallbackScope constructor finishes, but that doesn’t happen if DomainEnter() reported that we’re trying to enter a disposed domain.

I have a patch for an error here. Will get you the commit later today.

Not sure what to say here, you can feel free to post the patch or let me know what the problem is (and whether it’s a new one).

sorry. that comment isn't a blocker. was a self-reminder b/c i thought the fix depended on this PR.

Note to look at later: This may need to go before the pushed_ids_ check because the call to _fatalExpression will automatically call all the after() callbacks and drain the queue.

This matches the previous behaviour, and I think that’s okay because you already explicitly accounted for that situation in pop_ids().

Looks like DomainEnter/DomainExit are both still defined in src/async-wrap.cc. Since they're now only used in src/node.cc mind moving them over?

this is purely my personal opinion of having balance between what runs entering and exiting the scope, but I feel that from here down should be put into it's own method (e.g. PostProcessing()) then at the end of InternalMakeCallback() do the IsInnerMakeCallback() check after scope.Close() then run it.

again, this isn't a technical issue, but it jumps at me every time I see it.

I’m not 100 % following – you mean, move this from scope.Close() to InternalMakeCallback()? That would defeat the whole purpose of exposing CallbackScope, though…

Question: why is this correct? the stack unwinds completely?

Yup, this is only executed for the bottom-most callback scope.

I question if this is necessary anymore if we're returning a MaybeLocal<>. The only time Undefined() is returned is in the case of InternalMakeCallback() being wrapped in a TryCatch, and is not rethrown. In which case I'd assume the caller would be planning on handling the error anyway.

It may be considered a semver-major, but looks like always returning ret would simplify code elsewhere.

The only time Undefined() is returned is in the case of InternalMakeCallback() being wrapped in a TryCatch, and is not rethrown.

Why is that?

It may be considered a semver-major, but looks like always returning ret would simplify code elsewhere.

Yes, MakeCallback should always return an empty MaybeLocal in case of a thrown exception, but yes, that would be up to a semver-major follow-up PR.

The only time Undefined() is returned is in the case of InternalMakeCallback() being wrapped in a TryCatch, and is not rethrown.

Why is that?

That's what testing has shown. c++ file:

double zid = 41;

static void Run(const v8::FunctionCallbackInfo<v8::Value>& args) {
  auto isolate = args.GetIsolate();
  auto fn = args[0].As<Function>();
  auto obj = args[1].As<Object>();

  MaybeLocal<Value> ret =
    node::MakeCallback(isolate, obj, fn, 0, nullptr, { ++zid, ++zid });
  fprintf(stderr, "IsEmpty(): %s\n", ret.IsEmpty() ? "true" : "false");
}

js file:

const release_type = process.config.target_defaults.default_configuration;
const addon = require(`./build/${release_type}/addon`);
const async_hooks = require('async_hooks');
const print = (...a) => require('fs').writeSync(1, require('util').format(...a));


function a() {
  print(`a id: ${async_hooks.executionAsyncId()}   ret: ${addon.run(b, {})}\n`);
}
function b() {
  print(`b id: ${async_hooks.executionAsyncId()}   ret: ${addon.run(c, {})}\n`);
}
function c() {
  print(`c id: ${async_hooks.executionAsyncId()}\n`);
  throw new Error('crap');
}

addon.run(a);

output:

$ node run.js
c id: 46
IsEmpty(): true
IsEmpty(): true
IsEmpty(): true
/var/projects/tmp/node-basic-module/run.js:17
  throw new Error('crap');
  ^

Error: crap

And actually, even if the call is wrapped in a TryCatch the first return is still an empty handle. Basically, I can't figure out a scenario where the callback throws and an empty handle isn't returned automatically.

Basically, I can't figure out a scenario where the callback throws and an empty handle isn't returned automatically.

When there is no JS stack below the call ;) #14922 adds a test for it.