Skip to content

domain: re-implement domain over async_hook #16222

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

domain: re-implement domain over async_hook #16222

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
60 changes: 44 additions & 16 deletions lib/domain.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@

const util = require('util');
const EventEmitter = require('events');
const { inherits } = util;
const { createHook } = require('async_hooks');

// communicate with events module, but don't require that
// module to have to load this one, since this module has
Expand All @@ -48,13 +48,54 @@ Object.defineProperty(process, 'domain', {
}
});

const pairing = new Map();
const asyncHook = createHook({
init(asyncId, type, triggerAsyncId, resource) {
if (process.domain !== null && process.domain !== undefined) {
// if this operation is created while in a domain, let's mark it
pairing.set(asyncId, process.domain);
resource.domain = process.domain;
if (resource.promise !== undefined &&
resource.promise instanceof Promise) {
// resource.promise instanceof Promise make sure that the
// promise comes from the same context
// see https://github.com/nodejs/node/issues/15673
resource.promise.domain = process.domain;
}
}
},
before(asyncId) {
const current = pairing.get(asyncId);
if (current !== undefined) { // enter domain for this cb
current.enter();
}
},
after(asyncId) {
const current = pairing.get(asyncId);
if (current !== undefined) { // exit domain for this cb
current.exit();
}
},
destroy(asyncId) {
pairing.delete(asyncId); // cleaning up
}
});

// It's possible to enter one domain while already inside
// another one. The stack is each entered domain.
const stack = [];
exports._stack = stack;
process._setupDomainUse(stack);

class Domain extends EventEmitter {
Copy link
Member

@AndreasMadsen AndreasMadsen Oct 27, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Technically changing this to ES6 style classes is API breaking. We are likely going to land this in node 9 anyway so it shouldn't be a problem. Just as a note.


// let the process know we're using domains
const _domain_flag = process._setupDomainUse(_domain, stack);
constructor() {
super();

this.members = [];
asyncHook.enable();
}
}

exports.Domain = Domain;

