microsoft / vscode-js-debug

A DAP-compatible JavaScript debugger. Used in VS Code, VS, + more
MIT License
1.67k stars 279 forks source link

Possible race condition in source mapping / breakpoint binding within dynamic imports #1510

Open dqh-au opened 1 year ago

dqh-au commented 1 year ago

EDIT: I have a simpler way to trigger this behaviour, see following comment

I have a breakpoint inside dynamically import()ed code that sometimes works. I can reliably make it work, or not, by enabling or disabling an entirely different breakpoint.

It does not work if the imported-file-breakpoint is the only breakpoint enabled. It does work if I also set a breakpoint in the importing code, just before I invoke the imported code with the breakpoint in it.

Complicating matters, the imported code has been generated and contains an inline source map pointing to a project file that at that point, has not been loaded by the runtime.

The code looks something like the following:

const data = { ... }
import('generated.js')
    .then(
        (module) =>
        {
            fs.writeFile('output.html', module.default(data)); // magic breakpoint needs to go on this line
        });

If I place a breakpoint on the fs.writeFile line, then a breakpoint inside the file that was transpiled to 'generated.js' will work, breaking execution within the call to module.default(data). However it does not work if the breakpoint on fs.writeFile is removed or disabled.

I can generate and send trace logs, please let me know you want them.

If this is expected behaviour, is there a programatic way I can force node and the vscode js debugger to get in sync?

Version: 1.74.2 (Universal) Commit: e8a3071ea4344d9d48ef8a4df2c097372b0c5161 Date: 2022-12-20T10:26:09.430Z Electron: 19.1.8 Chromium: 102.0.5005.167 Node.js: 16.14.2 V8: 10.2.154.15-electron.0 OS: Darwin arm64 22.2.0 Sandboxed: No

dqh-au commented 1 year ago

Actually, it's not quite as I described. The behaviour changes whether or not ANY breakpoint has been disabled via unticking in vscode, rather than deleted. In my previous tests I'd been disabling the 'magic breakpoint', but I have since found that there is nothing special about breaking just before invoking the dynamically imported code.

I'm actually importing and invoking 9 generated js files. I can make breakpoints concurrently set in all 9 files work 'reliably' IF:

  1. No breakpoints are disabled; and
  2. invocation of imported code is delayed using setTimeout(..., 100)

If I disable any breakpoint (without deleting it) then none will work.

If I don't delay at all, 1 or 2 randomish breakpoints will work. The number of breakpoints that work increases as I increase the delay. All 9 start to work on my dev machine at 4ms. By 'work' I mean that if I launch my process once, execution will pause at each of the 9 breakpoints, with me manually resuming after each breakpoint.

connor4312 commented 1 year ago

