Revisit introduction of mainLockfile

[bpasero]

It looks like recently a mainLockfile was added to signal whether a user data dir is in use or not (126218b). I see numerous issues with that:

• writing a file on startup slows down startup, especially if the process has to wait for that to happen. We typically only tolerate this when there is very strong need for it and try to avoid this at all costs (cc @jrieken). It seems here we only add the log file for the debug case for debugging VSCode, but every user now will pay the price of slower startup even when not debugging VSCode.
• dealing with files as a means to lock a folder is complicated. you cannot assume that lifecycleMainService.onWillShutdown is ever called, the application might crash leaving a stale lock file around (cc @alexdima who just implemented a locking solution that avoids these kind of issues for the extension host state process via b1b44a3)

I don't really understand why the existing IPC socket cannot be used to figure out if an instance is running? The IPC handle is statically computed based on some properties that can all be determined by reading package.json and or product.json:

vscode/src/vs/platform/environment/electron-main/environmentMainService.ts

Line 54 in 126218b

get mainIPCHandle(): string { return createStaticIPCHandle(this.userDataPath, 'main', this.productService.version); }

I hope we can find a different solution for this.

[bpasero]

Actually, given we already put a vscode.lock file into the workspace storage folder whenever an instance is running, I wonder if the js-debug extensions couldn't simply scan the <user data>/User/workspaceStorage folder for a folder that contains such a file. That should be a sufficient enough indicator to figure out if an instance is running.

Still, to be entirely sure the instance was not crashing leaving this file around, you would probably have to check the file contents and do similar logic as Alex had to implement, otherwise it would only be a guess.

[connor4312]

writing a file on startup slows down startup, especially if the process has to wait for that to happen. We typically only tolerate this when there is very strong need for it and try to avoid this at all costs

I think it is safe to make this async. It just increases the risk of race if the file is written and the process is shut down before the lifecycle handler is registered, but this risk is present regardless.

dealing with files as a means to lock a folder is complicated. you cannot assume that lifecycleMainService.onWillShutdown is ever called, the application might crash leaving a stale lock file around

Yea, this is an inherent nature of lockfiles. Even the posix native flock(2) doesn't remove the file on process shutdown. That's why the PID is written in the lockfile, and why there's a "debug anyway" option on the dialog that js-debug presents. (In js-debug I haven't implemented logic to double check if the process ID is still running, but I could do so pretty easily if the need arises.)

I don't really understand why the existing IPC socket cannot be used to figure out if an instance is running? The IPC handle is statically computed based on some properties that can all be determined by reading package.json and or product.json

This involves writing VS Code-specific logic into the generic js-debug that duplicates the name-generation process that VS Code does. In the current state we still look for a code.lock file, but this is comparatively minimal, and we do so in parallel to looking for the Chrome lockfiles, so it does not add significant performance or complexity overhead. (Aside, on Windows stating a pipe exists will cause a connection to be made to that pipe, not sure if that would cause issues. This was a problem I hit in js-debug at one point, which uses pipes to communicate from processes back to the debugger.)

For the same reason I would rather avoid doing the workspace storage scanning in js-debug, but at least that is slightly less dependent on details like how the pipe name is formed, so I would be a little happier with that solution.

However, ultimately we just want to know if there is a main process running from this user data dir, so I think having a lockfile is the cleaner option all around.

[alexdima]

given we already put a vscode.lock file into the workspace storage folder whenever an instance is running, I wonder if the js-debug extensions couldn't simply scan the /User/workspaceStorage folder for a folder that contains such a file.

Please don't take a dependency on the vscode.lock file existing. I have received some negative feedback on the whole approach e.g. #128234 , #128237 , so we might want to revisit the whole thing.

[JacksonKearl]

I've been getting lots of errors saying something like "Cannot debug because an existing debug session was found" in past couple weeks, I assume because of this. I also find it unexpected that deleting ~/Library/Application\ Support/code-oss-dev and ~/.vscode-oss-dev no longer gives you a fresh OSS instance.

[connor4312]

Cannot debug because an existing debug session was found

Can you grab a gif of this?

no longer gives you a fresh OSS instance

The new profile directory used for debugging is in the .profile-oss in the vscode folder

[JacksonKearl]

I get this pretty frequently recently,

[connor4312]

That's somewhat expected if OSS is torn down or killed ungracefully. While, while we're developing on it, tends to happen. The main process PID is written to the lockfile, but I don't do a liveness check on that yet. It's on my todo, but you can safely debug anyway if you think that happens.

[connor4312]

I've made a change so that the lock file is acquired asynchronously which should address the startup time concern.

[bpasero]

I am still not convinced we need to invent our own lock file though. For example, doing a fs.existsSync("/Users/bpasero/Library/Application Support/code-oss-dev/1.59.0-main.sock") returns true for me which would be a safe way of figuring out if VSCode is running on POSIX. To make it version independent you could also just check for .sock files. We would need to check if it works on Windows too where we resort to "named pipes".

Other than that, I am sure there is a file Chrome creates for locking (possibly for their various Index DBs on disk) which maybe we could probe on?

[connor4312]

We would need to check if it works on Windows too where we resort to "named pipes".

That's where the difference lies. Named pipes on Windows are created in a global namespace so we create the name by hashing information about the VS Code version. As far as I can tell on windows there isn't a "readdir" that works on \\pipe and I don't want to duplicate that hashing logic in js-debug.

[bpasero]

Nothing changed here right? Besides, once we are adopting requestSingleInstanceLock I think we can piggy back on the file that Chrome creates to signal the lock.

[connor4312]

Ok, I'll keep this open to track that, then.

[bpasero]

Depends on #97626