Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Expose terminal process ID through API #11919

Closed
Tyriar opened this issue Sep 13, 2016 · 29 comments
Closed

Expose terminal process ID through API #11919

Tyriar opened this issue Sep 13, 2016 · 29 comments
Assignees
Labels
api feature-request Request for new features or functionality terminal Integrated terminal issues

Comments

@Tyriar
Copy link
Member

Tyriar commented Sep 13, 2016

Part of #11422

@Tyriar
Copy link
Member Author

Tyriar commented Sep 16, 2016

Extension authors want to get at the process ID of the shell so they can track what they run, at least on a low level. The problem is that due to a bug in electron (electron/electron#38) there is another process in between the renderer thread and the shell which launches the shell, meaning it's an async operation to grab the PID after the terminal is created.

Therefore, in order to maintain backwards compatibility I think this will need to be exposed as a promise:

// Returns the shell PID
getProcessID(): Thenable<number>;

Alternatively we could expose the in-between process synchronously, it would probably mean more work and confusion for extension authors though:

// Returns the PID of the process that launches the shell
getProcessID(): number;

@jrieken does the Thenable version look reasonable given the situation?

@jrieken
Copy link
Member

jrieken commented Sep 19, 2016

You could just expose the tenable property? Like

interface Terminal {
 // ...
 pid: Thenable<number>
}

@Tyriar
Copy link
Member Author

Tyriar commented Sep 19, 2016

If that's preferable 👍

@jrieken
Copy link
Member

jrieken commented Sep 19, 2016

Yeah - I think that more JavaScript'ish

@Tyriar
Copy link
Member Author

Tyriar commented Sep 19, 2016

FYI @weinand for the debug stuff we discussed #9957

@Tyriar Tyriar closed this as completed in 7236f59 Sep 19, 2016
Tyriar added a commit to microsoft/vscode-extension-samples that referenced this issue Sep 19, 2016
@Tyriar
Copy link
Member Author

Tyriar commented Sep 19, 2016

I ended up expanding to processId for clarity:

interface Terminal {
  processId: Thenable<number>
}

FYI @daviwil

@daviwil
Copy link
Contributor

daviwil commented Sep 19, 2016

Awesome, thanks man!

@wclr
Copy link

wclr commented Sep 22, 2016

@Tyriar So it is not there yet? Updated to latest insiders and terminal._id stopped to work.

@Tyriar
Copy link
Member Author

Tyriar commented Sep 22, 2016

@whitecolor insiders just exposed the above API terminal.processId. For your use case though you should use window.onDidCloseTerminal.

@wclr
Copy link

wclr commented Sep 22, 2016

@Tyriar Ok processId is available now. As I've mentioned I used pid for getting number of child processes not only to determine if is opened/closed, but aslo its state (depending on number of currently running processes).

There seem to be a problemwith processId: it seem to expose some other id that was previously in terminal._id - number of its child processes always is zero (I use psTree to get running children).

And aslo why setProcessId is a public method? What does it supposed to do? https://github.com/Microsoft/vscode/blob/7236f593e7e8363ac940660a105c15e4f51ea198/src/vs/workbench/api/node/extHostTerminalService.ts#L70

@Tyriar
Copy link
Member Author

Tyriar commented Sep 22, 2016

@whitecolor processId is the shell process, not the process that launches the shell:

Before: _id was the pid of terminalProcess which spawned bash/zsh/cmd/etc.
After: processId is the pid of bash/zsh/cmd/etc. that was spawned by terminalProcess

The child processes of processId should be the programs launched by the shell.

setProcessId is public on ExtHostTerminalService but it's not part of the API interface so you should not be touching it, vscode.d.ts defines the API.

@wclr
Copy link

wclr commented Sep 22, 2016

ok, but it seem that internal processes (running inside the console) is spawned not by this processesId, but by teminalProcess.

Btw processId is not available on the terminal just after window.createTerminal executed, have to wait some time, possible to fix?

@jrieken
Copy link
Member

jrieken commented Sep 22, 2016

@Tyriar Maybe we should not make setProcessId a public method. How about a ctor argument which is the (still unresolved) promise of the process id. That will also not require having this ugly wait loop

@Tyriar
Copy link
Member Author

Tyriar commented Sep 22, 2016

@whitecolor processId being a promise cannot change due to a bug in Electron (electron/electron#38) forcing the in between process to begin with.

It doesn't seem right that the processes would be children of terminalProcess.js which only spawns the shell. I looked into this in the past and it worked fine. There's only a single spawn in terminalProcess https://github.com/Microsoft/vscode/blob/master/src/vs/workbench/parts/terminal/electron-browser/terminalProcess.js#L30 The processes launched by the terminal are launched by the shell that they're run in.

@Tyriar
Copy link
Member Author

Tyriar commented Sep 22, 2016

@jrieken 👍 #12434

@wclr
Copy link

wclr commented Sep 22, 2016

@Tyriar ok, thanks, I will look into it more carefully

@wclr
Copy link

wclr commented Sep 22, 2016

@Tyriar I check all process running in windows (wmic.exe PROCESS GET Name,ProcessId,ParentProcessId,Status) after terminal created and I can not find there pid returned by terminal.processId.then(...)

@Tyriar
Copy link
Member Author

Tyriar commented Sep 22, 2016

Can you launch vscode, the terminal and then some program in the terminal and see what the tree looks like? I expect it to be:

  • VS Code
    • ...
    • terminalProcess.js
      • cmd.exe
        • program ran in cmd.exe

@wclr
Copy link

wclr commented Sep 22, 2016

and see what the tree looks like

What tree? (where should I look it)? I just get pid, and check it either using wmic or process exlorer, there is not such pid at all. Should it be there?

@Tyriar
Copy link
Member Author

Tyriar commented Sep 22, 2016

@whitecolor depends on your platform, on Linux I can run pstree. This is the output I get, you can see that gulp is under bash:

        │         │         │         ├─code-insiders─┬─code-insiders───code-insiders─┬─code-insiders─┬─2*[code-insiders───{code-insiders}]
        │         │         │         │               │                               │               ├─code-insiders─┬─{WorkerPool/1113}
        │         │         │         │               │                               │               │               └─{code-insiders}
        │         │         │         │               │                               │               ├─{WorkerPool/1120}
        │         │         │         │               │                               │               └─5*[{code-insiders}]
        │         │         │         │               │                               ├─code-insiders─┬─2*[{WorkerPool/1150}]
        │         │         │         │               │                               │               ├─6*[{WorkerPool/1151}]
        │         │         │         │               │                               │               ├─{WorkerPool/9554}
        │         │         │         │               │                               │               └─5*[{code-insiders}]
        │         │         │         │               │                               ├─code-insiders─┬─{WorkerPool/1079}
        │         │         │         │               │                               │               ├─{WorkerPool/6316}
        │         │         │         │               │                               │               ├─{WorkerPool/7855}
        │         │         │         │               │                               │               └─5*[{code-insiders}]
        │         │         │         │               │                               ├─code-insiders─┬─bash───gulp─┬─4*[{V8 WorkerThread}]
        │         │         │         │               │                               │               │             ├─4*[{gulp}]
        │         │         │         │               │                               │               │             └─{node}
        │         │         │         │               │                               │               ├─2*[{WorkerPool/1149}]
        │         │         │         │               │                               │               └─2*[{code-insiders}]
        │         │         │         │               │                               ├─{Chrome_ChildIOT}
        │         │         │         │               │                               ├─3*[{CompositorTileW}]
        │         │         │         │               │                               ├─{Compositor}
        │         │         │         │               │                               ├─{HTMLParserThrea}
        │         │         │         │               │                               ├─{WorkerPool/1010}
        │         │         │         │               │                               ├─3*[{WorkerPool/1011}]
        │         │         │         │               │                               └─5*[{code-insiders}]

@wclr
Copy link

wclr commented Sep 22, 2016

I run node:
image
There is pid 984 for node and 1436 for cmd

And processId gives 868 (no such pid at all)

@wclr
Copy link

wclr commented Sep 27, 2016

@Tyriar can you confirm my issue with process id?

@Tyriar
Copy link
Member Author

Tyriar commented Sep 27, 2016

@whitecolor it's being tested today by 2 team members that will verify the process ID.

@wclr
Copy link

wclr commented Oct 19, 2016

@Tyriar Was it a bug and fixed?

@Tyriar
Copy link
Member Author

Tyriar commented Oct 19, 2016

@whitecolor no bug has been reported so far, not sure how many people are using it though.

@wclr
Copy link

wclr commented Oct 19, 2016

it's being tested today by 2 team members that will verify the process ID.

So it was tested and you can not confirm it?

@Tyriar
Copy link
Member Author

Tyriar commented Oct 19, 2016

Yes nothing was reported during the test phase and it's currently in stable.

@wclr
Copy link

wclr commented Oct 24, 2016

@Tyriar Well I'm not sure how I should demonstate that something wrong here.

For me terminal.processId promise gives the PID value that doesn't exist in processes list at all. The terminal is running.

image

@Tyriar
Copy link
Member Author

Tyriar commented Oct 24, 2016

Created #14286 to track your issue

@vscodebot vscodebot bot locked and limited conversation to collaborators Nov 18, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api feature-request Request for new features or functionality terminal Integrated terminal issues
Projects
None yet
Development

No branches or pull requests

4 participants