Subprocesses are not killed when stopping the debugger

[DonJayamanne]

• Debug following code in VS Code:

import subprocess

p = subprocess.Popen(['python', '-c', 'import time; time.sleep(10000)'])
print(p.pid)
p = subprocess.Popen(['python', '-c', 'import time; time.sleep(10000)'])
print(p.pid)
p = subprocess.Popen(['python', '-c', 'import time; time.sleep(10000)'])
print(p.pid)
p.wait()

• Stop the debugger
• The above (child) processes are not killed.

In VS Code when using the older version of PTVSD (3.0.0), the Python Extension kills all child processes (https://github.com/Microsoft/vscode-python/blob/master/src/client/debugger/PythonProcess.ts#L69).
Right now, this responsibility falls on PTVSD 4.x as it kills the process.

[DonJayamanne]

@brettcannon Please could you assign a priority for this issue.

[brettcannon]

Baseline fixes are more important (not sure how you want that flagged with the labels).

[DonJayamanne]

Why is this in an icebox? I don't think it belongs in there.

[karthiknadig]

We have to discus how this should be handled now given that VSC will be able to connect to individual sub-processes.
@int19h Do you think the ownership to kill child processes should be on parent process? what if the subprocess is expected to run even after parent process is killed? is this behavior different between no-debug and debug modes?

[int19h]

I don't think we can do anything here other than let the user code decide. Anything else will break some user scenario or the other - as you say, sometimes those child processes really are meant to be independent.

[DonJayamanne]

Sounds like we need a spec @qubitron /cc

[qubitron]

The important thing is that for flask and django apps using auto-reload, that users can start and stop debugging without leaving orphaned processes alive occupying ports. Most are not aware that multiple processes are being used to do the auto-reload, so this would cause all sorts of frustration in the auto-reload scenario.

Chatting with Don, the previous version of ptvsd debugger in VSC automatically killed child processes, and this was implemented because of multiple user complaints. We haven't received any complaints about killing child processes, so this indicates killing processes is the expected behavior.

I'd say the default would be to kill child processes, it's generally safer to clean up things than to have unexpected processes running. If we receive user complaints we can add a setting to disable this behavior.

[karthiknadig]

I am adding this to sub-process debugging epic.
@int19h, @DonJayamanne One way i think we can do this is, if this was a sub-process launch, we always kill the process on disconnect. we can detect this with the presence of the parent process port and public key etc.. Those should only be used in sub-process scenario.

[karthiknadig]

@int19h Any recommendation on this, now that we have better implementation details :)

[DonJayamanne]

@int19h @karthiknadig
I've added code in VSC to kill child processes spawned and attached to, during multi-process debugging (only when the main process was launched via the debugger).

I could utilize the same code to fulfill the requirements of this GitHub issue very easily in VSC now, provided PTSD sends custom event that contains the process id along with the launch request (as you do today in ptvsd_process for child processes).

Today a process event is sent back by PTSD to the VSC, I can intercept it in the debug adapter, but can't pass this back to the extension.
To pass it into the extension i need to re-send this information as a custom event on the same stream, but that's very dangerous, as PTVSD is also sending information on the same stream - ie. there every possibility I could write data into the stream midway and bring the whole thing down.
I could solve this by not piping the information, but that makes things a lot more complicated - unnecessarily.

Long story short if you send a custom even the same time the process event is sent, I can handle everything from my end.

[int19h]

I'm confused - don't we already send all the PIDs (child, parent and root) for all the ptvsd_subprocess events? It seems like VSC should have the list of all child process PIDs from that, and could just go over it and kill each?

[DonJayamanne]

I'm confused - don't we already send all the PIDs (child, parent and root) for all the ptvsd_subprocess events?

Isn't this event sent ONLY when multi-process debugging is turned on.
If yes, then awesome.
If no, then i'll need this info in the same event or different event.

It seems like VSC should have the list of all child process PIDs from that, and could just go over it and kill each?

yes we do

[int19h]

It is. But if multiprocessing mode is not enabled, then the debugger doesn't know about child processes at all, by design - so why would we want to autokill them?

[DonJayamanne]

then the debugger doesn't know about child processes at all

Yes. However if I could get just the parent process id (the main process), it would solve the problem for me. I could just tell the OS to kill all child processes. That's what I used to do with the old version of the debugger in VSC.

so why would we want to autokill them?

To meet the requirements of this Issue.

[DonJayamanne]

@karthiknadig @int19h
Sorry guys, hit a few issues trying to implement this in VSC, basically no can do. Needs to be done in DA (in PTVSD).

[karthiknadig]

I would really like to remove the ptvsd_process event if it is not going to be used.

[karthiknadig]

We had moved this out to next milestone due to unplanned work; bringing this back