Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

When thread is no longer stopped the focus is not automatically passed to another thread #125144

Closed
luabud opened this issue Jun 1, 2021 · 12 comments
Assignees
Labels
author-verification-requested Issues potentially verifiable by issue author debug Debug viewlet, configurations, breakpoints, adapter issues feature-request Request for new features or functionality insiders-released Patch has been released in VS Code Insiders on-release-notes Issue/pull request mentioned in release notes verification-needed Verification of issue is requested verified Verification succeeded
Milestone

Comments

@luabud
Copy link
Member

luabud commented Jun 1, 2021

Hey there!

We have a problem in the Python extension when it comes to the UX of multi-process debugging.
Take the example below:

from concurrent.futures import ProcessPoolExecutor
from time import sleep

values = [2, 3, 4, 5, 6]

def cube(x): 
    res = x ** 3
    print(f"Cube of {x}:{res}")

if __name__ == "__main__":
    result = []
    with ProcessPoolExecutor(max_workers=5) as executor:
        result = executor.map(cube,values)

    for r in result:
        print(r)

If you add a breakpoint to e.g. line 7 (res = x ** 3), start debugging this file in VS Code (just make sure to select "Python File" from the drop down menu once you hit F5) and hit continue, the first subprocess that is in focus will continue, and the focus will remain in it, even though you have multiple other subprocesses paused on that breakpoint.
The request here is if there's a way the focus could automatically switch to another subprocess that is paused.

image

@luabud
Copy link
Member Author

luabud commented Jun 1, 2021

@isidorn isidorn changed the title Improve experience for Python multiprocess debugging When thread is no longer stopped the focus is not automatically passed to another thread Jun 2, 2021
@isidorn isidorn added debug Debug viewlet, configurations, breakpoints, adapter issues under-discussion Issue is under discussion for relevance, priority, approach labels Jun 2, 2021
@isidorn
Copy link
Contributor

isidorn commented Jun 2, 2021

Thanks for opening the issue and I agree we should improve here.
We got the same feedback from @testforstephen here #68955

As I mentioned the problem on the VS Code side is the following:

  • Once a user presses continue on a thread T1, VS Code does not know if it should pass focus to another thread. Problem being that T1 might hit a breakpoint again in 1s, and if we passed focus to T2 then it might be a bad experience for the user that he just lost focus on what he was actually debugging.

I am open for ideas on how to solve this. But we might need to add some hints to the DAP so the debug adapter could control this.

Let me know what you think. Thanks
Also fyi @gregg-miskelly since C# might be hitting this as well

@gregg-miskelly
Copy link
Member

@isidorn The way Visual Studio handles this situation is that if the UI gets multiple stopping events, the subsequent stopping events are queued. Hitting F5 just dequeues the next stopping event, so we will switch focus to that process. I don't see any reason why this approach wouldn't work for VS Code as well. This wouldn't require any DAP changes.

@weinand
Copy link
Contributor

weinand commented Jun 7, 2021

@isidorn Gregg's suggestion sounds like an excellent idea.

@isidorn
Copy link
Contributor

isidorn commented Jun 7, 2021

We can try that out. But the problem is that if Thread T1 we hit continue we will automatically switch focus to an already stopeed thread T2, even if T1 is about to stop in 100ms.

We can treat continue specially here, so that this would not happen for step over and step into.
@gregg-miskelly Visual Studio does this only for when continue is pressed? And thanks a lot for a good suggestion

@isidorn isidorn added this to the June 2021 milestone Jun 7, 2021
@gregg-miskelly
Copy link
Member

gregg-miskelly commented Jun 7, 2021

@isidorn No, full VS has the same behavior for all three execution commands. This can cause annoying bouncing back and forth if the process you aren't trying to step is hitting breakpoints. Fortunately if this happens you can, as a user of the debugger, fix this by disabling your breakpoints and hitting continue. Then when you step completes things will stay on the stepping process.

