Electron: investigate to use requestSingleInstanceLock() API

[bpasero]

Currently VSCode is implementing it's own solution for preventing a second instance from starting. Electron provides API to do the same but there are differences: https://github.com/electron/electron/blob/7-1-x/docs/api/app.md#apprequestsingleinstancelock

Things we need:

• pass the process environment over (second-instance event should pass ENV of requester electron/electron#18434)
• communicate between second and first instance to support code --status [1]

[1] code --status will print information about the active window in the first instance to the console of the second instance and thus requires access to the workspace, including remote scenarios.

[rzhao271]

Electron's requestSingleInstanceLock() API is a wrapper on top of Chromium. In other words, the implementation heavily depends on what Chromium is doing.

The general shape of the callback is determined at process_singleton.h: https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/process_singleton.h;l=102

The Chromium implementation is process-dependent. The following links are for the Windows implementation:

The part that transfers the commandLine and workingDirectory information from one instance to another is at AttemptToNotifyRunningChrome: https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/win/chrome_process_finder.cc;l=37;drc=477e08a0f1fc11af8e1147dda5edb86b74f04e64

The callback that takes in those same commandLine and workingDirectory args is run during ProcessLaunchNotification: https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/process_singleton_win.cc;l=172

Bonus: I was getting confused by what all the bind functions were doing. It's explained well at https://source.chromium.org/chromium/chromium/src/+/main:base/bind.h

[rzhao271]

Update: Electron has a copy of those files here: https://github.com/electron/electron/tree/main/chromium_src/chrome/browser

Removing chromium label.

[deepak1556]

Adding relevant upstream issues,

pass the process environment over

electron/electron#18434
electron/electron#23220

communicate between second and first instance to support code --status

electron/electron#20942

[rzhao271]

I'm wondering how one of the callbacks should be handled if we're implementing first->second instance data passing on the Chromium side.
Consider the following code:

app.on('second-instance', (event, commandLine, workingDirectory, additionalData, ackCallback) => {
    console.log('Received data from first instance');
    console.log(JSON.stringify(additionalData));
    console.log('Sending data back to the second instance before timeout expires');
    ackCallback(returnObj);
});

I added a new parameter ackCallback instead of using event.setResponse (context electron/electron#20942 (comment)), since I don't think event objects usually have that method. But, does this mean the Chromium side must wait for the callback to be called? What if it is never called?
The current code on the Chromium side also expects to read back the ack from the first instance in the same method that it wrote the message to give to the first instance. If the callback means that we must move around those functions, I worry that the patch will be much harder to maintain.

[deepak1556]

The response callback implementation depends on how the underlying platform communication is handled, first lets consider the Posix implementation.

1. When a second process is launched it will send the commandLine, workingDirectory and additionalData via the socket established in the user data directory to the first process. It will then wait for an ACK message from the first process for a certain timeout (currently this is set to 20s). If the first process does not send the ACK message on time then the second process will consider the first process to be hung and kill it. It will then start itself as the new first process.

For a scenario to send data from the first process to the second, currently the ACK message is sent once the NotificationCallback is run and this is the callback responsible for emitting the JS event second-instance. For the proposed api change with a response callback, this ACK message has to be delayed until the callback is run but not unconditionally (the timeout on the socket read should still be respected to take into consideration that the current process can be frozen). So the design would be that ackCallback will run a native callback on ProcessSingleton class that would send the ACK message along with serialized data to the second process before the socket timeout happens.

Lets consider the windows implementation

2. In this case, the first process will create a hidden message window for a given user data directory and it is responsible for receiving WindowProc messages from new instance launch process. Once the required message is received which is sent by the new process via SendMessageTimeout, the NotificationCallback similar to the posix callback will be run in the first process. If the message was notified successfully then the visible window related to the first process will be brought to foreground and if the first process could not handle the WindowProc message before the timeout (which is 20s, same as posix) then it will be killed. The main difference here is the ACK info is actually handled by the OS instead of the application, the control will return to the window calling SendMessageTimeout once it gets processed by the target window and we cannot delay it after processing. What we can do here is instead is switch to use SendMessage and return control via ReplyMessage once the ackCallback is run from JS. To monitor for the process hung state, we should still send a dummy message via SendMessageTimeout and kill the target window process if this completes before SendMessage yields.

[bpasero]

Refs: electron/electron#31460

[rzhao271]

Fixed a bug upstream for the posix impl so I'm hoping to get that merged first before 31460.
electron/electron#31661
Edit: It's been merged. Now waiting for 31460.

[rzhao271]

Currently fixing a bug where ConnectNamedPipe sometimes hangs, because the other process doesn't actually go through a code path that connects to that waiting pipe.

electron/electron#33736

[deepak1556]

Adding @bpasero comments about the benefits from using the API:

• Might help mitigate multiple dock icon on macOS
• Performance gain (needs to be measured) if runtime handles the communication channel and exit of the new instance
• Handle connection between elevated and non-elevated instances.

[bpasero]

I looked again into how we use our launch service for when a second instance launches and feel we can probably explore adopting requestSingleInstanceLock even without being able to send data back from the second instance. I found only 2 usages:

• vscode/src/vs/code/electron-main/main.ts

  Lines 358 to 361 in 0edaded

  	// Windows: allow to set foreground
  	if (isWindows) {
  	await this.windowsAllowSetForegroundWindow(otherInstanceLaunchMainService, logService);
  	}

• vscode/src/vs/code/electron-main/main.ts

  Lines 346 to 356 in 0edaded

  	if (environmentMainService.args.status) {
  	return instantiationService.invokeFunction(async () => {
  	const diagnosticsService = new DiagnosticsService(NullTelemetryService, productService);
  	const mainProcessInfo = await otherInstanceLaunchMainService.getMainProcessInfo();
  	const remoteDiagnostics = await otherInstanceDiagnosticsMainService.getRemoteDiagnostics({ includeProcesses: true, includeWorkspaceMetadata: true });
  	const diagnostics = await diagnosticsService.getDiagnostics(mainProcessInfo, remoteDiagnostics);
  	console.log(diagnostics);
  	throw new ExpectedError();
  	});
  	}

The first one should not be needed anymore once we switch over, it is a workaround for allowing a second instance to bring a first instance into focus. I am sure, Chrome supports that in their implementation (and I think @rzhao271 confirmed this).

The second one can easily be worked around I would argue. We could simply send a temporary file path from second to first instance asking to write to the file for status info and then read from it. We do a similar trick when we adopted the open command on macOS here.

As such, I think we could do an exploration for adopting requestSingleInstanceLock with what we have today in Electron APIs. We mainly need to validate that we can pass all the things over we need to:

vscode/src/vs/platform/launch/electron-main/launchMainService.ts

Line 36 in 0edaded

start(args: NativeParsedArgs, userEnv: IProcessEnvironment): Promise<void>;

Thoughts?

Update:
It looks like we also expose the IDiagnosticsMainService from the other instance:

vscode/src/vs/platform/diagnostics/electron-main/diagnosticsMainService.ts

Line 27 in 0edaded

getRemoteDiagnostics(options: IRemoteDiagnosticOptions): Promise<(IRemoteDiagnosticInfo | IRemoteDiagnosticError)[]>;

But I feel this is comparable to the task of writing diagnostics into a file and letting the second instance simply read from it.

[bpasero]

Super hacky solution that actually already seems to work 🚀

const result = app.requestSingleInstanceLock({
	args: JSON.stringify(environmentMainService.args),
	env: JSON.stringify(process.env)
});

if (!result) {
	instantiationService.invokeFunction(this.quit);
	return;
}

added right as first line into:

vscode/src/vs/code/electron-main/main.ts

Line 116 in 0edaded

await instantiationService.invokeFunction(async accessor => {

And then

app.on('second-instance', (event: Event, argv: string[], workingDirectory, additionalData: any) => {
	this.start(JSON.parse(additionalData.args), JSON.parse(additionalData.env));
});

added into the constructor of LaunchMainService:

vscode/src/vs/platform/launch/electron-main/launchMainService.ts

Line 52 in 0edaded

) { }

[bpasero]

Interesting, looks like I found a blocker: with requestSingleInstanceLock when you run one instance as sudo and the other as normal user, the second launch will be able to request the lock for itself 🤯

In other words, it looks like the lock is partitioned also by user and not just process, but in our solution we are able to detect this case and show a warning dialog that another instance is running as admin.

Since we do not enforce a separate user data dir and extensions dir (which maybe Chrome does?), we cannot really support this scenario.

Update: on Windows it is even worse, the second instance tries to talk to the first instance but fails silently.

[deepak1556]

Good catch, maybe the API can provide an option to allow this default behavior or abort and surface as error to users.

app.requestSingleInstanceLock{{allowAdminLaunch: <boolean>})

[deepak1556]

Are we planning to pursue this in upcoming milestones, if not I suggest we close this one out.

[vscodenpa]

We closed this issue because we don't plan to address it in the foreseeable future. If you disagree and feel that this issue is crucial: we are happy to listen and to reconsider.

If you wonder what we are up to, please see our roadmap and issue reporting guidelines.

Thanks for your understanding, and happy coding!