[SCP-759] Fix profiler shutdown when isolate is terminated by nsavoire · Pull Request #215 · DataDog/pprof-nodejs

nsavoire · 2025-06-05T23:38:26Z

What does this PR do?:

When an isolate is terminated abruptly (eg. by Worker.terminate()), the profiler is not stopped and not removed from the isolate->profiler map. This can lead to a crash.

This commit adds a cleanup hook with node::AddEnvironmentCleanupHook that is called when the isolate is destroyed.
The cleanup hook stops the profiler and removes it from the profiler map.

pr-commenter · 2025-06-05T23:41:57Z

Benchmarks

Benchmark execution time: 2025-06-10 09:16:55

Comparing candidate commit 17343f3 in PR branch nsavoire/fix_profiler_shutdown with baseline commit bfa2637 in branch main.

Found 2 performance improvements and 0 performance regressions! Performance is the same for 88 metrics, 30 unstable metrics.

scenario:profiler-light-load-no-wall-profiler-22

🟩 cpu_user_time [-5.500ms; -1.111ms] or [-7.485%; -1.512%]

scenario:profiler-light-load-with-wall-profiler-24

🟩 cpu_user_time [-10.178ms; -2.901ms] or [-8.759%; -2.497%]

github-actions · 2025-06-05T23:50:47Z

Overall package size

Self size: 1.58 MB
Deduped: 1.95 MB
No deduping: 1.95 MB

Dependency sizes

| name | version | self size | total size | |------|---------|-----------|------------| | source-map | 0.7.4 | 226 kB | 226 kB | | pprof-format | 2.1.0 | 111.69 kB | 111.69 kB | | p-limit | 3.1.0 | 7.75 kB | 13.78 kB | | delay | 5.0.0 | 11.17 kB | 11.17 kB | | node-gyp-build | 3.9.0 | 8.81 kB | 8.81 kB |

_{🤖 This report was automatically generated by heaviest-objects-in-the-universe}

datadog-datadog-prod-us1 · 2025-06-06T09:15:45Z

+  it('should not crash when worker is terminated', async function () {
+    this.timeout(20000);
+    const nruns = 5;
+    const concurrentWorkers = 100;
+    for (let i = 0; i < nruns; i++) {
+      const workers = [];
+      for (let j = 0; j < concurrentWorkers; j++) {
+        const worker = new Worker('./out/test/worker2.js')
+        worker.postMessage('hello')
+
+        worker.on('message', () => {
+          worker.terminate()
+        })
+
+        workers.push(new Promise<void>((resolve, reject) => {
+          worker.on('exit', (exitCode) => {
+            if (exitCode === 1) {
+              resolve()
+            } else {
+              reject(new Error('Worker exited with code 0'))
+            }
+          })
+        }))
+      }
+      await Promise.all(workers)
+    }
+  });


⚪ Code Quality Violation

Unexpected unnamed function. (...read more)

It is easier to debug your application code when you avoid anonymous functions so that the stack trace can show you meaningful error messages. This rule enforces all your function to be consistently declared with a name.

When an isolate is terminated abruptly (eg. by Worker.terminate()), the profiler is not stopped and not removed from the isolate->profiler map. This can lead to a crash. This commit adds a cleanup hook with `node::AddEnvironmentCleanupHook` that is called when the isolate is destroyed. The cleanup hook stops the profiler and removes it from the profiler map.

szegedi

Overall looks great! Going to approve as-is but I wouldn't mind if you considered my suggestion for that if (isolate != nullptr) block in Dispose.

szegedi · 2025-06-10T07:36:50Z

    cpuProfiler_ = nullptr;

-    g_profilers.RemoveProfiler(isolate, this);
+    if (removeFromMap) {


You could have one big if (isolate != nullptr) block here, and move both this and the if (collectAsyncId_) logic (and the cleanup hook removal) below into it. I know they're no-ops when isolate is null, but I think it'd read better as in "here's what is executed when isolate is known".

I removed the call to Dispose from destructor

Well, that's one way to simplify it 😁. I like it. I did feel our disposal and destruction story was a bit overcomplicated, so this is 👍.

Dispose call in destructor felt like a desperate attempt to clean up, now that we have the environment cleanup hook, this should not be needed anymore (and I am not even sure in which cases, other than profiler stop, the destructor is called).

szegedi · 2025-06-10T07:50:22Z

 }

