Terminal confirmOnExit should only warn when the terminal shell is running something

[Tyriar]

We could check if the shell process from node-pty has any child processes using something like pgrep -P <pid>. If we do this confirmOnExit=true may be better off as the default.

[Njanderson]

Are you working on this or could a new contributor take a stab at it? I use the integrated terminal almost always, and I think this is something I could take a crack at.

[Tyriar]

@Njanderson wasn't planning on working on it for a while, it would be fantastic if you could give it a go 😃. The plan was basically to investigate the best/most reliable methods to count the number of child processes of a PID on each OS and implement each of them.

[Njanderson]

For Windows, I was thinking of doing something like:

wmic process where ParentProcessId=11712 get Caption,ProcessId
Where 11712 was the PID of the powershell process I was looking at. Looks like you already have the PID information, so we don't need to track it down. If you are using the Ubuntu subsystem shell, then it should be the same for Linux below.

I also see that this test shows that we have the process id of the terminal, so I have a bit more confidence. 👍

	test('terminal, processId immediately after createTerminal should fetch the pid', (done) => {
		window.createTerminal().processId.then(id => {
			assert.ok(id > 0);
			done();
		});
	});

With the process id, I could use powershell or windows to call wmic. Then, for me, winpty-agent.exe is the parent of that pid for cmd or powershell.

For Ubuntu and Mac, what you said seems to work for me.

nja4@ubuntu:~$ pgrep -P 4920
4977

These seem like potential solutions, but I am not sure how to proceed with executing these commands, even if I know where to add them.

As for where these commands are executed, in terminalService.ts:

	private _onWillShutdown(): boolean {
		if (!this.configHelper.config.confirmOnExit) {
			// Don't veto if configured to skip confirmation
			return false;
		}
		if (this.terminalInstances.length === 0) {
			// No terminal instances, don't veto
			return false;
		}

                // START OF NEW CODE
                if (!hasTerminalChildProcesses()) {
			// No terminal instance subprocesses, don't veto
			return false;
                }
                // END OF NEW CODE

		// Veto based on response to message
		return this._showTerminalCloseConfirmation();
	}

As for how, do we try to use another terminal session? Where would you like the platform specific code to be run?

[Tyriar]

Thanks for looking into this @Njanderson, sorry about not getting to this I took last week off and am still catching up. Your proposal looks pretty good, just keep hasTerminalChildProcesses in TerminalInstance, fork a new child process and wait for your new child process calls to finish. You'll also need to change _onWillShutdown's return type to boolean | TPromise<boolean> so that it will wait for our new async hasTerminalChildProcesses.

Note that on Windows processId won't be correct until #14286 is resolved where I need to release and update an npm dep.

We also need to confirm that wmic is supported and has the same format in Windows 7+, not just 10.

[Tyriar]

We're moving ahead with the wmic stuff as part of #30152, this will be something we can look at afterwards as we'll have a mostly complete picture of the process tree. /cc @Lixire

[Tyriar]

This fix sees the method of checking sub-processes in debug world for all platforms #37589 (comment) 8f36137

The terminal should have something similar to this.

[Tyriar]

We may want to add a "running" icon as part of capturing this state? #59672

[ericbf]

Any update here?

[Tyriar]

No update, open to PRs though. Basic outline of what needs to happen is to put logic to query the process probably inside terminalProcess.ts, and then call into it like TerminalInstance -> TerminalProcessManager -> TerminalProcess.

[NotWearingPants]

When this change is made I would like to also have a setting that reverts to the current behavior, i.e. warn on exit even if there are no subprocesses.

[Tyriar]

@NotWearingPants noted, we'd probably want to change confirmOnExit to a string enum then: 'always' | 'never' | 'subProcesses'

[lambainsaan]

This is an issue that I have struggled with as well, I can take it up. 🤘

[Tyriar]

@lambainsaan sounds good, some more info:

• We don't want to use wmic on Windows as it's slow, windows-process-tree was built to use the native windows apis instead.
• There are utils in the codebase I think in the debugging and the issue reporter check process trees, searching windows-process-tree or ps will probably find it
• The strategy for polling the tree is interesting thing here
• WindowsShellHelper already queries the process tree on Windows only, it's triggered on input though

[ericbf]

🎉

[stuartleeks]

Tested on Windows (running powershell -Command "sleep 120")
Tested on WSL and in dev container (running bash -c "sleep 2m")