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

Use an UtilityProcess for the extension host #150167

Merged
merged 3 commits into from May 24, 2022

Conversation

alexdima
Copy link
Member

@alexdima alexdima commented May 23, 2022

When launching with VSCODE_USE_UTILITY_PROCESS=1, will use the new UtilityProcess Electron proposed API for the extension host, and use a MessageChannel instead of a named pipe for renderer <-> ext host communication.

Observed problems with UtilityProcess implementation:

  • all env values must be strings, otherwise the env is ignored.
  • args/execArgv shapes are different compared to nodejs
  • on('spawn') / on('exit') are never fired
  • ❗ cannot observe that a process exits gracefully (exit code 0) -- causes the shutdown to always take 6s

To test:

  • check that the extension host process can be brought into debugging mode (e.g. via profiling)

@alexdima alexdima self-assigned this May 23, 2022
@VSCodeTriageBot VSCodeTriageBot added this to the May 2022 milestone May 23, 2022
jrieken
jrieken previously approved these changes May 23, 2022
@deepak1556
Copy link
Contributor

Thanks for setting this up!

  1. all env values must be strings

It was based on type mentioned in https://github.com/nodejs/node/blob/b92bc5931dcc09fda5050ae8a870602d1f6f3c9a/lib/child_process.js#L101, so values were converted natively to a std::map<std::string, std::string>

args don't make it across

Can you clarify which args don't make it ?

kill() takes number, not string

Oh it should support both, which signal did you try ?

on('spawn') is never fired

Confirmed the bug, will address it.

pid is undefined initially

Yeah this behavior is different from the child_process.fork, pid will be valid after spawn event. The reason being we don't perform the actual fork but delegate to chromium for launching the process which makes the process launch async.

Additional note:

exit event only gets emitted for normal termination code is 0 of the process, for all other process terminations we should listen to https://github.com/electron/electron/blob/main/docs/api/app.md#event-child-process-gone event.

* use `app.on('child-process-gone')` to detect crashes
* use `MessagePort.on('close')` to detect the renderer disconnecting
* enable waiting for the extension host to exit even if it means waiting for 6s due to the 'exit' event not firing
@alexdima alexdima merged commit dfa06b8 into main May 24, 2022
@alexdima alexdima deleted the alexd/ext-host-utility-process branch May 24, 2022 14:42
@github-actions github-actions bot locked and limited conversation to collaborators Jul 8, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants