Launch preLaunchTask should complete before resolveDebugConfiguration

[gustavomassa]

When attaching to a program using cppdbg with processId as ${command:pickProcess}, the command pick does not wait for the preLaunchTask initialization/finalization, processId command pick should start just after the preLaunchTask finalization.

Please refer to this issue: microsoft/vscode-cpptools#5287

[weinand]

Good observation: launch config resolution and variable substitution should only take place after the preLaunchTask has finished.

[isidorn]

@gustavomassa I acknolwedge the behavior you have hit. However we were always behaving like that, we can change as you suggest to first run the preLaunchTask and only then resolve. This could potentialy break extensions that rely on filling in the preLaunchTask attribute as part of the resovleDebugConfigurations call. I am not sure if there are extensions like this, but there could be.

I have just checked our docs and our API spec and as far as I saw we do not specify what should be run first.

Since there are still two weeks unitl the end of endgame I am fine with changing the behavior to first run the preLaunchTask. Hopefully that should give us enough time to catch any potential breakages.

[isidorn]

I just tested this and we can not change it, since some preLaunchTask depend on variables to be resolved before it is run.

For example, our starter Typescript extensions has the following launch configuration:
And because preLaunchTask is ${defaultBuildTask} that requires that we first resolve variables and only then run the prelaunch task. Due to this, I am pushing this issue to On-Deck and I am open for suggestions

{
			"name": "Run Extension",
			"type": "extensionHost",
			"request": "launch",
			"runtimeExecutable": "${execPath}",
			"args": [
				"--extensionDevelopmentPath=${workspaceFolder}"
			],
			"outFiles": [
				"${workspaceFolder}/out/**/*.js"
			],
			"preLaunchTask": "${defaultBuildTask}"
		}

[daniv-msft]

Thank you @isidorn for investigating this!

From what you describe, there are two different needs:

1. Ability to use in preLaunchTask variables defined when running resolveDebugConfiguration - Current behavior, needed by Typescript.
2. Ability to use in resolveDebugConfiguration variables defined when running preLaunchTask - My case and @gustavomassa's case.

Given that the order in which these are executed is arbitrary, a potential solution would be to propose another equivalent to preLaunchTask that would be called before resolveDebugConfiguration.

So, we would have an order of execution like:

• preResolveTask (name TBD)
• resolveDebugConfiguration
• preLaunchTask

Is this a possibility that could be considered?

[isidorn]

@daniv-msft yes that is one option. Though please note that we already have a postDebugTask that runs after debugging, so what you are suggesting is to introduce a third task. While it is a possiblity I would first like to try to find a smaller scoped solution.

But is it possible that your extension simply handles this on it's own?
As part of the resolveDebugConfiguration you can use the Task api to run any task you want?

[daniv-msft]

@isidorn Thank you for your reply. I prototyped what you suggested, and running the task directly from the resolveDebugConfiguration seems to be working.
However, that implies recreating the equivalent of the preLaunchTask logic manually, which is not great:

public async resolveDebugConfigurationAsync(debugConfiguration: vscode.DebugConfiguration): Promise<vscode.DebugConfiguration> {
    const targetTaskName = debugConfiguration[`preLaunchTask`];
    const availableTasks: vscode.Task[] = await vscode.tasks.fetchTasks();
    const targetTask = availableTasks.find(t => t.name === targetTaskName);
    const taskExecution = await vscode.tasks.executeTask(targetTask);
    await new Promise((resolve, reject): void => {
        vscode.tasks.onDidEndTask((e: vscode.TaskEndEvent) => {
            if (e.execution.task.name === taskExecution.task.name) {
                resolve();
            }
        });
    });

    // Code that relies on the task being executed here.
    ...

    // TODO: Find a way so that the preLaunchTask don't get executed another time when resolveDebugConfiguration completes.
}

Also, I'm not sure how sustainable it is on the long run. Typically, if in two months a timeout is added by VS Code on resolveDebugConfiguration to limit the maximum time spent before returning (as it was added for the task provider API), this solution wouldn't work anymore. The task we run takes around 20 seconds to complete, and waits for a user interaction, so we would be very sensitive to any timeout added.

