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

Mechanism to abort the debug launch without showing launch.json #54213

Closed
rodrigovaras opened this Issue Jul 12, 2018 · 9 comments

Comments

@rodrigovaras

rodrigovaras commented Jul 12, 2018

We have today the with this method:
interface DebugConfigurationProvider {
resolveDebugConfiguration(...)

So far is flexible enough except in scenarios where we want our provider to abort the debug launch without having vscode to think that the configuration is wrong or missing something important. In the 'live share' scenario we are launching remote debug sessions by allowing vscode to display them but not necessary to launch a remote debug session that will eventually arrive later. We want the resolveDebugConfiguartion to allow this by return a blessed debug configuartion or probably best by throwing a DebugConfiguartionCancelledException() that vscode would handle nicely.

@rkeithhill

This comment has been minimized.

Show comment
Hide comment
@rkeithhill

rkeithhill Jul 15, 2018

Just ran into this today. After the PowerShell extension is activated, it takes about 5-8 seconds for the PowerShell Editor Services (lang/debug server) to finish starting up. While that is happening it isn't a good time to start debugging. ATM we have a NullReferenceException that occurs in PSES because it is still initializing. We could certainly fix the NullReferenceEx in PSES but I'm afraid this might be the tip of the iceberg in this case.

So I've put code in resolveDebugConfiguration to check the status of PSES and if it isn't in the Running state, we pop a warning notification and return out of the function. The notification asks the user to retry after PSES has finished starting but unfortunately, VSCode has switched the active editor to launch.json. This makes this approach annoying for the user as now they can't just press F5 again, they have to reselect the PowerShell editor window again and then press F5.

BTW if there's a better way to prevent debugging until the extension is ready, please let me know.

rkeithhill commented Jul 15, 2018

Just ran into this today. After the PowerShell extension is activated, it takes about 5-8 seconds for the PowerShell Editor Services (lang/debug server) to finish starting up. While that is happening it isn't a good time to start debugging. ATM we have a NullReferenceException that occurs in PSES because it is still initializing. We could certainly fix the NullReferenceEx in PSES but I'm afraid this might be the tip of the iceberg in this case.

So I've put code in resolveDebugConfiguration to check the status of PSES and if it isn't in the Running state, we pop a warning notification and return out of the function. The notification asks the user to retry after PSES has finished starting but unfortunately, VSCode has switched the active editor to launch.json. This makes this approach annoying for the user as now they can't just press F5 again, they have to reselect the PowerShell editor window again and then press F5.

BTW if there's a better way to prevent debugging until the extension is ready, please let me know.

rkeithhill added a commit to PowerShell/vscode-powershell that referenced this issue Jul 15, 2018

First approach to fix issue with dbg/run start before PSES running
Fix #1433

One minor issue is that if you abort (return null|undefined) from
resolveDebugConfiguration, VSCode "helpfully" opens your launch.json
file for you.  Actually, that is quite annoying. I found an issue and on
this and voted it up - Microsoft/vscode#54213

Also fix logic for "attach" error.  We only need to test if
OS != Windows.  If on Windows, PS Core supports attach. And tweaked the
error message wording to make more clear.

If the user attempts to start a dgb or "run with out debugging" session
before PSES is running, a NullRefEx occurs in PSES. Ideally, we would
wait in the resolveDebugConfiguration method for PSES to finish
initializing with a max wait time of say 10 seconds.  Unfortunately,
"sleep" in a loop in JavaScript is not so easy.  AFAIT requires a
significant rewrite of the method using setTimeout().  Not sure it is
worth it, unless someone more knowledgable in JS can find an easy
way to do the poll (for sessionstatus)/sleep loop.

BTW there is probably a fix we need to make in PSES to check if
SynchronizationContext is not null before we try to use it.

rkeithhill added a commit to PowerShell/vscode-powershell that referenced this issue Jul 16, 2018

First approach to fix issue with dbg/run start before PSES running (#…
…1436)

Fix #1433

One minor issue is that if you abort (return null|undefined) from
resolveDebugConfiguration, VSCode "helpfully" opens your launch.json
file for you.  Actually, that is quite annoying. I found an issue and on
this and voted it up - Microsoft/vscode#54213

Also fix logic for "attach" error.  We only need to test if
OS != Windows.  If on Windows, PS Core supports attach. And tweaked the
error message wording to make more clear.

If the user attempts to start a dgb or "run with out debugging" session
before PSES is running, a NullRefEx occurs in PSES. Ideally, we would
wait in the resolveDebugConfiguration method for PSES to finish
initializing with a max wait time of say 10 seconds.  Unfortunately,
"sleep" in a loop in JavaScript is not so easy.  AFAIT requires a
significant rewrite of the method using setTimeout().  Not sure it is
worth it, unless someone more knowledgable in JS can find an easy
way to do the poll (for sessionstatus)/sleep loop.

BTW there is probably a fix we need to make in PSES to check if
SynchronizationContext is not null before we try to use it.

@weinand weinand added the debug label Jul 27, 2018

@weinand weinand added this to the On Deck milestone Jul 27, 2018

@weinand weinand modified the milestones: On Deck, September 2018 Sep 13, 2018

@weinand weinand added the api label Sep 13, 2018

@weinand

This comment has been minimized.

Show comment
Hide comment
@weinand

weinand Sep 13, 2018

Member

I've fixed this issue by adding the following line to the comment for resolveDebugConfiguration and implementing the behaviour on the VS Code side:

Returning the value 'null' prevents the debug session from starting and opens the underlying debug configuration instead.

With this it is now possible to control what happens when aborting resolveDebugConfiguration:

resolveDebugConfiguration(folder: WorkspaceFolder | undefined, config: DebugConfiguration, token?: CancellationToken): ProviderResult<DebugConfiguration> {

	return vscode.window.showErrorMessage(`Program missing in debug configuration`, { modal: true }, `Open 'launch.json'`).then(result => {
		if (result && result.indexOf('Open') >= 0) {
			return null;	// opens launch.json
		}
		return undefined;	// aborts debugging silently
	});
}
Member

weinand commented Sep 13, 2018

I've fixed this issue by adding the following line to the comment for resolveDebugConfiguration and implementing the behaviour on the VS Code side:

Returning the value 'null' prevents the debug session from starting and opens the underlying debug configuration instead.

With this it is now possible to control what happens when aborting resolveDebugConfiguration:

resolveDebugConfiguration(folder: WorkspaceFolder | undefined, config: DebugConfiguration, token?: CancellationToken): ProviderResult<DebugConfiguration> {

	return vscode.window.showErrorMessage(`Program missing in debug configuration`, { modal: true }, `Open 'launch.json'`).then(result => {
		if (result && result.indexOf('Open') >= 0) {
			return null;	// opens launch.json
		}
		return undefined;	// aborts debugging silently
	});
}

@weinand weinand closed this in 1a11718 Sep 13, 2018

@testforstephen

This comment has been minimized.

Show comment
Hide comment
@testforstephen

testforstephen Sep 14, 2018

@weinand I like the new behavior, it works well for the workspace with launch.json file, but there may be a potential break change for the workspace without launch.json. It's better to keep the debugger owners for awareness.

In current VS Code 1.27.1, when the user clicks F5 in the workspace without launch.json file, VS Code will invoke resolveDebugConfiguration api first, if it's returned value is {}, or undefined, null, then delegate to provideDebugConfiguration api to generate launch.json file.

In the insider with your new change, the debugger extension must return null explicitly for resolveDebugConfiguration api, then VS Code will continue delegating to provideDebugConfiguration api to generate launch.json file. For all other return values, it will try to launch or abort debug session, no launch.json is generated or opened.

testforstephen commented Sep 14, 2018

@weinand I like the new behavior, it works well for the workspace with launch.json file, but there may be a potential break change for the workspace without launch.json. It's better to keep the debugger owners for awareness.

In current VS Code 1.27.1, when the user clicks F5 in the workspace without launch.json file, VS Code will invoke resolveDebugConfiguration api first, if it's returned value is {}, or undefined, null, then delegate to provideDebugConfiguration api to generate launch.json file.

In the insider with your new change, the debugger extension must return null explicitly for resolveDebugConfiguration api, then VS Code will continue delegating to provideDebugConfiguration api to generate launch.json file. For all other return values, it will try to launch or abort debug session, no launch.json is generated or opened.

@weinand

This comment has been minimized.

Show comment
Hide comment
@weinand

weinand Sep 14, 2018

Member

@testforstephen thanks for raising your concern.

@isidorn let's discuss. We might want to flip the "null" and "undefined".

Member

weinand commented Sep 14, 2018

@testforstephen thanks for raising your concern.

@isidorn let's discuss. We might want to flip the "null" and "undefined".

@weinand

This comment has been minimized.

Show comment
Hide comment
@weinand

weinand Sep 17, 2018

Member

We've concluded that the fix is good and @isidorn will make debug adapter authors aware of the change.

Member

weinand commented Sep 17, 2018

We've concluded that the fix is good and @isidorn will make debug adapter authors aware of the change.

@isidorn

This comment has been minimized.

Show comment
Hide comment
@isidorn

isidorn Sep 18, 2018

Contributor

@DanTup I know you are depending on this mechanism so please be aware of the change we are making in the new vscode release.
This comment covers the change #54213 (comment)
Basically now you should return null to explicitly let vscode know to open launch.json

Contributor

isidorn commented Sep 18, 2018

@DanTup I know you are depending on this mechanism so please be aware of the change we are making in the new vscode release.
This comment covers the change #54213 (comment)
Basically now you should return null to explicitly let vscode know to open launch.json

@DanTup

This comment has been minimized.

Show comment
Hide comment
@DanTup

DanTup Sep 18, 2018

@isidorn Great, thanks! I can remove some crufty code with this (where we return a debug config that has a type = null). I'll make the changes and do some testing and shout if I hit any problems.

DanTup commented Sep 18, 2018

@isidorn Great, thanks! I can remove some crufty code with this (where we return a debug config that has a type = null). I'll make the changes and do some testing and shout if I hit any problems.

@DanTup

This comment has been minimized.

Show comment
Hide comment
@DanTup

DanTup Sep 18, 2018

Tested it and it works great for me, closes #51069 too. Thanks!

DanTup commented Sep 18, 2018

Tested it and it works great for me, closes #51069 too. Thanks!

@isidorn

This comment has been minimized.

Show comment
Hide comment
@isidorn

isidorn Sep 25, 2018

Contributor

@DanTup thanks for testing, adding verified label

Contributor

isidorn commented Sep 25, 2018

@DanTup thanks for testing, adding verified label

rkeithhill added a commit to rkeithhill/vscode-powershell that referenced this issue Sep 27, 2018

Explicitly return undefined from resolveDbgConfig when sessn not started
In the September drop of VSCode this fixes the issue with VSCode
opening launch.json in this case.  Technically just returning nothing
works but better to be explicit in this case I think.

Microsoft/vscode#54213 (comment)

isidorn added a commit to isidorn/omnisharp-vscode that referenced this issue Sep 27, 2018

isidorn added a commit to isidorn/omnisharp-vscode that referenced this issue Sep 27, 2018

rjmholt added a commit to PowerShell/vscode-powershell that referenced this issue Sep 27, 2018

Explicitly return undefined from resolveDbgConfig when sessn not star…
…ted (#1548)

In the September drop of VSCode this fixes the issue with VSCode
opening launch.json in this case.  Technically just returning nothing
works but better to be explicit in this case I think.

Microsoft/vscode#54213 (comment)

pieandcakes added a commit to Microsoft/vscode-cpptools that referenced this issue Sep 27, 2018

Return null config to VS Code
return null to trigger VS Code to open a configuration file
Microsoft/vscode#54213

pieandcakes added a commit to Microsoft/vscode-cpptools that referenced this issue Sep 27, 2018

Return null config to VS Code (#2570)
return null to trigger VS Code to open a configuration file
Microsoft/vscode#54213

gregg-miskelly added a commit to OmniSharp/omnisharp-vscode that referenced this issue Oct 2, 2018

gregg-miskelly added a commit to gregg-miskelly/omnisharp-vscode that referenced this issue Oct 3, 2018

gregg-miskelly added a commit to gregg-miskelly/omnisharp-vscode that referenced this issue Oct 3, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment