[WIP] Fixes issue with multi-threaded application suspend and resume. #108
Conversation
| @@ -39,6 +40,12 @@ | |||
| ptvsd_sys_exit_code = 0 | |||
|
|
|||
|
|
|||
| # Setting the suspend policy for breakpoints here since pydevd does not expose this feature via CMD_SET_BREAK command | |||
There was a problem hiding this comment.
Since we have bitten the bullet and vendored pydevd, can this be done in a less hacky way now that you can also change things on the pydevd side of things?
I'm thinking something along the lines of CMD_SET_BREAK_XML, which works same as the existing command, but takes its arguments as an XML blob for extensibility. And then we can add a new attribute for suspend policy there.
There was a problem hiding this comment.
I will take this as a separate work item. since this will be a pydev protocol change, and it impacts pydevd tests as well.
ptvsd/wrapper.py
Outdated
| @@ -544,7 +555,11 @@ def on_threads(self, request, args): | |||
|
|
|||
| threads = [] | |||
| for xthread in xthreads: | |||
| tid = self.thread_map.to_vscode(xthread['id']) | |||
| try: | |||
| tid = self.thread_map.to_vscode(xthread['id'], False) | |||
There was a problem hiding this comment.
Can you use autogen=False here, so that it's clearer what False means from glancing at the code.
| if self.suspending_threads_for_bp and \ | ||
| self.vsc_breakpoint_tid and \ | ||
| self.suspended_thread_count == len(self.thread_map.vscode_ids()): | ||
| self.send_event('stopped', reason='breakpoint', threadId=self.vsc_breakpoint_tid, allThreadsStopped=True) |
There was a problem hiding this comment.
I'm confused - shouldn't this be done in the handler for CMD_THREADS_SUSPENDED. where we know that all threads are stopped?
There was a problem hiding this comment.
These events occur on multiple threads. I will reply back with the exact thread details tomorrow.
There was a problem hiding this comment.
Here is a example of how the events appear to ptvsd. the suspending event and suspended event appear on main and worker threads. Also note that the Worker thread pid_19284_id_1512282591696 suspended after we received the suspended event.
Suspending event: Thread-5 pid_19284_id_1512282591696
Created : MainThread pid_19284_id_1512267175360
Created : ptvsd.EventLoop pid_19284_id_1512282287072
Created : ptvsd.Client pid_19284_id_1512282287688
Created : Thread-5 pid_19284_id_1512282591696
Suspended : None pid_19284_id_1512267175360
Suspended event: Thread-5 pid_19284_id_1512282591696
Suspended : None pid_19284_id_1512282591696
The advantage of having suspended and suspending events is that it allows us to identify if the suspend is actually breakpoint suspend or due to user requesting to pause. But I feel this is not reflected in the naming of those events.
There was a problem hiding this comment.
Hm, so then the suspended event can happen before all threads have actually been suspended? Wouldn't that make it essentially useless though?
There was a problem hiding this comment.
Yep. The only useful event is suspending. The event name does not indicate its actual purpose, which is detect suspension of threads for breakpoint.
So, do you suggest any other alternatives? Is there a way we can use threading.Condition with the stack trace lock, to ensure that all threads have suspended and have stack_trace before we send the event to VS?
Checked with @DonJayamanne that this is the expected behavior for VSC, since it matches the old debugger behavior.
@int19h Let me know if there is a better way to manage waiting for all threads to suspend.