Make 'autogen' in 'IDMap.to_vscode' a required argument #135
Conversation
There was a problem hiding this comment.
I'm in favor of making autogen a required argument. Otherwise it's easy to miss what's going on. I'm also in favor of the various cleanup you've done. Thanks! All the places that return instead of failing is great too. I just have one concern about ignoring threads in the threads request handler.
ptvsd/wrapper.py
Outdated
| try: | ||
| tid = self.thread_map.to_vscode(xthread['id'], autogen=False) | ||
| except KeyError: | ||
| continue |
There was a problem hiding this comment.
This doesn't seem right. Don't we want to report all threads that PyDevd gives us (minus "internal" ones), even if we never got a CMD_THREAD_CREATE event previously? I'd expect newly discovered threads here to be the same as getting corresponding CMD_THREAD_CREATE events, which is what the original code does.
It seems like this change is saying that we are guaranteed to have gotten all the CMD_THREAD_CREATE events before a threads request is ever made. If that is the case (of which I'm not convinced) then it would be great to have a comment here explaining.
There was a problem hiding this comment.
I see. In that case should we send a create event to VS/VSC here if we encounter new threads?
@int19h any recommendations on what we should do here?
There was a problem hiding this comment.
There's also remote attach w/existing threads, and I'm not sure we'd get those events there either. I think that reporting anything we haven't reported before as freshly created would do the trick.
There was a problem hiding this comment.
@int19h, @ericsnowcurrently , I added a created event in case we find a previously unseen thread.
| try: | ||
| xframes = self.stack_traces[pyd_tid] | ||
| except KeyError: | ||
| # This means the stack was requested before the |
There was a problem hiding this comment.
This comment is helpful. Thanks!
| startFrame = int(args.get('startFrame', 0)) | ||
| levels = int(args.get('levels', 0)) | ||
|
|
||
| tid = self.thread_map.to_pydevd(tid) | ||
| pyd_tid = self.thread_map.to_pydevd(vsc_tid) |
There was a problem hiding this comment.
+1 on distinguishing the names :)
ptvsd/wrapper.py
Outdated
| try: | ||
| name = unquote(xml.thread['name']) | ||
| except KeyError: | ||
| name = None | ||
| if not self.is_debugger_internal_thread(name): | ||
| tid = self.thread_map.to_vscode(xml.thread['id'], autogen=True) |
There was a problem hiding this comment.
great!
It may be worth adding a brief comment indicating that this means "internal" threads will be ignored everywhere else that PyDevd reports to us a thread ID.
|
|
||
| # Stack trace, and all frames and variables for this thread | ||
| # are now invalid; clear their IDs. | ||
| with self.stack_traces_lock: | ||
| del self.stack_traces[pyd_tid] | ||
| try: | ||
| del self.stack_traces[pyd_tid] |
Fixes #137
Fixes #138