fix: Servers terminating even with 'detached: true' #1603

nkomonen-amazon · 2025-01-28T18:52:35Z

Background Info

In Unix-like environments, the child process would sometimes not exit even when the parent did.
The exit/watchdog timers were created due to address this behavior. See the docstring of setupExitTimer().
The 'detached' flag would not be honored in Unix-like environments, and the process to stay running in some cases.

Problem

If a language server was set as detached in the ServerOptions, and we wanted it to not exit on parent exit, it would
still exit.

Solution

Commits in this PR have been split up in to smaller concerns, see their comments for specific changes.

Summary is that when the detached flag is set in the ServerOptions, it will now ensure that the Server actually stays alive when the parent exits.

Testing

Wasn't able to find an easy way to write a test for behavior when the parent (client) terminates and that the child continues to run.
Created an E2E test that the server gets the "detached" flag forwarded to it by the client
I manually tested by doing the following:
- Have the test bed client log the ExtHost PID: console.log('Client PID: ${process.pid}')
- Have the test bed server log its PID, such as a through a notification to the client
- Run the test bed
- Configure the detached flag in the ServerOptions
- Kill the client PID
- Observe that the child PID continues running if detached: true, otherwise it should not exist if detached: false

Manually tested on:

MacOS
Windows
Linux

Behaviors

By default detached: false
- Server will terminate from either an exit notification, or the parent exit/watchdog timers
When detached: true & detachedTimeout is set THEN the server will timeout after detachedTimeout milliseconds have passed since the parent (client) have terminated
When detached: true, a graceful shutdown of the VS Code application results in the Language Server still running
- BUT if the client was pushed to context.subscriptions it will be cleaned up regardless of detached
- QUESTION: Should this behavior be documented, and where?

Resolves #1595

Signed-off-by: nkomonen-amazon [email protected]

nkomonen-amazon · 2025-01-28T18:57:27Z

@microsoft-github-policy-service agree company="Amazon"

dbaeumer · 2025-01-29T09:50:55Z

server/src/node/main.ts

+		// channel (e.g. IPC) disconnects, the child terminates.
+		process.on('disconnect', () => {});
+
+		// Keeps the server alive since node may terminate this process if


I would not hardcode this here. Could we pass that as a argument to the --detach param.

Good point, I've added an optional value for --detached NNNN to set the timeout in milliseconds.

nkomonen-amazon · 2025-02-11T19:28:10Z

~~I was able to get a Windows machine, I'm going to do some testing on it~~
Tested on Windows and things lgtm.

I've added some more information under the Behaviors section of the PR about how this change will impact things

nkomonen-amazon · 2025-02-18T18:41:32Z

Hey @dbaeumer would you be able to give this another look

We have a couple places we parse cli args that have a value, so a general utility will help deduplicate code Signed-off-by: nkomonen-amazon <[email protected]>

Background Info: - In Unix-like environments, the child process would sometimes not exit even when the parent did. - The exit/watchdog timers were created due to address this behavior. See the docstring of setupExitTimer(). - The 'detached' flag would not be honored in Unix-like environments, and looks to stay open regardless in some cases. Problem: If a language server was set as detached in the ServerOptions, and we wanted it to not exit on parent exit, it would still exit due to the setupExitTimer() Solution: - A server will know that it is detached by the cli process argument: '--detached' - The client side code to inject this is in a following commit - The exit/watchdog timers will not run if the detached flag is true - When the input stream to the server ends due to parent terminating, the server would also. This PR changes it to skip this if detached. Signed-off-by: nkomonen-amazon <[email protected]>

- IMPORTANT: We need the setInterval for detached to work, so that the server does not auto-terminate when the parent does. See the code comment which explains why. - By default if the user specifies for the language server to be detached, it will never quit after the parent/client terminates. But if a number (milliseconds) is provided when launching the server it will timeout after that amount of time, after the client terminates. Signed-off-by: nkomonen-amazon <[email protected]>

All different methods that can support detached (i.e the server will not terminate when the parent does) will now do so. - A new field has been added in for the ForkOptions so that forked servers can now be detached. - Only MessageTransport and StreamInfo will remain the same. I have not looked in to how those would support properly detaching. Signed-off-by: nkomonen-amazon <[email protected]>

Signed-off-by: nkomonen-amazon <[email protected]>

