Skip to content

Commit

Permalink
src: use callback scope for main script
Browse files Browse the repository at this point in the history
This allows removing custom code for setting the current async ids
and running nextTicks.

PR-URL: #30236
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
  • Loading branch information
addaleax committed Nov 6, 2019
1 parent 3698031 commit d80e49d
Show file tree
Hide file tree
Showing 7 changed files with 30 additions and 47 deletions.
11 changes: 6 additions & 5 deletions src/api/callback.cc
Original file line number Diff line number Diff line change
Expand Up @@ -43,12 +43,13 @@ InternalCallbackScope::InternalCallbackScope(AsyncWrap* async_wrap)
InternalCallbackScope::InternalCallbackScope(Environment* env,
Local<Object> object,
const async_context& asyncContext,
ResourceExpectation expect)
int flags)
: env_(env),
async_context_(asyncContext),
object_(object),
callback_scope_(env) {
CHECK_IMPLIES(expect == kRequireResource, !object.IsEmpty());
callback_scope_(env),
skip_hooks_(flags & kSkipAsyncHooks) {
CHECK_IMPLIES(!(flags & kAllowEmptyResource), !object.IsEmpty());
CHECK_NOT_NULL(env);

if (!env->can_call_into_js()) {
Expand All @@ -60,7 +61,7 @@ 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 (asyncContext.async_id != 0) {
if (asyncContext.async_id != 0 && !skip_hooks_) {
// No need to check a return value because the application will exit if
// an exception occurs.
AsyncWrap::EmitBefore(env, asyncContext.async_id);
Expand Down Expand Up @@ -89,7 +90,7 @@ void InternalCallbackScope::Close() {

if (failed_) return;

if (async_context_.async_id != 0) {
if (async_context_.async_id != 0 && !skip_hooks_) {
AsyncWrap::EmitAfter(env_, async_context_.async_id);
}

Expand Down
9 changes: 2 additions & 7 deletions src/node.cc
Original file line number Diff line number Diff line change
Expand Up @@ -406,13 +406,8 @@ MaybeLocal<Value> StartExecution(Environment* env, const char* main_script_id) {
->GetFunction(env->context())
.ToLocalChecked()};

Local<Value> result;
if (!ExecuteBootstrapper(env, main_script_id, &parameters, &arguments)
.ToLocal(&result) ||
!task_queue::RunNextTicksNative(env)) {
return MaybeLocal<Value>();
}
return scope.Escape(result);
return scope.EscapeMaybe(
ExecuteBootstrapper(env, main_script_id, &parameters, &arguments));
}

MaybeLocal<Value> StartMainThreadExecution(Environment* env) {
Expand Down
11 changes: 8 additions & 3 deletions src/node_internals.h
Original file line number Diff line number Diff line change
Expand Up @@ -203,12 +203,16 @@ v8::MaybeLocal<v8::Value> InternalMakeCallback(

class InternalCallbackScope {
public:
// Tell the constructor whether its `object` parameter may be empty or not.
enum ResourceExpectation { kRequireResource, kAllowEmptyResource };
enum Flags {
// Tell the constructor whether its `object` parameter may be empty or not.
kAllowEmptyResource = 1,
// Indicates whether 'before' and 'after' hooks should be skipped.
kSkipAsyncHooks = 2
};
InternalCallbackScope(Environment* env,
v8::Local<v8::Object> object,
const async_context& asyncContext,
ResourceExpectation expect = kRequireResource);
int flags = 0);
// Utility that can be used by AsyncWrap classes.
explicit InternalCallbackScope(AsyncWrap* async_wrap);
~InternalCallbackScope();
Expand All @@ -222,6 +226,7 @@ class InternalCallbackScope {
async_context async_context_;
v8::Local<v8::Object> object_;
AsyncCallbackScope callback_scope_;
bool skip_hooks_;
bool failed_ = false;
bool pushed_ids_ = false;
bool closed_ = false;
Expand Down
10 changes: 7 additions & 3 deletions src/node_main_instance.cc
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ using v8::HandleScope;
using v8::Isolate;
using v8::Local;
using v8::Locker;
using v8::Object;
using v8::SealHandleScope;

NodeMainInstance::NodeMainInstance(Isolate* isolate,
Expand Down Expand Up @@ -111,10 +112,13 @@ int NodeMainInstance::Run() {

if (exit_code == 0) {
{
AsyncCallbackScope callback_scope(env.get());
env->async_hooks()->push_async_ids(1, 0);
InternalCallbackScope callback_scope(
env.get(),
Local<Object>(),
{ 1, 0 },
InternalCallbackScope::kAllowEmptyResource |
InternalCallbackScope::kSkipAsyncHooks);
LoadEnvironment(env.get());
env->async_hooks()->pop_async_id(1);
}

env->set_trace_sync_io(env->options()->trace_sync_io);
Expand Down
9 changes: 0 additions & 9 deletions src/node_process.h
Original file line number Diff line number Diff line change
Expand Up @@ -34,15 +34,6 @@ v8::Maybe<bool> ProcessEmitDeprecationWarning(Environment* env,
v8::MaybeLocal<v8::Object> CreateProcessObject(Environment* env);
void PatchProcessObject(const v8::FunctionCallbackInfo<v8::Value>& args);

namespace task_queue {
// Handle any nextTicks added in the first tick of the program.
// We use the native version here for once so that any microtasks
// created by the main module is then handled from C++, and
// the call stack of the main script does not show up in the async error
// stack trace.
bool RunNextTicksNative(Environment* env);
} // namespace task_queue

} // namespace node
#endif // defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS
#endif // SRC_NODE_PROCESS_H_
16 changes: 0 additions & 16 deletions src/node_task_queue.cc
Original file line number Diff line number Diff line change
Expand Up @@ -41,22 +41,6 @@ static void EnqueueMicrotask(const FunctionCallbackInfo<Value>& args) {
isolate->EnqueueMicrotask(args[0].As<Function>());
}

// Should be in sync with runNextTicks in internal/process/task_queues.js
bool RunNextTicksNative(Environment* env) {
OnScopeLeave weakref_cleanup([&]() { env->RunWeakRefCleanup(); });

TickInfo* tick_info = env->tick_info();
if (!tick_info->has_tick_scheduled() && !tick_info->has_rejection_to_warn())
MicrotasksScope::PerformCheckpoint(env->isolate());
if (!tick_info->has_tick_scheduled() && !tick_info->has_rejection_to_warn())
return true;

Local<Function> callback = env->tick_callback_function();
CHECK(!callback.IsEmpty());
return !callback->Call(env->context(), env->process_object(), 0, nullptr)
.IsEmpty();
}

static void RunMicrotasks(const FunctionCallbackInfo<Value>& args) {
MicrotasksScope::PerformCheckpoint(args.GetIsolate());
}
Expand Down
11 changes: 7 additions & 4 deletions src/node_worker.cc
Original file line number Diff line number Diff line change
Expand Up @@ -347,17 +347,20 @@ void Worker::Run() {
inspector_started = true;
#endif
HandleScope handle_scope(isolate_);
AsyncCallbackScope callback_scope(env_.get());
env_->async_hooks()->push_async_ids(1, 0);
InternalCallbackScope callback_scope(
env_.get(),
Local<Object>(),
{ 1, 0 },
InternalCallbackScope::kAllowEmptyResource |
InternalCallbackScope::kSkipAsyncHooks);

if (!env_->RunBootstrapping().IsEmpty()) {
CreateEnvMessagePort(env_.get());
if (is_stopped()) return;
Debug(this, "Created message port for worker %llu", thread_id_);
USE(StartExecution(env_.get(), "internal/main/worker_thread"));
}

env_->async_hooks()->pop_async_id(1);

Debug(this, "Loaded environment for worker %llu", thread_id_);
}

Expand Down

1 comment on commit d80e49d

@benjamingr
Copy link
Member

Choose a reason for hiding this comment

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

@alexhultman-2fa @alexhultman if you want to interact with the Node.js org in GitHub - please contact moderation@nodejs.org to appeal your ban and show you are willing to interact in good faith. The Node.js org will not tolerate repeated abuse.

Please sign in to comment.