-std::string WallProfiler::StartInternal() {
+v8::ProfilerId WallProfiler::StartInternal() {


Nice that we can use this now 😄

szegedi · 2025-06-10T07:56:04Z


 WallProfiler::~WallProfiler() {
-  Dispose(nullptr);
+  Dispose(nullptr, true);


isolate == nullptr will make g_profilers.RemoveProfiler(isolate, this); in Dispose a no-op anyway so you might as well be passing false. On the other hand, if we do the transformation in Dispose I suggested then it'd get short-circuited by the nullptr anyway so it doesn't matter what's the value here.

I think we should remove this call to Dispose, the only case when it's not a no-op is when dispose is called on a running profiler, and that should be an error.

szegedi · 2025-06-10T08:09:50Z

  return V8toEpochOffset;
 }

+void WallProfiler::CleanupHook(void* data) {


Making this a private static method on WallProfiler actually isn't a bad idea. I should probably do an equivalent for GC callbacks so I can make them private.

datadog-datadog-prod-us1 · 2025-06-10T09:13:38Z

+  it('should not crash when worker is terminated', async function () {
+    this.timeout(30000);
+    const nruns = 5;
+    const concurrentWorkers = 20;
+    for (let i = 0; i < nruns; i++) {
+      const workers = [];
+      for (let j = 0; j < concurrentWorkers; j++) {
+        const worker = new Worker('./out/test/worker2.js');
+        worker.postMessage('hello');
+
+        worker.on('message', () => {
+          worker.terminate();
+        });
+
+        workers.push(
+          new Promise<void>((resolve, reject) => {
+            worker.on('exit', exitCode => {
+              if (exitCode === 1) {
+                resolve();
+              } else {
+                reject(new Error('Worker exited with code 0'));
+              }
+            });
+          })
+        );
+      }
+      await Promise.all(workers);
+    }
+  });


⚪ Code Quality Violation

Unexpected unnamed function. (...read more)

It is easier to debug your application code when you avoid anonymous functions so that the stack trace can show you meaningful error messages. This rule enforces all your function to be consistently declared with a name.

szegedi

🚢

* Fix profiler shutdown when isolate is terminated When an isolate is terminated abruptly (eg. by Worker.terminate()), the profiler is not stopped and not removed from the isolate->profiler map. This can lead to a crash. This commit adds a cleanup hook with `node::AddEnvironmentCleanupHook` that is called when the isolate is destroyed. The cleanup hook stops the profiler and removes it from the profiler map.

nsavoire added the semver-patch Bug or security fixes, mainly label Jun 5, 2025

nsavoire force-pushed the nsavoire/fix_profiler_shutdown branch 2 times, most recently from 2da68aa to 488b05f Compare June 5, 2025 23:50

nsavoire force-pushed the nsavoire/fix_profiler_shutdown branch 2 times, most recently from 87e6b98 to f9a54c2 Compare June 6, 2025 00:32

nsavoire marked this pull request as ready for review June 6, 2025 00:38

nsavoire requested a review from szegedi as a code owner June 6, 2025 00:38

datadog-datadog-prod-us1 Bot reviewed Jun 6, 2025

View reviewed changes

nsavoire force-pushed the nsavoire/fix_profiler_shutdown branch 3 times, most recently from a03101a to 10b3b89 Compare June 6, 2025 15:30

nsavoire added 3 commits June 6, 2025 15:35

Do not build for node 16

c6b1279

Add worker termination test

ab2ddb2

nsavoire force-pushed the nsavoire/fix_profiler_shutdown branch from 10b3b89 to ab2ddb2 Compare June 6, 2025 15:36

szegedi previously approved these changes Jun 10, 2025

View reviewed changes

Ensure profiler is stopped before disposing of it

17343f3

nsavoire dismissed szegedi’s stale review via 17343f3 June 10, 2025 09:12

datadog-datadog-prod-us1 Bot reviewed Jun 10, 2025

View reviewed changes

szegedi approved these changes Jun 10, 2025

View reviewed changes

szegedi merged commit 2f6f2f8 into main Jun 10, 2025
57 checks passed

szegedi deleted the nsavoire/fix_profiler_shutdown branch June 10, 2025 09:24

nsavoire changed the title ~~Fix profiler shutdown when isolate is terminated~~ [SCP-759] Fix profiler shutdown when isolate is terminated Jun 10, 2025

This was referenced Jun 10, 2025

v5.8.1 proposal #216

Merged

Update pprof-nodejs to 5.8.2 DataDog/dd-trace-js#5866

Merged

szegedi mentioned this pull request Jun 12, 2025

Update min-node-version in release.yml to 18 #219

Merged

szegedi mentioned this pull request Dec 1, 2025

Fix isolate cleanup segfault #245

Merged

Conversation

nsavoire commented Jun 5, 2025

Uh oh!

pr-commenter Bot commented Jun 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Benchmarks

scenario:profiler-light-load-no-wall-profiler-22

scenario:profiler-light-load-with-wall-profiler-24

Uh oh!

github-actions Bot commented Jun 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Overall package size

Uh oh!

datadog-datadog-prod-us1 Bot Jun 6, 2025

Choose a reason for hiding this comment

⚪ Code Quality Violation

Uh oh!

szegedi left a comment

Choose a reason for hiding this comment

Uh oh!

szegedi Jun 10, 2025

Choose a reason for hiding this comment

Uh oh!

nsavoire Jun 10, 2025

Choose a reason for hiding this comment

Uh oh!

szegedi Jun 10, 2025

Choose a reason for hiding this comment

Uh oh!

nsavoire Jun 10, 2025

Choose a reason for hiding this comment

Uh oh!

szegedi Jun 10, 2025

Choose a reason for hiding this comment

Uh oh!

szegedi Jun 10, 2025

Choose a reason for hiding this comment

Uh oh!

nsavoire Jun 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

szegedi Jun 10, 2025

Choose a reason for hiding this comment

Uh oh!

datadog-datadog-prod-us1 Bot Jun 10, 2025

Choose a reason for hiding this comment

⚪ Code Quality Violation

Uh oh!

szegedi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

pr-commenter Bot commented Jun 5, 2025 •

edited

Loading

github-actions Bot commented Jun 5, 2025 •

edited

Loading

nsavoire Jun 10, 2025 •

edited

Loading