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

pydevd does not support "terminate" nor "terminateDebuggee" #1755

Closed
int19h opened this issue Sep 9, 2019 · 3 comments

Comments

@int19h
Copy link
Contributor

commented Sep 9, 2019

Curiously, the "initialize" response claims that it does, although that particular item in it is also misspelled:

            'supportTerminateDebuggee': True,

Either way, in practice it doesn't do anything. Thus, there's currently no way to terminate the process when the launcher is not in the picture - e.g. for all attach scenarios.

This is going to be less important once we do #1712, since then the adapter can kill the process by PID in all "attach" scenarios. However, that has a potential race condition, in that the debuggee may exit, and a new process with the same PID can be spawned, by the time the adapter tries to kill it. It would be better if pydevd could self-terminate when asked, and the adapter would then only try kill-by-PID as a fallback if it notices that debuggee is still running after "terminate".

@fabioz

This comment has been minimized.

Copy link
Collaborator

commented Sep 10, 2019

@karthiknadig @int19h I started to do this, but one thing that I just realized is that it should not only kill the process itself, it also needs to kill child processes to avoid leaving any zombie processes there.

So, usually I'd rely on psutil for this, but I don't think we want to add that dependency, so, I was thinking about calling externally taskkill /PID /T on Windows and kill -STOP/pgrep -P/kill -KILL on Linux/Mac (similar to https://github.com/fabioz/winp/blob/master/src/main/java/org/jvnet/unixp/UnixProcess.java).

What do you think? Do you see a better way to do this?

@int19h

This comment has been minimized.

Copy link
Contributor Author

commented Sep 10, 2019

Killing child processes isn't always desirable - especially on Win32, where a parent/child relationship doesn't imply ownership. One process might spawn another process that is semantically unrelated and is not controlled. On the other hand, there are clearly many scenarios where child processes are, essentially, components of the parent process.

We discussed this several times, but couldn't come up with a definition that wouldn't break down outside of a specific use case. The one where processes are interdependent is by far the most common, so it's a reasonable default, but it can't be the only way.

So the takeaway was that we should let the users manage this. When multiproc is done via the adapter, this naturally follows: each pydevd instance tracks its own process, the adapter tracks all processes as a coherent whole and presents it as such to the IDE, and the IDE presents a UI in which the user can detach or kill processes on a case-by-case basis. When debug session is stopped, the default is still to kill the whole tree - and the adapter can do it because it knows about all of them, so there's no need for psutil - but explicitly detached child processes effectively opt out of it.

So from pydevd perspective, it just needs to do os._exit() in this case, and let the adapter do the rest as needed.

@fabioz

This comment has been minimized.

Copy link
Collaborator

commented Sep 10, 2019

Humm, I'm not sure about that arrangement... if we're all in Python subprocesses this works, but if the user spawns a non-Python process it won't stop that process as we won't attach to it.

-- as a note, in Eclipse/PyDev all processes are always killed and I never had any complain about that -- I only ever had complains that a subprocess was not killed and had 0 complains after always killing everything... if there's some case where a user does really want that, I think it should be an opt-out and not opt-in (and the automatic attach to subprocesses shouldn't be the same option).

As a note, I already have the code working (without psutil) to handle that case, so, it's mostly a matter of choosing the option in which it's used or not (and whether it should be the default).

fabioz added a commit to fabioz/ptvsd that referenced this issue Sep 11, 2019
fabioz added a commit to fabioz/ptvsd that referenced this issue Sep 11, 2019
fabioz added a commit to fabioz/ptvsd that referenced this issue Sep 12, 2019
fabioz added a commit to fabioz/ptvsd that referenced this issue Sep 12, 2019
fabioz added a commit that referenced this issue Sep 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.