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

Move extension host out of the workbench for process reuse #123592

Closed
rzhao271 opened this issue May 11, 2021 · 7 comments · Fixed by #135774
Closed

Move extension host out of the workbench for process reuse #123592

rzhao271 opened this issue May 11, 2021 · 7 comments · Fixed by #135774
Assignees
Labels
debt Code quality issues engineering VS Code - Build / issue tracking / etc. extension-host Extension host issues insiders-released Patch has been released in VS Code Insiders sandbox Running VSCode in a node-free environment
Milestone

Comments

@rzhao271
Copy link
Contributor

As part of #120431, we have to move the extension host out of the workbench.
Currently, the workbench is the parent of the extension host. When we do a reload, the workbench is recreated. When process reuse is enabled, the workbench PID stays the same throughout the reload, but the node environment is still taken down. Due to this cleanup, the child exit signal from the extension host is not handled, creating a zombie.
By moving the extension host out of the workbench, another longer-lived process can handle the signal instead.

@alexdima do you have any specific ideas as to how the new architecture should look?

@rzhao271 rzhao271 added the debt Code quality issues label May 11, 2021
@rzhao271 rzhao271 added this to the May 2021 milestone May 11, 2021
@deepak1556 deepak1556 added extension-host Extension host issues sandbox Running VSCode in a node-free environment engineering VS Code - Build / issue tracking / etc. and removed debt Code quality issues labels May 11, 2021
@deepak1556 deepak1556 removed this from the May 2021 milestone May 17, 2021
@bpasero bpasero added this to the June 2021 milestone May 18, 2021
@bpasero bpasero added this to To do in Sandbox Renderer Jun 7, 2021
@alexdima
Copy link
Member

alexdima commented Jun 8, 2021

Unless there are major objections, as a first step, I would try to spawn the extension host process from the shared process and establish a pipe between the extension host process and the renderer process using nodejs API. We can then remove this nodejs usage later when we get better support for IPC inside a sandboxed renderer. Given we will pipe things directly between the extension host process and the renderer process, I don't think there will be any extra load on the shared process.

@bpasero
Copy link
Member

bpasero commented Jun 8, 2021

@alexdima Yeah that sounds like a good strategy to get going. This would reduce the sandbox related problem to switching from a node.js IPC solution to a sandbox compatible IPC mechanism 👍

One thing I was curious when I thought about how to move the extension host: currently a lot of extension host related code lives in src/vs/workbench/api but we actually will be spawning the extension host not within a workbench context, but from shared process. Would it make sense to also think about a better place for the extension host code itself? It almost seems to be this is becoming its own layer (vs/exthost)? Obviously, all the mainThread.* things would remain in the workbench land as the implementation of the protocol.

PS: In my 1on1 with @rzhao271 yesterday we discussed to have a call with you on IPC solutions for the extension host once @deepak1556 is back from vacation (later this milestone).

@alexdima
Copy link
Member

alexdima commented Jun 9, 2021

One thing I was curious when I thought about how to move the extension host: currently a lot of extension host related code lives in src/vs/workbench/api but we actually will be spawning the extension host not within a workbench context, but from shared process. Would it make sense to also think about a better place for the extension host code itself? It almost seems to be this is becoming its own layer (vs/exthost)? Obviously, all the mainThread.* things would remain in the workbench land as the implementation of the protocol.

You're right. I took a quick glance at the process spawning code in localProcessExtensionHost.ts and it uses the following vs/workbench imports:

  • vs/workbench/api/common/extHost.protocol
  • vs/workbench/services/environment/electron-sandbox/environmentService
  • vs/workbench/services/environment/electron-sandbox/shellEnvironmentService
  • vs/workbench/services/extensions/common/extensionDevOptions
  • vs/workbench/services/extensions/common/extensionHostProtocol
  • vs/workbench/services/extensions/common/extensions
  • vs/workbench/services/extensions/common/remoteConsoleUtil
  • vs/workbench/services/host/browser/host
  • vs/workbench/services/lifecycle/common/lifecycle
  • vs/workbench/services/output/common/output

Maybe some of these imports disappear when I will try to move the code, but if I understand correctly, these imports are off-limit to code running in the shared process?

Also, to set some expectations, I am sorry that I will only be able to work on the the sandbox effort after doing all my other plan items (so it gets a muscle again).

@bpasero
Copy link
Member

bpasero commented Jun 9, 2021

If it makes things easier, maybe we decouple the layer-issue from the sandbox task. I see no problem with some code in shared process spawning a main-entry point that is defined by vs/workbench, in the end we do something similar already given we open windows from the main process that are implemented by workbench:

return require('vs/workbench/electron-sandbox/desktop.main').main(configuration);

And thus the layer thing would be a follow up issue to tackle as debt? I believe it would be nice to have a new layer specifically for the extension host and then push those things down to platform that are reused between extension host and workbench.

However, it looks like some of the dependencies are actually required to instantiate the extension host initially so even if we were to move all things to a new layer, that init data is still needed. I see this very similar to the way we start a remote extension host: we send some payload from the renderer to the remote extension host manager to start a new extension host. I wonder if this model should be the same for a sandboxed renderer: all these types are accessible in sandbox and the payload can be send over from the renderer to tell the shared process to spawn a new extension host. On the downside, this would mean we could not start an extension host at the same time we open a window, but to be honest, this would slow down the window significantly, so I am not sure we could even do that anyway, given how much we care about startup perf.

@alexdima
Copy link
Member

@bpasero I took a look at how we do things in the remote case and I don't have so many concerns about the imports / layering, I think we should be able to eliminate most of the vs/workbench imports if we extract a piece of code focused solely on forking the process and establishing a socket with it.

@deepak1556 deepak1556 removed this from the June 2021 milestone Jun 22, 2021
@bpasero bpasero added this to the On Deck milestone Jun 24, 2021
@bpasero bpasero added the debt Code quality issues label Aug 31, 2021
@bpasero bpasero modified the milestones: On Deck, September 2021 Sep 6, 2021
@bpasero bpasero moved this from To do to In progress in Sandbox Renderer Sep 20, 2021
@deepak1556
Copy link
Contributor

Fixed with 18cde69

@deepak1556 deepak1556 moved this from In progress to Done / Deferred in Sandbox Renderer Sep 22, 2021
@rzhao271 rzhao271 removed their assignment Sep 24, 2021
@bpasero bpasero reopened this Sep 29, 2021
@bpasero bpasero modified the milestones: September 2021, October 2021 Sep 29, 2021
@bpasero
Copy link
Member

bpasero commented Sep 29, 2021

Will continue in October given the recent revert.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
debt Code quality issues engineering VS Code - Build / issue tracking / etc. extension-host Extension host issues insiders-released Patch has been released in VS Code Insiders sandbox Running VSCode in a node-free environment
Projects
No open projects
Sandbox Renderer
  
✔️ Done / Deferred
Development

Successfully merging a pull request may close this issue.

5 participants
@bpasero @deepak1556 @alexdima @rzhao271 and others