Looking again at the Typescript example you provide, I'm wondering if we could find a solution by resolving variables (such as ${defaultBuildTask}) before running preLaunchTask, but only calling the resolveDebugConfiguration function after that.
That way, we would fix the scenario I'm presenting without breaking Typescript. Would this be possible?

[isidorn]

@daniv-msft great to hear that the workaround is working for you.

Yes the suggestion which you said that we only first resolve variables but do not call resolveDebugConfiguration has crossed my mind, however that might break some other corner case.
However I am open for experimenting with that change and am also open for a PR which tackles this.

[weinand]

@daniv-msft @isidorn
Please note that there are two resolveDebugConfiguration hooks: one before variables have been substituted, one (resolveDebugConfigurationWithSubstitutedVariables) after the substitution took place.
If I understand the current logic correctly the preLaunchTask is run after both hooks.
Would it help (and would it be possible) to run the preLaunchTask after the first hook but before the second?

[isidorn]

@weinand yes I guess we could also change it such that we run resolveDebugConfiguartion then run the prelaunchTask and then run the resolveDebugConfigurationWithSubstitutedVariables. However I am not sure if this would cover @daniv-msft use case.

[weinand]

... and we would not run the second hook if preLaunchTask fails.

[daniv-msft]

@isidorn, @weinand > Thank you for your replies. Yes, that solution would work for me!

[isidorn]

Since that might potentialy break some clients I am assigning this to August so we do this change at the start of the August milestone so we have enough time to gather feedback.

[daniv-msft]

Thanks @isidorn, that sounds indeed reasonable.

[isidorn]

As we agreed upon I have pushed a change such that prelaunchTask is ran before the resolveDebugConfigurationWithSubstitutedVariables
I have verified that this does not break the starter TS ext sample.

@daniv-msft it would be great if you try your use case and let us know if it works for you. VS Code insiders with this change should be out on Friday.

[daniv-msft]

Thanks @isidorn, that's great! I'll give this a try once the VS Code insiders release is available, and I will let you know how it goes.

[daniv-msft]

@isidorn I just wanted to confirm that I've been able to update our code according to the latest version of insiders, and I can confirm that the fix you pushed works for us.
Thank you again for your help fixing our scenario!

[weinand]

@isidorn I wonder whether regressions like #105179 are triggered by this change...

[isidorn]

@weinand might be. Let's wait for the C++ team to investigate #105179 and let us know
fyi @WardenGnaw and @gregg-miskelly for C#. Gist of the above discussion is in this comment

[gregg-miskelly]

I can't think of any reason why this change would be a problem for C#. @WardenGnaw do you have the cycles to see what is going on with #105179?

[WardenGnaw]

#105179 was reported by a member of the Language Service C++ team.

The issue is "Build and Debug Active File" is a TextEditorCommand (C_Cpp.BuildAndDebugActiveFile) and it uses a ConfigurationProvider to find configurations to display to the user. The user selects a configuration and then the command launches the debugger with the selected configuration.

@isidorn @weinand Do we need to write the configuration selected to a launch.json before calling vscode.debug.startDebugging?

Adding C++ Language Server Folks:
@sean-mcmanus who worked on the feature
@elahehrashedi who orignally reported this issue

[isidorn]

@WardenGnaw no, you do not need to write configurations into launch.json before calling startDebugging. You can just pass the full configuration into the startDebugging API and it should just work.

[WardenGnaw]

Actually, it might be the Attempt to use the user's (possibly) modified configuration before using the generated one.

If that call throws an exception, we call startDebugging with the full configuration.

Did VS Code add a feature to show an error dialog when it could not find a matching name launch configuration?

[isidorn]

@WardenGnaw not that I can remember.
If you could write a simple starter extension that shows this problem that would be great.

[WardenGnaw]

@isidorn This is not a regression for 1.49.0 since this also appears in 1.48.1. It is due to trying to use vscode.debug.startDebugging with a configuration that does not exist in launch.json.

@sean-mcmanus We should validate if that configuration exists in launch.json instead of calling vscode.debug.startDebugging and using the thrown exception to detect if we should launch via configuration.

[isidorn]

Thanks for trying it in older versions, that always helps.

[isidorn]

Based on @daniv-msft comment adding verified label.