Rkeithhill/is194 ctrl f5 run #210
Conversation
|
It's our profile friend again. When I try to debug this I see the |
| /// Disables all breakpoints in the runspace. | ||
| /// </summary> | ||
| /// <returns></returns> | ||
| public async Task DisableAllBreakpoints() |
There was a problem hiding this comment.
Technically this shouldn't be necessary since a new process is created each time so there would be no breakpoints to disable. Have you seen breakpoints persist across debugging sessions?
There was a problem hiding this comment.
No I haven't seen that. The problem is the ordering of the messages. Right now we get the launchRequest, setBreakpoints and configurationDone. But at one point we were getting the launchRequest at the end of this sequence (which is I the code for those two messages tolerates order). If the launchRequest ever comes after the setBreakpoints then it is too late to "not set" them.
I guess my confidence in the stability of the debug protocol message ordering isn't extremely high. :-) But at the moment, the given order I see on the insiders build suggests that we could just "not set" them.
There was a problem hiding this comment.
Here's a possible strategy. Let's go ahead and check in a solution where we just assume the message ordering comes in correctly and that we know the value of NoDebug before the breakpoint request comes in. If NoDebug is on, we'll just ignore any breakpoint request that comes through.
Since the 1.0 release of VS Code is supposed to land at the end of April, we can keep an eye on it and see if the debug messages continue to be sent in the right order. We'd then release our update on or just after the 1.0 release is done. If it turns out people see issues with running in NoDebug mode, we can release a quick bugfix update that re-introduces the breakpoint disabling code.
I think I'd rather take this strategy than have to make further changes to our code to avoid a possible message ordering problem. What do you think?
There was a problem hiding this comment.
Works for me. I 'll make the changes tonight. BTW we are broken again in the just released 1.0.0 alpha. If you try to start DebugTest.ps1 under the debugger with no breakpoints set, you get no output. :-(
There was a problem hiding this comment.
Argh... Does it look like the script is getting executed on our end? Are they just not showing our output in the UI?
There was a problem hiding this comment.
Nevermind. I'm an idiot. Every time I re-install the alpha I have to remember to update the package.json file for the PS extension to use my debug bits. Forgot to do that. :-(
There was a problem hiding this comment.
Man, I hate having to do that. There needs to be some way to refer to a setting string for the extension in a debug config parameter...
There was a problem hiding this comment.
You and I both. It would be great if that path was a user setting/preference like the editor session host path and waitForDebugger, etc.
There was a problem hiding this comment.
So I am changing to not honoring the setBreakpoint requests when noDebug is true. However we are still on the hook to return a valid response. So right now I'm configuring an empty array of breakpoints to be used in the response. This results in the breakpoints showing as unverified (grayed out) in VSCode until the script finishes. And if I add or delete a breakpoint, the existing ones light up as verified (red) but the script doesn't break on any of them.
If I look at how a node.js program handles Ctrl+F5, the breakpoints remain verified while the script runs without debugging.
Which way to do we want to behave. My original approach would return all the breakpoint info as verified so the breakpoints remained red when running with Ctrl+F5. If I continue down the current path, I'm going to have to modify the code to not return an empty array of breakpoint details but generate an array of breakpointDetails based on the source breakpoint info. It's a bit messy but doable.
There was a problem hiding this comment.
I'd say it's your call if you feel more strongly about a particular path. I'm fine with non-messy and not optimal or the full solution. If you'd rather not spend more time on it yet, we can file a bug for the next release and see how we feel about it then.
|
Seems like all we really have to do in "NoDebug" mode is ignore any breakpoint requests that come in. Did you see a case where breakpoints already existed in the session by the time we launch the script? |
…the launchRequest *before* any setBreakpoint requests come in. We stash the value of noDebug when we get the launchRequest and then when we get the setBreakpoint request and noDebug is true, we skip the bit that actually sets the breakpoints. However we tell VSCode that the breakpoints were set so they show up like normal in the editor.
|
@daviwil Check out the latest update and see what you think. |
|
Looks good to me! |
Consume Editor Services host as PowerShell module
Take a good look at the DebugAdapter change. The LaunchScript method is not async. I "think" I'm waiting on the awaitable DisableAllBreakpoints() method correctly by using Task.WhenAll(). If not, let me know.
This change is