The Wayback Machine - https://web.archive.org/web/20220203023154/https://github.com/nodejs/node/issues/41795
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

Jump to bottom

Purpose of double Debugger.setPauseOnExceptions #41795

Open
Delapouite opened this issue Nov 10, 2017 · 7 comments
Open

Purpose of double Debugger.setPauseOnExceptions #41795

Delapouite opened this issue Nov 10, 2017 · 7 comments
Labels
debugger good first issue

Comments

@Delapouite
Copy link
Member

@Delapouite Delapouite commented Nov 10, 2017

Hi

During the init phase: https://github.com/nodejs/node-inspect/blob/master/lib/internal/inspect_repl.js#L1041

  function initAfterStart() {
    const setupTasks = [
      Runtime.enable(),
      Profiler.enable(),
      Profiler.setSamplingInterval({ interval: 100 }),
      Debugger.enable(),
      Debugger.setPauseOnExceptions({ state: 'none' }),
      Debugger.setAsyncCallStackDepth({ maxDepth: 0 }),
      Debugger.setBlackboxPatterns({ patterns: [] }),
      Debugger.setPauseOnExceptions({ state: pauseOnExceptionState }),
      restoreBreakpoints(),
      Runtime.runIfWaitingForDebugger(),
    ];
    return Promise.all(setupTasks);
  }

What's the purpose of doing Debugger.setPauseOnExceptions twice?

Debugger.setPauseOnExceptions({ state: 'none' }),
Debugger.setPauseOnExceptions({ state: pauseOnExceptionState }),
@jkrems
Copy link
Contributor

@jkrems jkrems commented Nov 13, 2017

Hm, I don't think this is on purpose. I assume that the first got copied from somewhere and then I later added the 2nd line without looking properly. Nice find! I assume the first one could just be removed.
@ak239
Copy link
Member

@ak239 ak239 commented Nov 13, 2017

It looks like pauseOnExceptionState always 'none' in second invocation. In any case from protocol point of view there is no sense to call Debugger.setPauseOnExceptions twice. Implementation of this method just changes a flag inside V8.
@jkrems
Copy link
Contributor

@jkrems jkrems commented Nov 13, 2017 •
edited

@ak239 Thanks for confirming!

Btw, we don't reset it before connecting/restarting. So it might not be 'none' when connecting after restart. So the hardcoded 'none' is definitely wrong and shouldn't be there.
@ak239
Copy link
Member

@ak239 ak239 commented Nov 13, 2017

I think we should support this case on backend side. When at least one protocol client requests pause on all exceptions - debugger should issue Debugger.paused event for each exception, if at least one client request pause on uncaught exception - issue event for uncaught exception.
I filled bug against V8: issue
@jkrems
Copy link
Contributor

@jkrems jkrems commented Nov 13, 2017 •
edited

Fixed link, edited: https://bugs.chromium.org/p/chromium/issues/detail?id=1246881
@jkrems
Copy link
Contributor

@jkrems jkrems commented Jan 31, 2022

New location:

node/lib/internal/debugger/inspect_repl.js

Lines 1125 to 1128 in 4fbe9e5

await Debugger.setPauseOnExceptions({ state: 'none' });
await Debugger.setAsyncCallStackDepth({ maxDepth: 0 });
await Debugger.setBlackboxPatterns({ patterns: [] });
await Debugger.setPauseOnExceptions({ state: pauseOnExceptionState });

@jkrems jkrems transferred this issue from nodejs/node-inspect Jan 31, 2022
@jkrems
Copy link
Contributor

@jkrems jkrems commented Jan 31, 2022

The fix here may be to just remove the first call with the hard-coded 'none'. If the tests pass, that can likely just land.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
debugger good first issue
3 participants
@Delapouite @ak239 @jkrems