This is generally the responsibility of the outFiles option; unlike browsers, Node.js (where I assume you're running) does not provide us a way to pause before a script with a sourcemap is evaluated. Therefore, we use the outFiles to scan the disk ahead of time for files with sourcemaps. I'm also guessing you're creating generate.js at runtime somehow, or it is outside your configured outFiles. In this case the debugger will parse the sourcemap and set any breakpoints that apply, but this may or may not before any relevant code is evaluated.

Collecting a trace log using the instructions in the issue template will provide more information.

dqh-au commented 1 year ago

I see. Yes - this is running under node 18, and I am generating generated.js* using rollup, and then executing that code to produce a static HTML file.

I have emailed you a log of a run with 9 enabled breakpoints, all in generated code, and without the call to setTimeout(). In this run, one breakpoint did trigger but I can't remember which one. Please let me know if you'd like any specific scenarios tested.

dqh-au commented 1 year ago

It sounds like I should experiment with executing the generated code in a node subprocess and rely on vscode auto-attach, would you expect the race to be avoided with this approach?

dqh-au commented 1 year ago

(that does work nicely, based on initial testing. I'll rearchitect my build system to use this approach unless you can suggest a way to get deterministic breakpoint behaviour out of dynamically-generated-and-then-import()ed code)

connor4312 commented 1 year ago

Yea, executing the generated code in a subprocess should work. New debug sessions--each child process is a new session--will run a targeted re-scan of files (though if you run into problems, you should try on the nightly build where scanning has been reworked and improved.)

connor4312 commented 1 year ago

Though the 'proper' way to do this is with a prelaunch task that builds your code before starting to debug it. As a slight benefit, the generation will run a little faster if it's not under the debugger.

dqh-au commented 1 year ago

Thanks a lot for your help :) In my case I need to run the build system under the debugger, as I am actively developing a custom build system alongside the project itself. So I'll implement the final HTML generation phase as a single subprocess that executes each generated file and writes the output.

connor4312 commented 1 year ago

👍 good luck in your project

dqh-au commented 1 year ago

Actually - perhaps there is still a bug here? While a race is understandable if node doesn't give you a way to block until the source map has been processed, why would disabling one breakpoint prevent all of them from working, no matter how long execution of imported code is delayed? (EDIT: sometimes)

dqh-au commented 1 year ago

For anyone coming across this situation in the future, while a node subprocess works, using a node 'Worker' is a much better developer experience. In particular, it's simpler to pass data to the worker code, simpler to get the output, and all worker threads within a node process share a single debug console within vscode.

dqh-au commented 1 year ago

Issue recap: Early breakpoints inside dynamic import()ed code don't work unless a delay is introduced.

Apologies if re-opening is not ok but I want to return to dynamic imports for other reasons and I am facing this issue again.

I have found that placing await new Promise((resolve) => setTimeout(resolve, 25)); in the imported script works, and on my machine this I can use a timeouts as low as 2ms. But, this approach is obviously just making it more likely that the breakpoint will work by shifting the race. Not something I'd want to ship to users.

I previously asked if there was a general way that this issue could be fixed, and the answer was no because Node doesn't provide a way to pause the script. But what if the import()ed script contained code to wait for vscode?

I am well outside my experience here, but could the node inspector API could somehow be used to block execution of a dynamic import()ed script until vscode has processed sourcemaps? I have seen that inspector.waitForDebugger() exists. Calling it from my import()ed script blocks indefinitely. The documentation states that it "Blocks until a client (existing or connected later) has sent Runtime.runIfWaitingForDebugger command." . In theory it seems like vscode could send that command on its own inspector session after loading sourcemaps?

connor4312 commented 1 year ago

Sorry, I thought I had an idea for this scenario, but misremembered the issue.

Those functionalities aren't too useful in this case. waitForDebugger() is actually used by the bootloader, but once attached that will just block the program until a debugger sends Runtime.runIfWaitingForDebugger again. It doesn't look like calling that method emits anything back to already-attached debuggers that we could use to signal.

In recent Node.js versions, the instrumentation breakpoint that asks the runtime to pause beforeScriptWithSourceMapExecution has been nominally been enabled. However, this doesn't appear to actually work for my test case, but maybe it does for you. It can be enabled in the debugger by setting pauseForSourceMap: true in your launch.json.

Bear in mind this comes with non-negligible performance implications that could impact projects that pull in a large number of files with sourcemap declarations. Notably these include node modules if authors have built their package with sourcemaps enabled, even if those sourcemaps are not actually published.

dqh-au commented 1 year ago

I think I had tried pauseForSourceMap before, but I just tried again and it isn't working for me either.

My use case is npx nakedjsx, a new JSX based static site generator that I recently released. It generates intermediate .mjs files from page source files, which are then import()ed to produce one or more HTML files as a side effect. Setting breakpoints in the page source files works if I add that slight delay, but otherwise doesn't work most of the time.

I had another thought, I wonder if vscode could utilise the experimental Node.js loader hooks to get deterministic breakpoint behaviour for dynamic imports. Perhaps they could be magically injected by your debugger on attach / launch?

connor4312 commented 1 year ago

That's a neat idea. I'll explore.

dqh-au commented 1 year ago

(and if this works, it would be amazing if it still worked when using other loader hooks, such has the yarn pnp loader. I believe hooks are implemented as a chain so the building blocks of a generalised solution seem to be present)

MinisculeGirraffe commented 7 months ago

I can confirm I am impacted by this bug too.

My use case is around debugging lambda functions. Where executing the script doesn't actually do anything and we need a shim to invoke the function handler.

I checked and the sourcemaps are loaded properly, but the breakpoints don't seem to get set because the file is behind a dynamic import.

 {
          "type": "node",
          "request": "launch",
          "name": "Debug Lambda",
          "pauseForSourceMap": true,
          "program": "import(\"${workspaceFolder}/${input:selectLambda}/lib/index.mjs\").then((loadedModule) => loadedModule.handler(JSON.parse(require(\"fs\").readFileSync(\"${workspaceFolder}/${input:selectLambda}/src/event.json\", { encoding: \"utf8\", flag: \"r\" }).toString())))",
          "preLaunchTask": "BuildLambda",
          "autoAttachChildProcesses": true,
          "console": "integratedTerminal",
          "profileStartup": true,
          "stopOnEntry": true,
          "smartStep": false,
          "outFiles": [
            "${workspaceFolder}/${input:selectLambda}/lib/*",
          ],
          "runtimeArgs": ["--enable-source-maps","-e"]
        }