From 209112adca9d42c4b32c41c289b8b287980ef99c Mon Sep 17 00:00:00 2001 From: Santiago Gimeno Date: Thu, 16 Nov 2023 14:51:55 +0100 Subject: [PATCH 1/4] src: change EnvList::promise_tracking_() signature Pass directly the SharedEnvInst. PR-URL: https://github.com/nodesource/nsolid/pull/25 Reviewed-by: Santiago Gimeno --- src/nsolid/nsolid_api.cc | 7 +++---- src/nsolid/nsolid_api.h | 2 +- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/src/nsolid/nsolid_api.cc b/src/nsolid/nsolid_api.cc index 26389f7e83..6d1ec26736 100644 --- a/src/nsolid/nsolid_api.cc +++ b/src/nsolid/nsolid_api.cc @@ -1556,8 +1556,7 @@ void EnvList::gen_ptiles_cb_(ns_timer*) { } -void EnvList::promise_tracking_(const EnvInst& envinst, bool track) { - SharedEnvInst envinst_sp = EnvInst::GetInst(envinst.thread_id()); +void EnvList::promise_tracking_(SharedEnvInst envinst_sp, bool track) { Environment* env = envinst_sp->env(); if (env->nsolid_track_promises_fn().IsEmpty() || !envinst_sp->can_call_into_js()) { @@ -1580,12 +1579,12 @@ void EnvList::promise_tracking_(const EnvInst& envinst, bool track) { void EnvList::enable_promise_tracking_(SharedEnvInst envinst_sp, void*) { - EnvList::promise_tracking_(*envinst_sp.get(), true); + EnvList::promise_tracking_(envinst_sp, true); } void EnvList::disable_promise_tracking_(SharedEnvInst envinst_sp, void*) { - EnvList::promise_tracking_(*envinst_sp.get(), false); + EnvList::promise_tracking_(envinst_sp, false); } diff --git a/src/nsolid/nsolid_api.h b/src/nsolid/nsolid_api.h index c6aa9f3ccd..199d3e731a 100644 --- a/src/nsolid/nsolid_api.h +++ b/src/nsolid/nsolid_api.h @@ -594,7 +594,7 @@ class EnvList { static void blocked_loop_timer_cb_(nsuv::ns_timer*); static void gen_ptiles_cb_(nsuv::ns_timer*); static void raw_metrics_timer_cb_(nsuv::ns_timer*); - static void promise_tracking_(const EnvInst& envinst, bool track); + static void promise_tracking_(SharedEnvInst envinst_sp, bool track); static void enable_promise_tracking_(SharedEnvInst envinst_sp, void*); static void disable_promise_tracking_(SharedEnvInst envinst_sp, void*); static void update_has_metrics_stream_hooks(SharedEnvInst, bool has_metrics); From 6f303c744e47a2fab2052da1741ee0b7caa3b663 Mon Sep 17 00:00:00 2001 From: Santiago Gimeno Date: Thu, 16 Nov 2023 14:52:55 +0100 Subject: [PATCH 2/4] src: fix EnvInst::GetCurrent() Handle the case where the `Environment` is already nullptr. PR-URL: https://github.com/nodesource/nsolid/pull/25 Reviewed-by: Santiago Gimeno --- src/nsolid/nsolid_api.cc | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/src/nsolid/nsolid_api.cc b/src/nsolid/nsolid_api.cc index 6d1ec26736..88cfef131e 100644 --- a/src/nsolid/nsolid_api.cc +++ b/src/nsolid/nsolid_api.cc @@ -301,7 +301,10 @@ void EnvInst::SetNodeStartupTime(const char* name, uint64_t ts) { std::string EnvInst::GetOnBlockedBody() { DCHECK(utils::are_threads_equal(creation_thread(), uv_thread_self())); uv_metrics_t metrics; - auto SharedEnvInst = GetCurrent(isolate_); + SharedEnvInst envinst = GetCurrent(isolate_); + if (envinst == nullptr) { + return ""; + } HandleScope scope(isolate_); Local stack = @@ -316,7 +319,7 @@ std::string EnvInst::GetOnBlockedBody() { std::string body_string = "{"; // TODO(trevnorris): REMOVE provider_times so libuv don't need to use the // floating patch. - uv_metrics_info(SharedEnvInst->event_loop(), &metrics); + uv_metrics_info(envinst->event_loop(), &metrics); uint64_t exit_time = metrics.loop_count == 0 ? env()->time_origin() : provider_times().second; @@ -401,12 +404,14 @@ SharedEnvInst EnvInst::GetInst(uint64_t thread_id) { SharedEnvInst EnvInst::GetCurrent(Isolate* isolate) { - return Environment::GetCurrent(isolate)->envinst_; + Environment* env = Environment::GetCurrent(isolate); + return env == nullptr ? nullptr : env->envinst_; } SharedEnvInst EnvInst::GetCurrent(Local context) { - return Environment::GetCurrent(context)->envinst_; + Environment* env = Environment::GetCurrent(context); + return env == nullptr ? nullptr : env->envinst_; } From be6aea6f00110e7a58caf7982574b9c71efd7902 Mon Sep 17 00:00:00 2001 From: Santiago Gimeno Date: Thu, 16 Nov 2023 14:53:46 +0100 Subject: [PATCH 3/4] src: cleanup the RunCommand queues on RemoveEnv Just to be sure no dangling SharedEnvInst references are left after the Environment is gone and the EnvInst instance can be deleted. PR-URL: https://github.com/nodesource/nsolid/pull/25 Reviewed-by: Santiago Gimeno --- src/nsolid/nsolid_api.cc | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/src/nsolid/nsolid_api.cc b/src/nsolid/nsolid_api.cc index 88cfef131e..e7b1dc4ca7 100644 --- a/src/nsolid/nsolid_api.cc +++ b/src/nsolid/nsolid_api.cc @@ -989,6 +989,22 @@ void EnvList::RemoveEnv(Environment* env) { stor.cb(envinst_sp, stor.data.get()); }); + // Cleanup the RunCommand queues just to be sure no dangling SharedEnvInst + // references are left after the Environment is gone and the EnvInst instance + // can be deleted. + EnvInst::CmdQueueStor stor; + while (envinst_sp->eloop_cmds_q_.dequeue(stor)) { + stor.cb(stor.envinst_sp, stor.data); + } + + while (envinst_sp->interrupt_cb_q_.dequeue(stor)) { + stor.cb(stor.envinst_sp, stor.data); + } + + while (envinst_sp->interrupt_only_cb_q_.dequeue(stor)) { + stor.cb(stor.envinst_sp, stor.data); + } + // Don't allow execution to continue in case a RunCommand() is running in // another thread since they might need access to Environment specific // resources (like the Isolate) that won't be valid after this returns. From f16e22802fa9bfe1871b419c3b3fc39ca419eb31 Mon Sep 17 00:00:00 2001 From: Santiago Gimeno Date: Thu, 16 Nov 2023 14:55:37 +0100 Subject: [PATCH 4/4] src: fix SetupArrayBufferExports() declaration PR-URL: https://github.com/nodesource/nsolid/pull/25 Reviewed-by: Santiago Gimeno --- src/nsolid/nsolid_api.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/nsolid/nsolid_api.cc b/src/nsolid/nsolid_api.cc index e7b1dc4ca7..34f0986890 100644 --- a/src/nsolid/nsolid_api.cc +++ b/src/nsolid/nsolid_api.cc @@ -2516,7 +2516,7 @@ static void GetOnBlockedBody(const FunctionCallbackInfo& args) { static void SetupArrayBufferExports(Isolate* isolate, Local target, Local context, - std::shared_ptr envinst_sp) { + SharedEnvInst envinst_sp) { std::unique_ptr bs = ArrayBuffer::NewBackingStore(envinst_sp->count_fields, EnvInst::kFieldCount * sizeof(double),