We have thought about implementing a "priority boost" where if the user hits step will will wait a small amount of time before dequeuing breakpoint events from other threads/processes. But the priority of this scenario has never risen high enough to implement something like this. The tricky part is to find the right delay period. You don't want to wait too long since the stepping thread may be waiting on the other process/thread (the one that is stopped). On the other hand, too short of a delay means we are back to alternating threads.

@isidorn
Copy link
Contributor

isidorn commented Jun 7, 2021

@gregg-miskelly thanks for providing more details. Making the user disable all breakpoint before stepping sounds like a bad experience.

Maybe a simple 500ms delay would help. Since most of the steps would hit a consequent stop in that time.
Anyways I will try to experiment with this this milestone and once we have something you can all try it out and we can fine tune the behaviour. Thanks!

@isidorn
Copy link
Contributor

isidorn commented Jun 28, 2021

I have looked into this and here are my findings:

  1. The approach (queueing stop events) @gregg-miskelly suggested makes good sense and I have implemented it
  2. The case that @luabud outlines is actually not a multi threaded example. Instead it is a parent session with mutiple child debug session and each child has exactly one thread. Since this is somewhat of an implementation detail I think we can use the same logic when switching focus between sessions as we do when switching focus between threads.
  3. If I step over Session A, and I have come to the "end" the focus automatically switches to Session B which can be confusing to the user especially if both sessions are stopped on the same file.

Due to point 3 and to the problem of back and forth between two threads I have decided to only auto focus thread / session after 800ms delay if no stop even has come in the meantime.
I believe this timeout will improve the "back and forth" case and make it a bit clearer to the user that a focus has changed.

@luabud @gregg-miskelly @testforstephen please try it out from tomorrow and let me know what you think. In the testing that I have done this is the approach I liked the best, however I am open for suggestions on how to improve.

@isidorn isidorn added feature-request Request for new features or functionality verification-needed Verification of issue is requested and removed under-discussion Issue is under discussion for relevance, priority, approach labels Jun 28, 2021
@weinand weinand added the author-verification-requested Issues potentially verifiable by issue author label Jun 30, 2021
@connor4312 connor4312 added the verified Verification succeeded label Jun 30, 2021
@connor4312
Copy link
Member

hm, this doesn't seem to work for me using the original example: https://memes.peet.io/img/21-06-bae6a450-922b-4f9c-9c30-8d64fdc6898b.mp4

@connor4312 connor4312 reopened this Jun 30, 2021
@connor4312 connor4312 added verification-found Issue verification failed and removed verified Verification succeeded labels Jun 30, 2021
@isidorn isidorn closed this as completed in 5278e78 Jul 1, 2021
@isidorn
Copy link
Contributor

isidorn commented Jul 1, 2021

@connor4312 good catch, I missed a case here.

Also I decided to not overcomplicate to start: there will be no 800ms delay. This just complicates things, and since VS survived without a timeout for so long we should first start without it and introduce it if we find that it is needed.

@isidorn isidorn added on-release-notes Issue/pull request mentioned in release notes and removed verification-found Issue verification failed labels Jul 1, 2021
@rzhao271 rzhao271 added the verified Verification succeeded label Jul 2, 2021
@luabud
Copy link
Member Author

luabud commented Jul 8, 2021

I know it's been verified but just wanted to leave a late note saying the experience looks great for Python 😊 thanks a lot!!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
author-verification-requested Issues potentially verifiable by issue author debug Debug viewlet, configurations, breakpoints, adapter issues feature-request Request for new features or functionality insiders-released Patch has been released in VS Code Insiders on-release-notes Issue/pull request mentioned in release notes verification-needed Verification of issue is requested verified Verification succeeded
Projects
None yet
Development

No branches or pull requests

7 participants
@weinand @isidorn @connor4312 @rzhao271 @gregg-miskelly @luabud and others