Expand All @@ -64,19 +105,8 @@ exports.create = exports.createDomain = function() {

// the active domain is always the one that we're currently in.
exports.active = null;


inherits(Domain, EventEmitter);

function Domain() {
EventEmitter.call(this);

this.members = [];
}

Domain.prototype.members = undefined;


// Called by process._fatalException in case an error was thrown.
Domain.prototype._errorHandler = function _errorHandler(er) {
var caught = false;
Expand Down Expand Up @@ -155,7 +185,6 @@ Domain.prototype.enter = function() {
// to push it onto the stack so that we can pop it later.
exports.active = process.domain = this;
stack.push(this);
_domain_flag[0] = stack.length;
};


Expand All @@ -166,7 +195,6 @@ Domain.prototype.exit = function() {

// exit all domains until this one.
stack.splice(index);
_domain_flag[0] = stack.length;

exports.active = stack[stack.length - 1];
process.domain = exports.active;
Expand Down
41 changes: 0 additions & 41 deletions lib/internal/process/next_tick.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,6 @@ function setupNextTick() {
process.nextTick = nextTick;
// Needs to be accessible from beyond this scope.
process._tickCallback = _tickCallback;
process._tickDomainCallback = _tickDomainCallback;

// Set the nextTick() function for internal usage.
exports.nextTick = internalNextTick;
Expand Down Expand Up @@ -190,46 +189,6 @@ function setupNextTick() {
} while (tickInfo[kLength] !== 0);
}

function _tickDomainCallback() {
do {
while (tickInfo[kIndex] < tickInfo[kLength]) {
++tickInfo[kIndex];
const tock = nextTickQueue.shift();
const callback = tock.callback;
const domain = tock.domain;
const args = tock.args;
if (domain)
domain.enter();

// CHECK(Number.isSafeInteger(tock[async_id_symbol]))
// CHECK(tock[async_id_symbol] > 0)
// CHECK(Number.isSafeInteger(tock[trigger_async_id_symbol]))
// CHECK(tock[trigger_async_id_symbol] > 0)

emitBefore(tock[async_id_symbol], tock[trigger_async_id_symbol]);
// TODO(trevnorris): See comment in _tickCallback() as to why this
// isn't a good solution.
if (async_hook_fields[kDestroy] > 0)
emitDestroy(tock[async_id_symbol]);

// Using separate callback execution functions allows direct
// callback invocation with small numbers of arguments to avoid the
// performance hit associated with using `fn.apply()`
_combinedTickCallback(args, callback);

emitAfter(tock[async_id_symbol]);

if (kMaxCallbacksPerLoop < tickInfo[kIndex])
tickDone();
if (domain)
domain.exit();
}
tickDone();
_runMicrotasks();
emitPendingUnhandledRejections();
} while (tickInfo[kLength] !== 0);
}

class TickObject {
constructor(callback, args, asyncId, triggerAsyncId) {
this.callback = callback;
Expand Down
6 changes: 0 additions & 6 deletions src/cares_wrap.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -596,12 +596,6 @@ class QueryWrap : public AsyncWrap {
QueryWrap(ChannelWrap* channel, Local<Object> req_wrap_obj)
: AsyncWrap(channel->env(), req_wrap_obj, AsyncWrap::PROVIDER_QUERYWRAP),
channel_(channel) {
if (env()->in_domain()) {
req_wrap_obj->Set(env()->domain_string(),
env()->domain_array()->Get(env()->context(), 0)
.ToLocalChecked());
}

Wrap(req_wrap_obj, this);

// Make sure the channel object stays alive during the query lifetime.
Expand Down
26 changes: 0 additions & 26 deletions src/env-inl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -200,22 +200,6 @@ inline bool Environment::AsyncCallbackScope::in_makecallback() const {
return env_->makecallback_cntr_ > 1;
}

inline Environment::DomainFlag::DomainFlag() {
for (int i = 0; i < kFieldsCount; ++i) fields_[i] = 0;
}

inline uint32_t* Environment::DomainFlag::fields() {
return fields_;
}

inline int Environment::DomainFlag::fields_count() const {
return kFieldsCount;
}

inline uint32_t Environment::DomainFlag::count() const {
return fields_[kCount];
}

inline Environment::TickInfo::TickInfo() {
for (int i = 0; i < kFieldsCount; ++i)
fields_[i] = 0;
Expand Down Expand Up @@ -349,12 +333,6 @@ inline v8::Isolate* Environment::isolate() const {
return isolate_;
}

inline bool Environment::in_domain() const {
// The const_cast is okay, it doesn't violate conceptual const-ness.
return using_domains() &&
const_cast<Environment*>(this)->domain_flag()->count() > 0;
}

inline Environment* Environment::from_immediate_check_handle(
uv_check_t* handle) {
return ContainerOf(&Environment::immediate_check_handle_, handle);
Expand Down Expand Up @@ -395,10 +373,6 @@ inline Environment::AsyncHooks* Environment::async_hooks() {
return &async_hooks_;
}

inline Environment::DomainFlag* Environment::domain_flag() {
return &domain_flag_;
}

inline Environment::TickInfo* Environment::tick_info() {
return &tick_info_;
}
Expand Down
24 changes: 0 additions & 24 deletions src/env.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -309,7 +309,6 @@ class ModuleWrap;
V(internal_binding_cache_object, v8::Object) \
V(buffer_prototype_object, v8::Object) \
V(context, v8::Context) \
V(domain_array, v8::Array) \
V(domains_stack_array, v8::Array) \
V(inspector_console_api_object, v8::Object) \
V(module_load_list_array, v8::Array) \
Expand Down Expand Up @@ -472,26 +471,6 @@ class Environment {
DISALLOW_COPY_AND_ASSIGN(AsyncCallbackScope);
};

class DomainFlag {
public:
inline uint32_t* fields();
inline int fields_count() const;
inline uint32_t count() const;

private:
friend class Environment; // So we can call the constructor.
inline DomainFlag();

enum Fields {
kCount,
kFieldsCount
};

uint32_t fields_[kFieldsCount];

DISALLOW_COPY_AND_ASSIGN(DomainFlag);
};

class TickInfo {
public:
inline uint32_t* fields();
Expand Down Expand Up @@ -560,7 +539,6 @@ class Environment {

inline v8::Isolate* isolate() const;
inline uv_loop_t* event_loop() const;
inline bool in_domain() const;
inline uint32_t watched_providers() const;

static inline Environment* from_immediate_check_handle(uv_check_t* handle);
Expand All @@ -577,7 +555,6 @@ class Environment {
inline void FinishHandleCleanup(uv_handle_t* handle);

inline AsyncHooks* async_hooks();
inline DomainFlag* domain_flag();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The DomainFlag type is no longer used. Can you remove the class

node/src/env.h

Line 466 in e3503ac

class DomainFlag {
:)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch!

inline TickInfo* tick_info();
inline uint64_t timer_base() const;

Expand Down Expand Up @@ -722,7 +699,6 @@ class Environment {
uv_check_t idle_check_handle_;

AsyncHooks async_hooks_;
DomainFlag domain_flag_;
TickInfo tick_info_;
const uint64_t timer_base_;
bool using_domains_;
Expand Down
65 changes: 5 additions & 60 deletions src/node.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -160,7 +160,6 @@ using v8::Number;
using v8::Object;
using v8::ObjectTemplate;
using v8::Promise;
using v8::PromiseHookType;
using v8::PromiseRejectMessage;
using v8::PropertyCallbackInfo;
using v8::ScriptOrigin;
Expand Down Expand Up @@ -842,7 +841,6 @@ bool ShouldAbortOnUncaughtException(Isolate* isolate) {
return isEmittingTopLevelDomainError || !DomainsStackHasErrorHandler(env);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would like to understand why the returned caught boolean in process._fatalException is not enough to determine if we should abort on uncaughtException.

}


Local<Value> GetDomainProperty(Environment* env, Local<Object> object) {
Local<Value> domain_v =
object->GetPrivate(env->context(), env->domain_private_symbol())
Expand Down Expand Up @@ -883,36 +881,6 @@ void DomainExit(Environment* env, v8::Local<v8::Object> object) {
}
}


void DomainPromiseHook(PromiseHookType type,
Local<Promise> promise,
Local<Value> parent,
void* arg) {
Environment* env = static_cast<Environment*>(arg);
Local<Context> context = env->context();

if (type == PromiseHookType::kInit && env->in_domain()) {
Local<Value> domain_obj =
env->domain_array()->Get(context, 0).ToLocalChecked();
if (promise->CreationContext() == context) {
promise->Set(context, env->domain_string(), domain_obj).FromJust();
} else {
// Do not expose object from another context publicly in promises created
// in non-main contexts.
promise->SetPrivate(context, env->domain_private_symbol(), domain_obj)
.FromJust();
}
return;
}

if (type == PromiseHookType::kBefore) {
DomainEnter(env, promise);
} else if (type == PromiseHookType::kAfter) {
DomainExit(env, promise);
}
}


void SetupDomainUse(const FunctionCallbackInfo<Value>& args) {
Environment* env = Environment::GetCurrent(args);

Expand All @@ -923,38 +891,13 @@ void SetupDomainUse(const FunctionCallbackInfo<Value>& args) {
HandleScope scope(env->isolate());
Local<Object> process_object = env->process_object();

Local<String> tick_callback_function_key = env->tick_domain_cb_string();
Local<Function> tick_callback_function =
process_object->Get(tick_callback_function_key).As<Function>();

if (!tick_callback_function->IsFunction()) {
fprintf(stderr, "process._tickDomainCallback assigned to non-function\n");
ABORT();
}

process_object->Set(env->tick_callback_string(), tick_callback_function);
env->set_tick_callback_function(tick_callback_function);

CHECK(args[0]->IsArray());
env->set_domain_array(args[0].As<Array>());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it was lost in a collapsed comment: we can remove all uses of domain_array. Just do a search for domain_array.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be good now. I cleaned up some domain related flags in multiple places ( 821f51d ) Hop I did not miss any

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it compiles without the domain_array definition I think it should be good :)


CHECK(args[1]->IsArray());
env->set_domains_stack_array(args[1].As<Array>());
env->set_domains_stack_array(args[0].As<Array>());

// Do a little housekeeping.
env->process_object()->Delete(
env->context(),
FIXED_ONE_BYTE_STRING(args.GetIsolate(), "_setupDomainUse")).FromJust();

uint32_t* const fields = env->domain_flag()->fields();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove the domain_flag declaration from env.h and env-inl.h.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be good now

uint32_t const fields_count = env->domain_flag()->fields_count();

Local<ArrayBuffer> array_buffer =
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't figure out what this array_buffer was actually used for before :/

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not certain either. Seems to come from this PR #2022 . But could not really see in detail what it does.

ArrayBuffer::New(env->isolate(), fields, sizeof(*fields) * fields_count);

env->AddPromiseHook(DomainPromiseHook, static_cast<void*>(env));

args.GetReturnValue().Set(Uint32Array::New(array_buffer, 0, fields_count));
}


Expand Down Expand Up @@ -1078,7 +1021,8 @@ InternalCallbackScope::InternalCallbackScope(Environment* env,
// If you hit this assertion, you forgot to enter the v8::Context first.
CHECK_EQ(Environment::GetCurrent(env->isolate()), env);

if (env->using_domains() && !object_.IsEmpty()) {
if (asyncContext.async_id == 0 && env->using_domains() &&
!object_.IsEmpty()) {
DomainEnter(env, object_);
}

Expand Down Expand Up @@ -1111,7 +1055,8 @@ void InternalCallbackScope::Close() {
AsyncWrap::EmitAfter(env_, async_context_.async_id);
}

if (env_->using_domains() && !object_.IsEmpty()) {
if (async_context_.async_id == 0 && env_->using_domains() &&
!object_.IsEmpty()) {
DomainExit(env_, object_);
}

Expand Down
Loading