Fix deadlock caused by import 'vscode' from modules loaded via require(esm)#285417
Fix deadlock caused by import 'vscode' from modules loaded via require(esm)#285417mizdra wants to merge 2 commits intomicrosoft:mainfrom
import 'vscode' from modules loaded via require(esm)#285417Conversation
| // Module loading tricks based on `module.registerHooks`. | ||
| // `module.registerHooks` is a generic interceptor that intercepts `require(...)`, `import ...`, and `import(...)`. | ||
| // However, at this time, `NodeModuleInterceptor` only intercepts `import 'vscode'` and `import('vscode')`. | ||
| // This can also intercept `require('vscode')`, but interception by `NodeModuleRequireInterceptor` takes precedence. |
There was a problem hiding this comment.
It's a bit confusing that only require('vscode') gets intercepted by NodeModuleRequireInterceptor. So I tried modifying it as follows to have require('vscode') intercepted by NodeModuleInterceptor.
--- a/src/vs/workbench/api/node/extHostExtensionService.ts
+++ b/src/vs/workbench/api/node/extHostExtensionService.ts
@@ -35,7 +35,7 @@ class NodeModuleRequireInterceptor extends RequireInterceptor {
const originalLoad = node_module._load;
node_module._load = function load(request: string, parent: { filename: string }, isMain: boolean) {
request = applyAlternatives(request);
- if (!that._factories.has(request)) {
+ if (!that._factories.has(request) || request === 'vscode') {
return originalLoad.apply(this, arguments);
}However, this causes an ENAMETOOLONG error.
$ cd repro-vscode-test-hang
$ # Edit files
$ vim
$ git diff
diff --git a/test/index.test.js b/test/index.test.js
index 209123b..3b8b882 100644
--- a/test/index.test.js
+++ b/test/index.test.js
@@ -1,6 +1,10 @@
import assert from 'node:assert/strict';
// The following will hang the test.
-import * as vscode from 'vscode';
+// import * as vscode from 'vscode';
+
+import { createRequire } from 'node:module';
+const require = createRequire(import.meta.url);
+const vscode = require('vscode');
assert.equal(1, 2);
diff --git a/test/runTest.js b/test/runTest.js
index 01ac63b..99c3376 100644
--- a/test/runTest.js
+++ b/test/runTest.js
@@ -12,7 +12,7 @@ async function main() {
extensionDevelopmentPath,
extensionTestsPath,
launchArgs: ['--disable-extensions'],
- // vscodeExecutablePath: '/Users/mizdra/src/github.com/microsoft/vscode/scripts/code.sh',
+ vscodeExecutablePath: '/Users/mizdra/src/github.com/microsoft/vscode/scripts/code.sh',
});
} catch (err) {
console.error(err);
$ npm start
Error: ENAMETOOLONG: name too long, open 'data:text/javascript;base64,Y29uc3QgX3ZzY29kZUluc3RhbmNlID0gZ2xvYmFsVGhpcy5fVlNDT0RFX0lNUE9SVF9WU0NPREVfQVBJKCc5MGMzNTUwMC0yNjY0LTQ2M2...(long text)...107'
at Object.readFileSync (node:fs:441:20)
at t.readFileSync (node:electron/js2c/node_init:2:11013)
at defaultLoadImpl (node:internal/modules/cjs/loader:1112:17)
at loadSource (node:internal/modules/cjs/loader:1742:20)
at Module._extensions..js (node:internal/modules/cjs/loader:1841:44)
at Module.load (node:internal/modules/cjs/loader:1448:32)
at Module._load (node:internal/modules/cjs/loader:1270:12)
at c._load (node:electron/js2c/node_init:2:17993)
at Module._load (file:///Users/mizdra/src/github.com/microsoft/vscode/out/vs/workbench/api/node/extensionHostProcess.js:68:29)
at Module.load (file:///Users/mizdra/src/github.com/microsoft/vscode/out/vs/workbench/api/node/extHostExtensionService.js:32:37)
at Module.load [as _load] (file:///Users/mizdra/src/github.com/microsoft/vscode/out/vs/workbench/api/node/proxyResolver.js:290:33)
at TracingChannel.traceSync (node:diagnostics_channel:328:14)
at wrapModuleLoad (node:internal/modules/cjs/loader:244:24)
at Module.require (node:internal/modules/cjs/loader:1470:12)
at require (node:internal/modules/helpers:147:16)
at file:///Users/mizdra/src/github.com/mizdra/repro-vscode-test-hang/test/index.test.js:8:16
at ModuleJobSync.runSync (node:internal/modules/esm/module_job:454:37)
at ModuleLoader.importSyncForRequire (node:internal/modules/esm/loader:435:47)
at loadESMFromCJS (node:internal/modules/cjs/loader:1544:24)
at Module._compile (node:internal/modules/cjs/loader:1695:5)
at Module._extensions..js (node:internal/modules/cjs/loader:1848:10)
at Module.load (node:internal/modules/cjs/loader:1448:32)
at Module._load (node:internal/modules/cjs/loader:1270:12)
at c._load (node:electron/js2c/node_init:2:17993)
at Module._load (file:///Users/mizdra/src/github.com/microsoft/vscode/out/vs/workbench/api/node/extensionHostProcess.js:68:29)
at Module.load (file:///Users/mizdra/src/github.com/microsoft/vscode/out/vs/workbench/api/node/extHostExtensionService.js:32:37)
at Module.load [as _load] (file:///Users/mizdra/src/github.com/microsoft/vscode/out/vs/workbench/api/node/proxyResolver.js:290:33)
at TracingChannel.traceSync (node:diagnostics_channel:328:14)
at wrapModuleLoad (node:internal/modules/cjs/loader:244:24)
at Module.require (node:internal/modules/cjs/loader:1470:12)
at require (node:internal/modules/helpers:147:16)
at Object.run (/Users/mizdra/src/github.com/mizdra/repro-vscode-test-hang/test/runner.cjs:3:3)
at file:///Users/mizdra/src/github.com/microsoft/vscode/out/vs/workbench/api/common/extHostExtensionService.js:601:42
at new Promise (<anonymous>)
at ExtHostExtensionService._doHandleExtensionTests (file:///Users/mizdra/src/github.com/microsoft/vscode/out/vs/workbench/api/common/extHostExtensionService.js:580:16)
at async ExtHostExtensionService.$extensionTestsExecute (file:///Users/mizdra/src/github.com/microsoft/vscode/out/vs/workbench/api/common/extHostExtensionService.js:558:20)For some reason, originalLoad.apply() appears to return a base64-encoded path generated by NodeModuleInterceptor instead of the module instance. This seems to be a Node.js bug.
Since I couldn't find a workaround, I decided to abandon intercepting require('vscode') with NodeModuleInterceptor.
| // `module.registerHooks` is a generic interceptor that intercepts `require(...)`, `import ...`, and `import(...)`. | ||
| // However, at this time, `NodeModuleInterceptor` only intercepts `import 'vscode'` and `import('vscode')`. | ||
| // This can also intercept `require('vscode')`, but interception by `NodeModuleRequireInterceptor` takes precedence. | ||
| // In the future, we can consider migrating all interception logic to `NodeModuleInterceptor` and removing `NodeModuleRequireInterceptor`. |
There was a problem hiding this comment.
module._load was able to return an object from a hook. However, hooks registered via module.registerHooks currently cannot do this. Therefore, NodeModuleInterceptor circumvents this limitation by using base64-encoded source text and NodeModuleInterceptor._vscodeImportFnName. This is a complex workaround.
Incidentally, it appears Node.js plans to implement hooks that can return objects. Once implemented, this complex workaround could be removed. Porting the logic from NodeModuleRequireInterceptor to NodeModuleInterceptor should also become easier.
|
The review is ready. Could you please review it? @mjbvz |
|
Is there anything I should do to get this Pull Request accepted? Since To aid understanding of the issue, here's a bit more detail on how to reproduce it. While I mentioned that the deadlock can be reproduced with |
|
Any way to get this reviewed @jrieken? Would be a nice improvement :) |
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Fixes a deadlock when an extension is loaded via require(esm) and then performs an ESM import 'vscode' by switching from a custom loader-thread approach to module.registerHooks.
Changes:
- Replaces the ESM loader-thread interceptor with a
module.registerHooks-based interceptor for resolvingimport 'vscode'. - Removes MessageChannel-based cross-thread resolution and inlines resolution logic in-process.
- Updates ExtHost initialization comments to reflect the new interception approach.
Show a summary per file
| File | Description |
|---|---|
| src/vs/workbench/api/node/extHostExtensionService.ts | Reworks ESM interception to use module.registerHooks to avoid loader-thread deadlocks when importing vscode. |
Copilot's findings
- Files reviewed: 1/1 changed files
- Comments generated: 4
|
continues in #313500. Not ideal that the agent created a new PR, but your OG commit is in there and this PR will get "thank you'd" by our tools. And hereby also by me: Thanks for this contribution, this wasn't a trivial find nor fix |
|
Thank you for the review as well. It must have been very challenging to understand the background and the fix for this issue. I am glad you accepted my fix and refined it further. |
|
It looks like this fix was released in version 1.119. Thank you for releasing it. By the way, will this change be included in the release notes? Since it’s a bug fix, I thought it might be listed at https://code.visualstudio.com/updates/v1_119#_thank-you, but it doesn’t seem to be there. If the omission is intentional, that’s fine. If it’s not intentional, should I ask someone to add it? |
|
@mizdra Thanks for the contribution! Adding it to the 1.119 release notes - should be live later today. |
|
@ntrogh Thank you! |
close: #285297
Background
An extension with code like the following will cause a deadlock.
The detailed reasons are explained at #285297 (comment). This PR attempts to fix that issue using
module.registerHooks.How to test
The test for https://github.com/mizdra/repro-vscode-test-hang can run to completion without hanging.