fix(node): Local variables handle error (#13827)

`Runtime.releaseObject` can throw. I've reworked it to simplify and so that errors are caught. After a lot of testing, I've removed the memory leak test. I've tested this PR extensively on Node v20 + v22 and while memory usage does peak at ~350MB, it doesn't get above that after 15 minute of 1k errors per second. Memory usage varies from 150-300MB but I've concluded it's not actually leaking.
getsentry · Oct 4, 2024 · 0d42ae8 · 0d42ae8
1 parent 444d2fe
commit 0d42ae8
Show file tree

Hide file tree

Showing 3 changed files with 3 additions and 85 deletions.
diff --git a/...es/node-integration-tests/suites/public-api/LocalVariables/local-variables-memory-test.js b/...es/node-integration-tests/suites/public-api/LocalVariables/local-variables-memory-test.js
diff --git a/dev-packages/node-integration-tests/suites/public-api/LocalVariables/test.ts b/dev-packages/node-integration-tests/suites/public-api/LocalVariables/test.ts
@@ -1,4 +1,3 @@
-import * as childProcess from 'child_process';
 import * as path from 'path';
 import { conditionalTest } from '../../../utils';
 import { cleanupChildProcesses, createRunner } from '../../../utils/runner';
@@ -89,29 +88,4 @@ conditionalTest({ min: 18 })('LocalVariables integration', () => {
   test('Includes local variables for caught exceptions when enabled', done => {
     createRunner(__dirname, 'local-variables-caught.js').expect({ event: EXPECTED_LOCAL_VARIABLES_EVENT }).start(done);
   });
-
-  test('Should not leak memory', done => {
-    const testScriptPath = path.resolve(__dirname, 'local-variables-memory-test.js');
-
-    const child = childProcess.spawn('node', [testScriptPath], {
-      stdio: ['inherit', 'inherit', 'inherit', 'ipc'],
-    });
-
-    let reportedCount = 0;
-
-    child.on('message', msg => {
-      reportedCount++;
-      const rssMb = (msg as { memUsage: { rss: number } }).memUsage.rss / 1024 / 1024;
-      // We shouldn't use more than 135MB of memory
-      expect(rssMb).toBeLessThan(135);
-    });
-
-    // Wait for 20 seconds
-    setTimeout(() => {
-      // Ensure we've had memory usage reported at least 15 times
-      expect(reportedCount).toBeGreaterThan(15);
-      child.kill();
-      done();
-    }, 20000);
-  });
 });
diff --git a/packages/node/src/integrations/local-variables/worker.ts b/packages/node/src/integrations/local-variables/worker.ts
@@ -86,7 +86,7 @@ let rateLimiter: RateLimitIncrement | undefined;
 async function handlePaused(
   session: Session,
   { reason, data: { objectId }, callFrames }: PausedExceptionEvent,
-): Promise<string | undefined> {
+): Promise<void> {
   if (reason !== 'exception' && reason !== 'promiseRejection') {
     return;
   }
@@ -126,7 +126,7 @@ async function handlePaused(
     objectId,
   });
 
-  return objectId;
+  await session.post('Runtime.releaseObject', { objectId });
 }
 
 async function startDebugger(): Promise<void> {
@@ -145,19 +145,11 @@ async function startDebugger(): Promise<void> {
     isPaused = true;
 
     handlePaused(session, event.params as PausedExceptionEvent).then(
-      async objectId => {
+      async () => {
         // After the pause work is complete, resume execution!
         if (isPaused) {
           await session.post('Debugger.resume');
         }
-
-        if (objectId) {
-          // The object must be released after the debugger has resumed or we get a memory leak.
-          // For node v20, setImmediate is enough here but for v22 a longer delay is required
-          setTimeout(async () => {
-            await session.post('Runtime.releaseObject', { objectId });
-          }, 1_000);
-        }
       },
       async _ => {
         if (isPaused) {