Skip to content
This repository has been archived by the owner on Aug 2, 2023. It is now read-only.

On stackTrace should provide stack trace for any live thread #72

Closed
karthiknadig opened this issue Feb 13, 2018 · 18 comments
Closed

On stackTrace should provide stack trace for any live thread #72

karthiknadig opened this issue Feb 13, 2018 · 18 comments

Comments

@karthiknadig
Copy link
Member

microsoft/PTVS#3736

@karthiknadig
Copy link
Member Author

karthiknadig commented Feb 13, 2018

on_stackTrace currently only provides stackTrace for threads that have been suspended. For threads that are not suspended, such as the main thread in the bug above, there is no saved xframes available. We need to set the suspend_policy while setting breakpoints.

@karthiknadig karthiknadig added this to the Preview 1 milestone Feb 14, 2018
@karthiknadig karthiknadig self-assigned this Feb 15, 2018
@karthiknadig
Copy link
Member Author

The behavior we want is to only break the thread that had the breakpoint. Other threads should continue to run. This is the expected behavior in VSC.

@zooba
Copy link
Member

zooba commented Feb 16, 2018 via email

@karthiknadig
Copy link
Member Author

Agreed, both are needed. So looks like for VS we will have to set the allThreadsStopped in the response to breakpoint hit, and for VSC we should not set that flag. The IDE's can be distinguished using clientId in the initialize request.

@int19h
Copy link
Contributor

int19h commented Feb 16, 2018

We should ask them to make it a feature flag in the protocol in the future, to support other clients.

@karthiknadig
Copy link
Member Author

Even with that flag event order seems to be a problem. If i have a breakpoint set in a worker function, then when the breakpoint is hit, the breakpoint and paused events for worker and main threads can appear in any order. That makes difficult, if not impossible, to make sure that breakpoint is the last event. breakpoint event has to be the final event to ensure VS UI selects that thread while displaying in threads window and control position in the editor window.

Further there are issues with how we handle individual debuggind steps:

  1. continue currently continues only the thread that is stopped. Should it continue all the threads? I think it should.
  2. Step** : step-in/out/over are currently done per thread. What should be the action applied to other threads?

@zooba
Copy link
Member

zooba commented Feb 17, 2018 via email

@karthiknadig
Copy link
Member Author

karthiknadig commented Feb 17, 2018

@int19h suggested wrapping the code in pydevd where they generate the thread suspended events with start/end events (see pydevd.suspend_all_other_threads usage). This will allow us to solve the problem in a reliable way.

@huguesv huguesv modified the milestones: Preview 1, Preview 2 Mar 3, 2018
@brettcannon brettcannon modified the milestones: Preview 2, VSC Stable Apr 17, 2018
@MikhailArkhipov MikhailArkhipov modified the milestones: VSC Stable, June 2018 May 2, 2018
@karthiknadig karthiknadig removed their assignment May 8, 2018
@MikhailArkhipov MikhailArkhipov self-assigned this May 31, 2018
@karthiknadig karthiknadig modified the milestones: June 2018.1, June 2018.2 Jun 15, 2018
@DonJayamanne
Copy link
Contributor

@karthiknadig @MikhailArkhipov
What's the status of this issue?

@karthiknadig
Copy link
Member Author

This is not done yet. We have following pending items:

  1. A way to tell pydevd to suspend all threads on (breakpoint/exception/etc). There is currently a mechanim but that path does not use breakpoint_id's which we rely on.
  2. Get a notification from pydev that all threads are suspended. Currently there is only a way to tell if individual threads are suspended. we need a way to tell if all threads are suspended.

@fabioz
Copy link
Contributor

fabioz commented Jun 20, 2018

The part related to pydevd is on my todo-list (my current item is path mapping issues across platforms and right after this deal with the thread suspension policy).

@DonJayamanne
Copy link
Contributor

DonJayamanne commented Jun 20, 2018

@karthiknadig @fabioz Would be great if we have separate issues for those and linked them in here. That way we know what the pre-requisites are for this issue to be looked at. I.e. easier to track the status of this.

Please add a checked list of dependencies on the top comment, as follows:

  • Depends on #xxx
  • Depends on #xxx

@MikhailArkhipov MikhailArkhipov modified the milestones: June 2018.2, Future Jun 20, 2018
@fabioz
Copy link
Contributor

fabioz commented Jul 2, 2018

Starting to take a look at this.

@fabioz
Copy link
Contributor

fabioz commented Jul 4, 2018

After working a bit on how to provide a message saying that all threads are suspended I got into corner cases where this cannot be reliably done because to actually pause a thread some tracing event is required, but by pausing a thread it's possible that we get into situations where this never happens and waiting for such a message would not work (i.e.: it would effectively deadlock).

I'm posting an example below where this happens:

import threading
import time

go_on = threading.Event()
lock = threading.Lock()
counter = 0


def method():
    global counter
    with lock:
        counter += 1
    go_on.wait()
        

threads = [threading.Thread(target=method) for _ in range(3)]

for t in threads:
    t.start()

while counter < 3:
    time.sleep(.1)    

print('breakpoint with suspend_all here')  # Will never be able to suspend all threads if they reached go_on.wait() as no tracing event will be generated for the thread after reaching that point.

go_on.set()

So, not sure how to proceed... the feature requested seems to be getting the stack of threads even if they're not paused, so, maybe the best approach would be actually implementing that feature (creating a GET_THREAD_STACK command which can be used to get the stack for any running thread by using sys._current_frames).

@karthiknadig do you think that's reasonable?

@karthiknadig
Copy link
Member Author

@fabioz We will need the suspend_policy in CMD_SET_BREAK (currently it is only available if we don't use breakpoint_id). That along with GET_THREAD_STACK should be enough.

@fabioz
Copy link
Contributor

fabioz commented Jul 4, 2018

@karthiknadig ok, will work on that.

Although I'll only start those next Monday as I ended up chasing a deadlock today -- there's a pull request for it with the fix at: #591 -- I got reports from it when using scapy and attaching the remote debugger, so, it'd be interesting to merge it before another release as I think it's a pretty critical issue.

fabioz added a commit to fabioz/ptvsd that referenced this issue Jul 19, 2018
karthiknadig pushed a commit that referenced this issue Jul 23, 2018
* #72: Provide a way to set breakpoint suspension policy.

* Provide a CMD_GET_THREAD_STACK (#72).
@karthiknadig
Copy link
Member Author

karthiknadig commented Jul 24, 2018

Following items are pending on ptvsd side:

  • Set the flag to suspend all while setting breakpoints
  • Pause should send suspend request to all threads
  • Continue should send run request to all threads
  • Use the new get stack trace pydevd command to retrieve stack for running threads

@DonJayamanne
Copy link
Contributor

Use the new get stack trace pydevd command to retrieve stack for running threads

Can't we use this approach to get stack trace of suspended threads as well. Instead of using the current approach of strong some stack trace into a variable. I.e. request on demand.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

8 participants