diff --git a/src/base_object.h b/src/base_object.h index 8dc0a194185237..74af3c95274866 100644 --- a/src/base_object.h +++ b/src/base_object.h @@ -232,7 +232,9 @@ inline T* Unwrap(v8::Local obj) { // circumstances such as the GC or Environment cleanup. // If weak, destruction behaviour is not affected, but the pointer will be // reset to nullptr once the BaseObject is destroyed. -// The API matches std::shared_ptr closely. +// The API matches std::shared_ptr closely. However, this class is not thread +// safe, that is, we can't have different BaseObjectPtrImpl instances in +// different threads refering to the same BaseObject instance. template class BaseObjectPtrImpl final { public: diff --git a/src/node_worker.cc b/src/node_worker.cc index 3fa7fecbbd21bb..4a675fdfe7732b 100644 --- a/src/node_worker.cc +++ b/src/node_worker.cc @@ -771,28 +771,49 @@ void Worker::TakeHeapSnapshot(const FunctionCallbackInfo& args) { ->NewInstance(env->context()).ToLocal(&wrap)) { return; } - BaseObjectPtr taker = - MakeDetachedBaseObject(env, wrap); + + // The created WorkerHeapSnapshotTaker is an object owned by main + // thread's Isolate, it can not be accessed by worker thread + std::unique_ptr> taker = + std::make_unique>( + MakeDetachedBaseObject(env, wrap)); // Interrupt the worker thread and take a snapshot, then schedule a call // on the parent thread that turns that snapshot into a readable stream. - bool scheduled = w->RequestInterrupt([taker, env](Environment* worker_env) { - heap::HeapSnapshotPointer snapshot { - worker_env->isolate()->GetHeapProfiler()->TakeHeapSnapshot() }; + bool scheduled = w->RequestInterrupt([taker = std::move(taker), + env](Environment* worker_env) mutable { + heap::HeapSnapshotPointer snapshot{ + worker_env->isolate()->GetHeapProfiler()->TakeHeapSnapshot()}; CHECK(snapshot); + + // Here, the worker thread temporarily owns the WorkerHeapSnapshotTaker + // object. + env->SetImmediateThreadsafe( - [taker, snapshot = std::move(snapshot)](Environment* env) mutable { + [taker = std::move(taker), + snapshot = std::move(snapshot)](Environment* env) mutable { HandleScope handle_scope(env->isolate()); Context::Scope context_scope(env->context()); - AsyncHooks::DefaultTriggerAsyncIdScope trigger_id_scope(taker.get()); - BaseObjectPtr stream = heap::CreateHeapSnapshotStream( - env, std::move(snapshot)); - Local args[] = { stream->object() }; - taker->MakeCallback(env->ondone_string(), arraysize(args), args); - }, CallbackFlags::kUnrefed); + AsyncHooks::DefaultTriggerAsyncIdScope trigger_id_scope(taker->get()); + BaseObjectPtr stream = + heap::CreateHeapSnapshotStream(env, std::move(snapshot)); + Local args[] = {stream->object()}; + taker->get()->MakeCallback( + env->ondone_string(), arraysize(args), args); + // implicitly delete `taker` + }, + CallbackFlags::kUnrefed); + + // Now, the lambda is delivered to the main thread, as a result, the + // WorkerHeapSnapshotTaker object is delivered to the main thread, too. }); - args.GetReturnValue().Set(scheduled ? taker->object() : Local()); + + if (scheduled) { + args.GetReturnValue().Set(wrap); + } else { + args.GetReturnValue().Set(Local()); + } } void Worker::LoopIdleTime(const FunctionCallbackInfo& args) {