- Allows the client to configure a ServerOptions setting that sets the max lifetime of the detached language server after the parent/client has terminated. - Additionally, this exposes some utils from the language server so that they can be reused in tests. Otherwise we'd have to duplicate code and reduce code coverage Signed-off-by: nkomonen-amazon <[email protected]>

dbaeumer

Great work. Only small things.

dbaeumer · 2025-02-20T07:58:51Z

client/src/node/main.ts

@@ -57,10 +57,18 @@ namespace Transport {
 	}
 }

-export interface ExecutableOptions {
+export interface DetachedOptions {


I would do this differently. We should define detached as detached: boolean | number. If it is a boolean then we should use a default timeout. If it is a number we use it and -1 represent indefinite. This makes it more in line with other timeout settings.

dbaeumer · 2025-02-20T08:00:46Z

server/src/node/main.ts

+ * This function assumes the parent process has been properly configured with
+ * necessary settings (e.g., using `child_process.unref()`).
+ */
+function isDetached() {


Small things: declare as return a boolean

dbaeumer · 2025-02-20T08:05:05Z

server/src/node/main.ts

+		// the parent terminates + this process has nothing in the event loop.
+		setInterval(() => {
+			// Check if the detached server has reached the user-specified lifetime
+			if (_protocolConnectionEndTime && timeoutMillis && Date.now() - _protocolConnectionEndTime >= timeoutMillis) {


Although not happening here I usually try to make undefined checks explicit especially if the variable as a or type that can falsify.

_protocolConnectionEndTime !== undefined

dbaeumer · 2025-02-20T08:09:20Z

server/package.json

@@ -24,6 +24,10 @@
 		"./browser": {
 			"types": "./lib/browser/main.d.ts",
 			"browser": "./lib/browser/main.js"
+		},
+		"./utils": {


Why do we need this. It will export this from the npm package and will force use to keep it stable as API.

nkomonen-amazon changed the title ~~Detached fix~~ fix: Servers terminating even with 'detached: true' Jan 28, 2025

dbaeumer requested changes Jan 29, 2025

View reviewed changes

nkomonen-amazon force-pushed the detachedFix branch from 9fd7ee3 to 30aab27 Compare February 5, 2025 22:38

nkomonen-amazon added 6 commits February 19, 2025 10:53

util: Add process utility to parse args

3e5d161

We have a couple places we parse cli args that have a value, so a general utility will help deduplicate code Signed-off-by: nkomonen-amazon <[email protected]>

client: add e2e test that validate detached flag is received

bb822ef

Signed-off-by: nkomonen-amazon <[email protected]>

nkomonen-amazon force-pushed the detachedFix branch from 1c57b6a to 6adb005 Compare February 19, 2025 15:54

dbaeumer requested changes Feb 20, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Servers terminating even with 'detached: true' #1603

fix: Servers terminating even with 'detached: true' #1603

nkomonen-amazon commented Jan 28, 2025 •

edited

Loading

nkomonen-amazon commented Jan 28, 2025

dbaeumer Jan 29, 2025

nkomonen-amazon Feb 5, 2025

nkomonen-amazon commented Feb 11, 2025 •

edited

Loading

nkomonen-amazon commented Feb 18, 2025

dbaeumer left a comment

dbaeumer Feb 20, 2025

dbaeumer Feb 20, 2025

dbaeumer Feb 20, 2025

dbaeumer Feb 20, 2025

fix: Servers terminating even with 'detached: true' #1603

Are you sure you want to change the base?

fix: Servers terminating even with 'detached: true' #1603

Conversation

nkomonen-amazon commented Jan 28, 2025 • edited Loading

Background Info

Problem

Solution

Testing

Behaviors

nkomonen-amazon commented Jan 28, 2025

dbaeumer Jan 29, 2025

Choose a reason for hiding this comment

nkomonen-amazon Feb 5, 2025

Choose a reason for hiding this comment

nkomonen-amazon commented Feb 11, 2025 • edited Loading

nkomonen-amazon commented Feb 18, 2025

dbaeumer left a comment

Choose a reason for hiding this comment

dbaeumer Feb 20, 2025

Choose a reason for hiding this comment

dbaeumer Feb 20, 2025

Choose a reason for hiding this comment

dbaeumer Feb 20, 2025

Choose a reason for hiding this comment

dbaeumer Feb 20, 2025

Choose a reason for hiding this comment

nkomonen-amazon commented Jan 28, 2025 •

edited

Loading

nkomonen-amazon commented Feb 11, 2025 •

edited

Loading