Create extension host processes from a node worker in the main process#135774
Create extension host processes from a node worker in the main process#135774
Conversation
|
@alexdima looks a lot better now in CI, 👏 . maybe as a next step you restore the worker approach for forking the extension host? |
| import { IExtensionHostStarter, ipcExtensionHostStarterChannelName } from 'vs/platform/extensions/common/extensionHostStarter'; | ||
|
|
||
| registerSharedProcessRemoteService(IExtensionHostStarter, ipcExtensionHostStarterChannelName, { supportsDelayedInstantiation: true }); | ||
| const location = 'main' as 'main' | 'shared'; |
There was a problem hiding this comment.
Does this code mean that for now, we don't register the shared process remote service anymore?
There was a problem hiding this comment.
I wanted to have an easy way to switch between the main process spawning the extension host and the shared process spawning the extension host. Currently, this means the extension host is spawned from the main process.
| } | ||
|
|
||
| onDynamicStdout(id: string): Event<string> { | ||
| return this._proxy!.onDynamicStderr(id); |
There was a problem hiding this comment.
Should this be onDynamicStdout?
src/vs/platform/extensions/electron-main/directMainProcessExtensionHostStarter.ts
Show resolved
Hide resolved
src/vs/platform/extensions/electron-main/workerMainProcessExtensionHostStarter.ts
Show resolved
Hide resolved
| } | ||
| const pid = this._process.pid; | ||
| this._logService.info(`Waiting for extension host with pid ${pid} to exit.`); | ||
| await Promise.race([Event.toPromise(this.onExit), timeout(maxWaitTimeMs)]); |
There was a problem hiding this comment.
To clarify: we are not explicitly terminating the extension host here because we assume that the window is closing which will close the socket and then should trigger the self-termination from within the extension host?
I wonder if maybe a more explicit flow should be implemented where you terminate the extension host from here, because once we move to a different IPC mechanism in the future, maybe we cannot rely on the socket-close event anymore.
There was a problem hiding this comment.
Yes, we need to revisit the shutdown of the extension host. Today, the extension host shuts down when the renderer sends it a shutdown message via the socket or when the socket closes. If the new IPC mechanism does not support closing we need to find an alternative way to shutdown the extension host (maybe by posting a message using nodejs to it).
Fixes #123592: Creates a node worker in the main process which creates extension host processes.