-
Notifications
You must be signed in to change notification settings - Fork 68
Re-enable attach by pid tests (test_attaching_by_pid) #1520
Comments
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. |
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:
I'm not sure this is worth fixing now given that as @int19h mentioned, this code is expected to change quite a bit. |
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). |
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 |
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 |
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. |
* Attach to pid fixes with threading. Fixes #1542,#1520 * Make sure that the python code is actually in bytes. * Fix folder for attach to pid in mac os. * Better error message if unable to get the threadStateIndex for the current thread. * Raise number of attempts to resolve_label during attach to pid (windows).
No description provided.
The text was updated successfully, but these errors were encountered: