Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Memory leak when awaiting in inspector Debugger.pause callback #51397

Closed
timfish opened this issue Jan 7, 2024 · 5 comments · Fixed by #51417
Closed

Memory leak when awaiting in inspector Debugger.pause callback #51397

timfish opened this issue Jan 7, 2024 · 5 comments · Fixed by #51417
Labels
inspector Issues and PRs related to the V8 inspector protocol

Comments

@timfish
Copy link

timfish commented Jan 7, 2024

Version

20.10.0

Platform

All

Subsystem

inspector

What steps will reproduce the bug?

The following code leaks memory at about 10MB per second on my machine:

import { Session } from "node:inspector/promises";

const session = new Session();
session.connect();

session.on("Debugger.paused", async (event) => {
  for (const frame of event.params.callFrames) {
    const localScope = frame.scopeChain.find((scope) => scope.type === "local");

    if (localScope?.object?.objectId) {
      const props = await session.post("Runtime.getProperties", {
        objectId: localScope.object.objectId,
        ownProperties: true,
      });
    }
  }
});

await session.post("Debugger.enable");
await session.post("Debugger.setPauseOnExceptions", { state: "all" });

setInterval(() => {
  console.log(`${Math.floor(process.memoryUsage().rss / 1024 / 1024)} MB`);
}, 1000);

setInterval(() => {
  try {
    throw new Error("Hello");
  } catch {}
}, 1);

How often does it reproduce? Is there a required condition?

Every time

What is the expected behavior? Why is that the expected behavior?

No memory leaked

What do you see instead?

I see ~10MB leaked every second which is around 10kB per debugger pause event.

Additional information

I found that that the following code:

import { Session } from "node:inspector/promises";

const session = new Session();
session.connect();

session.on("Debugger.resumed", () => {
  console.log("Debugger resumed");
});

session.on("Debugger.paused", async (event) => {
  console.log("Debugger paused start");

  for (const frame of event.params.callFrames) {
    const localScope = frame.scopeChain.find((scope) => scope.type === "local");

    if (localScope?.object?.objectId) {
      console.log("Runtime.getProperties");
      const props = await session.post("Runtime.getProperties", {
        objectId: localScope.object.objectId,
      });
      console.log("Runtime.getProperties returned");
    }
  }

  console.log("Debugger.paused end");
});

await session.post("Debugger.enable");
await session.post("Debugger.setPauseOnExceptions", { state: "all" });

setTimeout(() => {
  try {
    throw new Error("Hello");
  } catch {}
}, 1000);

Outputs:

Debugger paused start
Runtime.getProperties
Debugger resumed
Runtime.getProperties returned
Runtime.getProperties
Runtime.getProperties returned
Runtime.getProperties
Runtime.getProperties returned
Debugger.paused end

The debugger does not wait for Debugger.resume. Instead it resumes automatically as soon as we call await session.post and I guess calling Runtime.getProperties after the resume causes something to be leaked?

@anonrig anonrig added the debugger Issues and PRs related to the debugger subsystem. label Jan 8, 2024
@anonrig
Copy link
Member

anonrig commented Jan 8, 2024

cc @legendecas @joyeecheung any suggestions?

@anonrig anonrig added the confirmed-bug Issues with confirmed bugs. label Jan 8, 2024
@legendecas legendecas added confirmed-bug Issues with confirmed bugs. and removed confirmed-bug Issues with confirmed bugs. labels Jan 9, 2024
@legendecas
Copy link
Member

The retained scope information with 'Debugger.paused' must be released with a 'Debugger.resume' call when the debugger is still paused.

For instance:

session.on("Debugger.paused", (event) => {
  // Call 'Debugger.resume' synchronously to release the remote object in `event`.
  session.post("Debugger.resume");
});

Notably, using breakpoints with a same-thread inspector session is not a good practice because the inspected script can not be distinguished from the inspector session codes and the program is literally been resumed when the 'Debugger.paused' fires.

@legendecas legendecas added inspector Issues and PRs related to the V8 inspector protocol and removed confirmed-bug Issues with confirmed bugs. debugger Issues and PRs related to the debugger subsystem. labels Jan 9, 2024
@timfish
Copy link
Author

timfish commented Jan 9, 2024

The issue is that the debugger resumes on it's own when I call await session.post("Runtime.getProperties").

For example, see this full code example I posted to a discussion:
https://github.com/orgs/nodejs/discussions/51393

I had to add isPaused to the above example to keep track of the paused status because If you call 'Debugger.paused' when it's not paused it throws an error.

session.on("Debugger.paused", (event) => {
  // Call 'Debugger.resume' synchronously to release the remote object in `event`.
  session.post("Debugger.resume");
});

So it not possible to call anything asynchronous from within the Debugger.paused callback? In that case the newer promises API can't be used?

@theanarkh
Copy link
Contributor

I think you can use the example as follows.

