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

Adopt utility process for shared process and file watchers #154050

Closed
bpasero opened this issue Jul 4, 2022 · 19 comments
Closed

Adopt utility process for shared process and file watchers #154050

bpasero opened this issue Jul 4, 2022 · 19 comments
Assignees
Labels
on-testplan plan-item VS Code - planned item for upcoming sandbox Running VSCode in a node-free environment shared-process VS Code shared process issues
Milestone

Comments

@bpasero
Copy link
Member

bpasero commented Jul 4, 2022

This is a follow up from #92164 and covers remaining work to eventually enable sandboxed renderers fully in Electron via app.enableSandbox().

This means that our shared process has to move away from a node.js enabled browser window to the new utility process.

Breaking down the usages today:

  • extension management
  • settings sync
  • profiles
  • terminals
  • file watcher

Some initial thoughts:

  • the shared process should probably just change to be a utility process as a first step
    • however, any code that relies on the browser window network stack instead has to leverage Electrons net APIs from the electron-main process to not loose proxy support
    • this can probably be done by implementing some kind of IRequestService that is backed by a main process service implementation
  • any child process has to decide whether it wants to lift up to a utility process off the main process or remain inside the shared process

//cc @alexdima

@bpasero bpasero added plan-item VS Code - planned item for upcoming shared-process VS Code shared process issues sandbox Running VSCode in a node-free environment labels Jul 4, 2022
@bpasero bpasero added this to the On Deck milestone Jul 4, 2022
@sandy081
Copy link
Member

sandy081 commented Jul 4, 2022

settings sync is safe to be removed as it does not use any non sandbox stuff like node apis.

@bpasero
Copy link
Member Author

bpasero commented Jul 4, 2022

To clarify: we can still have a shared process in the end, I do not have strong feelings, but it would be node.js only and not electron browser enabled. I think the benefit of having another process offload work from the main process is still valid.

@sandy081
Copy link
Member

sandy081 commented Jul 4, 2022

One concern I have with shared process not having electron browser enabled is the network proxy and If we can enable request service to redirect to main as you mentioned, this shall be fine. But I am wondering how much load we gonna put on main and if that impacts perf.

@bpasero
Copy link
Member Author

bpasero commented Jul 4, 2022

As far as I know, network load is handled from another utility process, so I think that should be fine, right @deepak1556 ?

@deepak1556
Copy link
Collaborator

Yes that is correct, the main process is only responsible for initiating the request from the JS thread, the actual work is done by the network utility process.

@Tyriar
Copy link
Member

Tyriar commented Jul 4, 2022

For the terminal part in shared proc, it just passes manages the pty host process and passing requests to/from it, so good there.

#153182 seems like a blocker to enabling utility processes though.

@bpasero
Copy link
Member Author

bpasero commented Jul 6, 2022

@Tyriar does #133895 still apply?

@Tyriar
Copy link
Member

Tyriar commented Jul 6, 2022

@bpasero I think so, but #153182 was happening with a single window

@bpasero
Copy link
Member Author

bpasero commented Jul 6, 2022

Sorry for the confusion, I was not suggesting the issues are related, rather was wondering if moving to utility process would help for #133895

@Tyriar
Copy link
Member

Tyriar commented Jul 6, 2022

Oh, I haven't looked at any of the utility process stuff yet, so not sure what the difference between a regular node (pty host now) and utility processes are. If the pty host was a utility process off the main process would I be able to setup direct comm channels between the renderer and the pty host?

@bpasero
Copy link
Member Author

bpasero commented Jul 6, 2022

@Tyriar yes that is the idea, in short:

  • a utility process is a new electron main process API similar to cp.fork (created by Deepak)
  • you get a full node.js environment within the utility process and can even further fork child processes
  • you can send a message port into the utility process and have the other end connected to a renderer
  • you can then transfer data exclusively between utility process and renderer

(the extension host has adopted and is validating this approach in insiders)

@bpasero
Copy link
Member Author

bpasero commented Jul 6, 2022

I think you can even keep having a PTY host (as a utility process) and then child processes for each terminal, but I would then do 1 message port per window to offload communication.

@deepak1556 just wondering: can I send a message port further down to child processes to even offload work from the utility process or is the message port limited to operating only from the utility process?

@Tyriar
Copy link
Member

Tyriar commented Jul 6, 2022

Sounds great! 🙂

@deepak1556
Copy link
Collaborator

Message port is limited to operating only from the utility process. I would prefer to keep it this way since otherwise we could have untrusted child processes talking directly to the renderer and serves as a way to break out of the renderer sandbox.

bpasero referenced this issue Jul 11, 2022
* Reactor telemetry appenders to reduce duplicated code

* Update tsconfig

* Do not use Object.create(null) because we need object.hasOwnProperty
@sandy081
Copy link
Member

Reverted request service in shared process to use browser XHR because of the bug #155204

@Tyriar Tyriar added this to the On Deck milestone Dec 13, 2022
@bpasero
Copy link
Member Author

bpasero commented Jan 3, 2023

Since the request service issue has been addressed, I am starting to work on this for January. I already figured out that the file watcher has to move out of the shared process into its own utility process. @Tyriar I will let you know how that work progresses because I feel once I made the move, PTY host should also move out and by then I am hopefully having a set of utilities in place to make the adoption easier. I am building on top of the code from AlexD used for the extension host.

@bpasero bpasero changed the title Adopt utility process for shared process and set app.enableSandbox() Adopt utility process for shared process, file watchers and terminal host and set app.enableSandbox() Jan 3, 2023
@bpasero bpasero modified the milestones: On Deck, January 2023 Jan 3, 2023
@bpasero bpasero modified the milestones: January 2023, February 2023 Jan 23, 2023
@bpasero
Copy link
Member Author

bpasero commented Feb 16, 2023

Some initial work landed on main and setting window.experimental.sharedProcessUseUtilityProcess to true will have the shared process run from a utility process, same as the file watcher.

There are some issues with integrated terminal that I cannot reproduce running out of sources but appear in the built version. But I was able to browse and install an extension.

@bpasero bpasero changed the title Adopt utility process for shared process, file watchers and terminal host and set app.enableSandbox() Adopt utility process for shared process, file watchers and terminal host and set app.enableSandbox()üpoiu9z88u9i+#üüß Feb 20, 2023
@bpasero bpasero changed the title Adopt utility process for shared process, file watchers and terminal host and set app.enableSandbox()üpoiu9z88u9i+#üüß Adopt utility process for shared process, file watchers and terminal host and set app.enableSandbox() Feb 20, 2023
@bpasero
Copy link
Member Author

bpasero commented Feb 20, 2023

Terminal works as well now! It was just an issue where we passed an unsupported execArgv to node.js via fork but took me days to actually find 🚀

#174852

@bpasero bpasero modified the milestones: February 2023, March 2023 Feb 20, 2023
@bpasero
Copy link
Member Author

bpasero commented Feb 24, 2023

I will close this one out for Feb since the significant portion of the work has already happened. Will create a new issue for moving terminals out, but for now they coexist as before.

@bpasero bpasero closed this as completed Feb 24, 2023
@bpasero bpasero changed the title Adopt utility process for shared process, file watchers and terminal host and set app.enableSandbox() Adopt utility process for shared process and file watchers Feb 24, 2023
@github-actions github-actions bot locked and limited conversation to collaborators Apr 10, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
on-testplan plan-item VS Code - planned item for upcoming sandbox Running VSCode in a node-free environment shared-process VS Code shared process issues
Projects
None yet
Development

No branches or pull requests

6 participants