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

TypeScript's dependency on electron limits the heap size to a point that breaks monorepos, and tsserver-bridge is no longer a workaround #151245

Closed
MarcCelani-at opened this issue May 11, 2022 · 22 comments · Fixed by #191019
Assignees
Labels
feature-request Request for new features or functionality insiders-released Patch has been released in VS Code Insiders javascript JavaScript support issues typescript Typescript support issues verification-needed Verification of issue is requested verification-steps-needed Steps to verify are needed for verification verified Verification succeeded

Comments

@MarcCelani-at
Copy link

MarcCelani-at commented May 11, 2022

Bug Report

Electron enforces a heap memory limit (due to pointer compression) that causes typescript to crash in large mono-repo. This was originally reported here, and then on the electron side here. One common workaround is to use tsserver-bridge to cause vscode to launch the typescript editor service in node rather than electron. Starting in typescript 4.6, it appears that the tsserver-bridge workaround no longer works.

Error when using tsserver-bridge:

Info 0    [09:17:36.144] Starting TS Server
Info 1    [09:17:36.145] Version: 4.6.4
Info 2    [09:17:36.145] Arguments: /my/path/to/node /my/path/to/.vscode/typescript/lib/_tsserver.js --useInferredProjectPerProjectRoot --enableTelemetry --cancellationPipeName /var/folders/l1/qrb13dbx3l3_wjlh4kgxnpm80000gn/T/vscode-typescript501/8d65dce0fd217a13c1cc/tscancellation-f05eabd1e2d0ae3f8300.tmp* --logVerbosity verbose --logFile /my/path/to/Code/logs/20220509T134031/exthost1/vscode.typescript-language-features/tsserver-log-Y6lofO/tsserver.log --locale en --noGetErrOnBackgroundUpdate --validateDefaultNpmLocation --useNodeIpc
Info 3    [09:17:36.145] Platform: darwin NodeVersion: 16 CaseSensitive: false
Info 4    [09:17:36.145] ServerMode: undefined syntaxOnly: false hasUnknownServerMode: undefined
Info 5    [09:17:36.149] Binding...
Info 6    [09:17:36.153] event:
    {"seq":0,"type":"event","event":"typingsInstallerPid","body":{"pid":28855}}
Err 7     [09:17:36.163] Exception on executing command unknown:

    process.send is not a function

    TypeError: process.send is not a function
        at IpcIOSession.writeMessage (/my/path/to/.vscode/typescript/lib/_tsserver.js:177001:29)
        at IpcIOSession.Session.send (/my/path/to/.vscode/typescript/lib/_tsserver.js:172938:22)
        at IpcIOSession.Session.event (/my/path/to/.vscode/typescript/lib/_tsserver.js:172947:22)
        at IpcIOSession.IOSession.event (/my/path/to/.vscode/typescript/lib/_tsserver.js:176966:48)
        at NodeTypingsInstaller.event (/my/path/to/.vscode/typescript/lib/_tsserver.js:176926:31)
        at Immediate.<anonymous> (/my/path/to/.vscode/typescript/lib/_tsserver.js:176776:71)
        at processImmediate (node:internal/timers:464:21)


Unfortunately, tsserver-bridge is no longer supported (author thinks it is no longer required since you can adjust heap size, but electron limits heap size because it implements pointer compression).

Electron has been so far unwilling to provide builds that disable pointer compression, and even if they did, vscode would need to begin leveraging it.

🔎 Search Terms

tsserver-bridge, heap limit, memory limit, process.send

🕗 Version & Regression Information

  • This changed between versions 4.5.4 and 4.6.4

🙁 Actual behavior

Large mono repos hit the memory limit and tsserver-bridge is not a workaround

🙂 Expected behavior

Allow tsserver-bridge to work or get off of electron

@jakebailey
Copy link
Member

ipc was recently added to tssever for perf reasons; if you're using VS Code I'm guessing there's a flag that could be passed to disable it? Maybe we (the server or somewhere in the client) need to be checking for send support?

@mjbvz

@mjbvz
Copy link
Collaborator

mjbvz commented May 11, 2022

If you pass TSServer --useNodeIpc, it will use node's ipc instead of stdio

tsserver-bridge likely needs to be updated to support ipc too. There's no user facing flag to disable this

@MarcCelani-at
Copy link
Author

tsserver-bridge likely needs to be updated to support ipc too.

what does this mean? The whole tsserver-bridge package is pretty, short, would appreciate a pointer of what to change here.

The stack trace I shared is already using IpcIOSession (not IOSession), so I think that means it is already using node ipc? I tried passing the flag by patching tsserver-bridge, and I the flag is being passed (according to arguments that are logged in tsserver), but that didn't fix it. Something else needs to change too?

@jakebailey
Copy link
Member

jakebailey commented May 11, 2022

The bridge code creates a script in place of tsserver that spawns the real tsserver as a subprocess, inheriting stdin/stdout/stderr so that subprocess can communicate with the client. But newer versions of TS and VS Code support IPC, and so the wrapper script in the middle has to proxy the IPC traffic as well if --useNodeIpc was passed (and probably run it via the version of spawn that enables IPC, too).

Otherwise, the client and server will attempt to communicate over IPC and then crash in the IPC code, as your trace shows.

@andrewbranch
Copy link
Member

We discussed this today and decided VS Code should possibly let users opt into spawning TS Server in their own installed version of Node (and inform TS Server so it can communicate over STDIO as before, IIUC). @mjbvz was going to see if other language extensions have run into something similar. Matt, can I transfer this issue to VS Code for now?

@MarcCelani-at
Copy link
Author

Is there a related issue on vscode side?

@RyanCavanaugh
Copy link
Member

Assigning to @mjbvz since I don't have rights to transfer this to the VS Code repo 😅

@typescript-bot
Copy link

This issue has been marked as 'External' and has seen no recent activity. It has been automatically closed for house-keeping purposes.

@MarcCelani-at
Copy link
Author

Can we keep this open until the vscode team actively takes ownership?

@mjbvz mjbvz reopened this Jun 3, 2022
@mjbvz mjbvz transferred this issue from microsoft/TypeScript Jun 3, 2022
@mjbvz mjbvz added this to the June 2022 milestone Jun 3, 2022
@MarcCelani-at
Copy link
Author

Thanks for taking ownership of this task in the vscode project.

I'm trying to understand if our team needs to work around this issue by patching tsserver-bridge or not. Is this issue a priority for vscode, and is it easy to fix?

@mjbvz
Copy link
Collaborator

mjbvz commented Jun 8, 2022

Our proposal is to let users specify a path to node and run TSServer using that. It's not a priority though so would definitely appreciate a PR :)

@MarcCelani-at
Copy link
Author

@mjbvz : Can you point me to relevant code? I've never touched this codebase.

@MarcCelani-at
Copy link
Author

Okay, here is a rough proposal...

New config:
typescript.runtime: string | null // Path to runtime bin or the name of a binary that is set in PATH

I'll update and rename serverProcess.electron.ts to consume these configurations and change how the fork happens to use this runtime value.

@mjbvz
Copy link
Collaborator

mjbvz commented Jun 9, 2022

Yes that sounds like the correct approach. A few points:

  • For security reasons, we can't just use this value from the workspace settings (because it could point to an evil version of node). You can take a look at how the php.validate.executablePath setting for a similar example of how this should work

  • I'd even say we only want the new setting to be configurable as user setting and never as a workspace setting. We don't expect many users to configure it and if they do, they likely want the custom version of node to always be used

  • We need to log when a custom node version is being used (we already log the TS version). This will help us track down if a custom node version is causing issues

@MarcCelani-at
Copy link
Author

Is the "validate" word a naming convention, and what does it imply? Where does the validation logic happen?

@mjbvz
Copy link
Collaborator

mjbvz commented Jun 14, 2022

validate is for error reporting. It's not relevant here (I was just sharing an example of another setting that contains a path to an executable)

@mjbvz
Copy link
Collaborator

mjbvz commented Jun 15, 2022

Actually let's merge this issue into #127105 which was already tracking this

Duplicate of #127105

@mjbvz
Copy link
Collaborator

mjbvz commented Aug 9, 2023

Reopening to track letting users point to a node executable for running tsserver

@mjbvz mjbvz reopened this Aug 9, 2023
@microsoft microsoft unlocked this conversation Aug 11, 2023
@gabritto
Copy link
Member

@mjbvz regarding your comment above:

We need to log when a custom node version is being used (we already log the TS version). This will help us track down if a custom node version is causing issues

Where should that be logged?

@jakebailey
Copy link
Member

Reopening to track letting users point to a node executable for running tsserver

I hadn't noticed, but #172719 is actually also the same request!

@mjbvz mjbvz added feature-request Request for new features or functionality typescript Typescript support issues javascript JavaScript support issues labels Sep 6, 2023
@vscodenpa vscodenpa added unreleased Patch has not yet been released in VS Code Insiders insiders-released Patch has been released in VS Code Insiders and removed unreleased Patch has not yet been released in VS Code Insiders labels Sep 6, 2023
@bhavyaus bhavyaus added the verification-needed Verification of issue is requested label Sep 26, 2023
@meganrogge meganrogge added the ~verification-steps-needed Steps to verify are needed (with bot comment) label Sep 26, 2023
@vscodenpa
Copy link

Friendly ping! Looks like this issue requires some further steps to be verified. Please provide us with the steps necessary to verify this issue.

@vscodenpa vscodenpa added verification-steps-needed Steps to verify are needed for verification and removed ~verification-steps-needed Steps to verify are needed (with bot comment) labels Sep 26, 2023
@mjbvz
Copy link
Collaborator

mjbvz commented Sep 26, 2023

For verification, this is a duplicate of #172719. Marking as verified based on that issue

@mjbvz mjbvz added the verified Verification succeeded label Sep 26, 2023
@github-actions github-actions bot locked and limited conversation to collaborators Oct 21, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
feature-request Request for new features or functionality insiders-released Patch has been released in VS Code Insiders javascript JavaScript support issues typescript Typescript support issues verification-needed Verification of issue is requested verification-steps-needed Steps to verify are needed for verification verified Verification succeeded
Projects
None yet
Development

Successfully merging a pull request may close this issue.

10 participants