import { Session } from "node:inspector";

const session = new Session();
session.connect();

session.on("Debugger.paused", async (event) => {
  for (const frame of event.params.callFrames) {
    const localScope = frame.scopeChain.find((scope) => scope.type === "local");
    if (localScope?.object?.objectId) {
       session.post("Runtime.getProperties", {
        objectId: localScope.object.objectId,
        ownProperties: true,
      }, (err, result) => {
        // console.log("Runtime.getProperties");
      });
    }
  }
  session.post("Debugger.resume");
});

session.post("Debugger.enable", (err) => {
  !err && session.post("Debugger.setPauseOnExceptions", { state: "all" });
});

setInterval(() => {
  console.log(`${Math.floor(process.memoryUsage().rss / 1024 / 1024)} MB`);
}, 1000);

setInterval(() => {
  try {
    throw new Error("Hello");
  } catch {}
}, 100);

because VM will call resume after Debugger.pause event, so you can not use await in Debugger.pause event.

@legendecas
Copy link
Member

As mentioned above, it is not recommended to use breakpoints with same thread inspector sessions. Instead, try to connect to the inspector with websocket connections or with a worker thread:

import { Session } from "node:inspector/promises";
import { Worker, isMainThread } from 'node:worker_threads';

if (!isMainThread) {
  inspectMainThread();
} else {
  mainThreadWork();

  new Worker(new URL(import.meta.url));
    .on('exit', (code) => console.log('worker exited', code));
}

function mainThreadWork() {
  setInterval(() => {
    try {
      throw new Error("Hello");
    } catch {}
  }, 1);
}

async function inspectMainThread() {
  setInterval(() => {
    console.log(`${Math.floor(process.memoryUsage().rss / 1024 / 1024)} MB`);
  }, 1000);

  const session = new Session();
  session.connectToMainThread();

  session.on("Debugger.paused", async (event) => {
    // `await` can be used here.

    // After all asynchronous work is done, resume the main thread.
    session.post('Debugger.resume');
  });

  await session.post("Debugger.enable");
  await session.post("Debugger.setPauseOnExceptions", { state: "all" });
}

nodejs-github-bot pushed a commit that referenced this issue Jan 18, 2024
Setting breakpoints with a same-thread inspector session should be
avoided because the program being attached and paused is exactly the
debugger itself. A worker thread inspector session or a debugger
program should be used if breakpoints are needed.

PR-URL: #51417
Fixes: #51397
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Medhansh404 pushed a commit to Medhansh404/node that referenced this issue Jan 19, 2024
Setting breakpoints with a same-thread inspector session should be
avoided because the program being attached and paused is exactly the
debugger itself. A worker thread inspector session or a debugger
program should be used if breakpoints are needed.

PR-URL: nodejs#51417
Fixes: nodejs#51397
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
marco-ippolito pushed a commit to marco-ippolito/node that referenced this issue Jan 22, 2024
Setting breakpoints with a same-thread inspector session should be
avoided because the program being attached and paused is exactly the
debugger itself. A worker thread inspector session or a debugger
program should be used if breakpoints are needed.

PR-URL: nodejs#51417
Fixes: nodejs#51397
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
marco-ippolito pushed a commit to marco-ippolito/node that referenced this issue Feb 2, 2024
Setting breakpoints with a same-thread inspector session should be
avoided because the program being attached and paused is exactly the
debugger itself. A worker thread inspector session or a debugger
program should be used if breakpoints are needed.

PR-URL: nodejs#51417
Fixes: nodejs#51397
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
targos pushed a commit that referenced this issue Feb 15, 2024
Setting breakpoints with a same-thread inspector session should be
avoided because the program being attached and paused is exactly the
debugger itself. A worker thread inspector session or a debugger
program should be used if breakpoints are needed.

PR-URL: #51417
Fixes: #51397
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
marco-ippolito pushed a commit to marco-ippolito/node that referenced this issue Feb 19, 2024
Setting breakpoints with a same-thread inspector session should be
avoided because the program being attached and paused is exactly the
debugger itself. A worker thread inspector session or a debugger
program should be used if breakpoints are needed.

PR-URL: nodejs#51417
Fixes: nodejs#51397
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
richardlau pushed a commit that referenced this issue Mar 25, 2024
Setting breakpoints with a same-thread inspector session should be
avoided because the program being attached and paused is exactly the
debugger itself. A worker thread inspector session or a debugger
program should be used if breakpoints are needed.

PR-URL: #51417
Fixes: #51397
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
richardlau pushed a commit that referenced this issue Mar 25, 2024
Setting breakpoints with a same-thread inspector session should be
avoided because the program being attached and paused is exactly the
debugger itself. A worker thread inspector session or a debugger
program should be used if breakpoints are needed.

PR-URL: #51417
Fixes: #51397
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
inspector Issues and PRs related to the V8 inspector protocol
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants