From 0812b244e524eca631f6a12c10fb5d8f035b3f80 Mon Sep 17 00:00:00 2001 From: JonasBa Date: Tue, 10 Sep 2024 15:23:06 -0400 Subject: [PATCH 1/3] ref(profiling) use cleanup instead of destructor --- packages/profiling-node/bindings/cpu_profiler.cc | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-) diff --git a/packages/profiling-node/bindings/cpu_profiler.cc b/packages/profiling-node/bindings/cpu_profiler.cc index 00996db9e8c9..a3756a92b39c 100644 --- a/packages/profiling-node/bindings/cpu_profiler.cc +++ b/packages/profiling-node/bindings/cpu_profiler.cc @@ -111,15 +111,9 @@ class MeasurementsTicker { size_t listener_count(); - ~MeasurementsTicker() { + void Cleanup() { uv_timer_stop(&timer); - - auto handle = reinterpret_cast(&timer); - - // Calling uv_close on an inactive handle will cause a segfault. - if (uv_is_active(handle)) { - uv_close(handle, nullptr); - } + uv_close(reinterpret_cast(&timer), nullptr); } }; @@ -1155,6 +1149,7 @@ void FreeAddonData(napi_env env, void *data, void *hint) { profiler->cpu_profiler = nullptr; } + profiler->measurements_ticker.Cleanup(); delete profiler; } From b9a3c02acb35b687205c714e6ede343f3175ffef Mon Sep 17 00:00:00 2001 From: JonasBa Date: Tue, 10 Sep 2024 15:52:56 -0400 Subject: [PATCH 2/3] fix(profiling) segfault on close --- .../profiling-node/bindings/cpu_profiler.cc | 45 +++++++++++-------- 1 file changed, 27 insertions(+), 18 deletions(-) diff --git a/packages/profiling-node/bindings/cpu_profiler.cc b/packages/profiling-node/bindings/cpu_profiler.cc index a3756a92b39c..c6d2a73dc029 100644 --- a/packages/profiling-node/bindings/cpu_profiler.cc +++ b/packages/profiling-node/bindings/cpu_profiler.cc @@ -73,7 +73,7 @@ enum class ProfileStatus { class MeasurementsTicker { private: - uv_timer_t timer; + uv_timer_t* timer; uint64_t period_ms; std::unordered_map> @@ -86,13 +86,15 @@ class MeasurementsTicker { public: MeasurementsTicker(uv_loop_t *loop) - : period_ms(100), isolate(v8::Isolate::GetCurrent()) { - uv_timer_init(loop, &timer); - uv_handle_set_data(reinterpret_cast(&timer), this); - uv_unref(reinterpret_cast(&timer)); + : period_ms(100), + isolate(v8::Isolate::GetCurrent()) { + timer = new uv_timer_t; + uv_timer_init(loop, timer); + uv_handle_set_data((uv_handle_t*)timer, this); + uv_ref((uv_handle_t*)timer); } - static void ticker(uv_timer_t *); + static void ticker(uv_timer_t*); // Memory listeners void heap_callback(); void add_heap_listener( @@ -111,9 +113,17 @@ class MeasurementsTicker { size_t listener_count(); - void Cleanup() { - uv_timer_stop(&timer); - uv_close(reinterpret_cast(&timer), nullptr); + ~MeasurementsTicker() { + uv_handle_t* handle = (uv_handle_t*)timer; + + uv_timer_stop(timer); + uv_unref(handle); + + if (!uv_is_closing(handle)) { + uv_close(handle, [](uv_handle_t *handle) { + delete handle; + }); + } } }; @@ -137,8 +147,8 @@ void MeasurementsTicker::add_heap_listener( heap_listeners.emplace(profile_id, cb); if (listener_count() == 1) { - uv_timer_set_repeat(&timer, period_ms); - uv_timer_start(&timer, ticker, 0, period_ms); + uv_timer_set_repeat(timer, period_ms); + uv_timer_start(timer, ticker, 0, period_ms); } } @@ -148,7 +158,7 @@ void MeasurementsTicker::remove_heap_listener( heap_listeners.erase(profile_id); if (listener_count() == 0) { - uv_timer_stop(&timer); + uv_timer_stop(timer); } }; @@ -202,12 +212,12 @@ void MeasurementsTicker::cpu_callback() { uv_free_cpu_info(cpu, count); }; -void MeasurementsTicker::ticker(uv_timer_t *handle) { +void MeasurementsTicker::ticker(uv_timer_t* handle) { if (handle == nullptr) { return; } - MeasurementsTicker *self = static_cast(handle->data); + MeasurementsTicker *self = static_cast(handle->data); self->heap_callback(); self->cpu_callback(); } @@ -217,8 +227,8 @@ void MeasurementsTicker::add_cpu_listener( cpu_listeners.emplace(profile_id, cb); if (listener_count() == 1) { - uv_timer_set_repeat(&timer, period_ms); - uv_timer_start(&timer, ticker, 0, period_ms); + uv_timer_set_repeat(timer, period_ms); + uv_timer_start(timer, ticker, 0, period_ms); } } @@ -227,7 +237,7 @@ void MeasurementsTicker::remove_cpu_listener( cpu_listeners.erase(profile_id); if (listener_count() == 0) { - uv_timer_stop(&timer); + uv_timer_stop(timer); } }; @@ -1149,7 +1159,6 @@ void FreeAddonData(napi_env env, void *data, void *hint) { profiler->cpu_profiler = nullptr; } - profiler->measurements_ticker.Cleanup(); delete profiler; } From 5aa761e51d3f64bd6c1949c63d7e412ae69f65a3 Mon Sep 17 00:00:00 2001 From: JonasBa Date: Tue, 10 Sep 2024 19:38:13 -0400 Subject: [PATCH 3/3] clang format --- .../profiling-node/bindings/cpu_profiler.cc | 21 ++++++++----------- 1 file changed, 9 insertions(+), 12 deletions(-) diff --git a/packages/profiling-node/bindings/cpu_profiler.cc b/packages/profiling-node/bindings/cpu_profiler.cc index c6d2a73dc029..ad3ca8079e00 100644 --- a/packages/profiling-node/bindings/cpu_profiler.cc +++ b/packages/profiling-node/bindings/cpu_profiler.cc @@ -73,7 +73,7 @@ enum class ProfileStatus { class MeasurementsTicker { private: - uv_timer_t* timer; + uv_timer_t *timer; uint64_t period_ms; std::unordered_map> @@ -86,15 +86,14 @@ class MeasurementsTicker { public: MeasurementsTicker(uv_loop_t *loop) - : period_ms(100), - isolate(v8::Isolate::GetCurrent()) { + : period_ms(100), isolate(v8::Isolate::GetCurrent()) { timer = new uv_timer_t; uv_timer_init(loop, timer); - uv_handle_set_data((uv_handle_t*)timer, this); - uv_ref((uv_handle_t*)timer); + uv_handle_set_data((uv_handle_t *)timer, this); + uv_ref((uv_handle_t *)timer); } - static void ticker(uv_timer_t*); + static void ticker(uv_timer_t *); // Memory listeners void heap_callback(); void add_heap_listener( @@ -114,15 +113,13 @@ class MeasurementsTicker { size_t listener_count(); ~MeasurementsTicker() { - uv_handle_t* handle = (uv_handle_t*)timer; + uv_handle_t *handle = (uv_handle_t *)timer; uv_timer_stop(timer); uv_unref(handle); if (!uv_is_closing(handle)) { - uv_close(handle, [](uv_handle_t *handle) { - delete handle; - }); + uv_close(handle, [](uv_handle_t *handle) { delete handle; }); } } }; @@ -212,12 +209,12 @@ void MeasurementsTicker::cpu_callback() { uv_free_cpu_info(cpu, count); }; -void MeasurementsTicker::ticker(uv_timer_t* handle) { +void MeasurementsTicker::ticker(uv_timer_t *handle) { if (handle == nullptr) { return; } - MeasurementsTicker *self = static_cast(handle->data); + MeasurementsTicker *self = static_cast(handle->data); self->heap_callback(); self->cpu_callback(); }