Re-enable attach by pid tests (test_attaching_by_pid)

[fabioz]

No description provided.

[int19h]

If the tests are broken after re-enabling in a non-obvious way, we'll probably want to wait until after the refactoring is done, since that will also change this code path substantially.

[fabioz]

I've done changes so that the attach to process can run code in the target pid, but there are still issues to fix. Namely:

• If the process was initially launched from vscode it doesn't really attach because it sees the debugger started in no debug mode.

• As it doesn't block before a connection, the tracing may not be set for existing threads (and thus the user is not able to actually debug existing threads) because the debugger is not setup when the method that sets the tracing for existing threads is called.

I'm not sure this is worth fixing now given that as @int19h mentioned, this code is expected to change quite a bit.

[fabioz]

I was thinking a bit more and I think that maybe a better approach would be first fixing #509 (so, I'd move the code that does the settrace after the attach to pid to a module which would be used during a regular settrace, not only during the attach to pid) -- I think this should be enough to fix the 2nd issue I mentioned without changes that'd conflict with the ptvsd refactoring -- and being able to set the tracing to existing threads (in CPython) will also be pretty nice in general.

The first issue I mentioned above could be a non-issue after the refactoring (depending on how it's done, so, probably something to keep in mind -- in particular, if the adapter does a launch directly without traces of the debugger it becomes a non-issue, if that's still an issue after the refactoring this will need to be better checked, but as it's very likely that it's the same code being refactored I'd rather wait for the refactoring to take a look at that).

[int19h]

The main reason why we even need to do anything about this today in ptvsd is for two reasons:

First, because VSCode needs to receive subprocess notifications, so that it can attach to them. That logic can be moved to pydevd entirely, we just need to standardize on the message format. It was impossible in the original model, because pydevd didn't talk DAP directly on its socket. But now that it is a DAP server, I think this is easy to solve.

The second reason is that we host pydevd inside ptvsd, and there's a lot of special logic there still. Thus, subprocesses need ptvsd injected into them, not just pydevd, and have it set up accordingly. After refactoring, there's going to be a lot less logic in ptvsd server - it's mostly just an API wrapper on top of pydevd at that point (so that stuff like ptvsd.enable_attach etc still works). But we still need it there. I'm thinking that it could be done much better with an API in pydevd, where ptvsd in the parent process can tell pydevd that for any subprocesses, after pydevd gets them loaded, it should execute a particular function with certain arguments. Or maybe even just a string of code to eval, because that's both simple and powerful. And pydevd could then propagate it all like it propagates its own settings, and do the eval, which would do import ptvsd etc as needed.

[fabioz]

Related to subprocess notifications, pydevd by default will just try to connect the sub processes to a socket which the DAP should be listening, so, as far as the ptvsd debug adapter is listening, no additional message should be needed (as it'll know that a new subprocess was created when a new socket connection is requested).

Related to pydevd inside ptvsd I agree that we could make pydevd more suitable for embedding, although another solution could be just putting pydevd and ptvsd on the same level in the PYTHONPATH and those would become non-issues as both would be importable at the top-level without any special work (and any pydevd customization which is still needed should be configurable as a parameter in the pydevd command line).

[int19h]

Sorry, I got confused there - thought this was multiprocess debugging, for some reason. With attach-by-PID we shouldn't need any new messages, just a way to inject arbitrary code.