preLaunchTask does not fire on restart when adapter supportsRestartRequest

[justin-romano]

Steps to Reproduce:

1. Create a build task called "Build" in tasks.json
2. Create a launch configuration in launch.json
3. set the "preLaunchTask": "Build"
4. Launch with F5 and the build task will fire.
5. Press Ctrl+Shift+F5 to restart the debugging session. the build does not fire first.
   Maybe we need a "preRestartTask"

[isidorn]

Works for me, restart will always trigger preLaunchTask.
If you would like us to investigate further please provide a github repository for which you can easily reporduce this. Though I believe this is not a vscode issue since I see other users asking the opposite #17439

[justin-romano]

Sorry i need to further qualify this issue. This is using the chrome debugger.

    "configurations": [{
            "type": "chrome",
            "request": "launch",
            "preLaunchTask": "Build",
            "name": "Launch Chrome against localhost",
            "url": "https://j2017.censored.com/censored/web/index.html",
            "webRoot": "${workspaceRoot}"
        }

[isidorn]

That is because the chrome debuggers is using a custom restart feature.

When a debug adapter is using this capability it is fully in charge what the restart exeprience should be like. So vscode does not do anything (including restarting the task). We currently do not support a half baked approach, where chrome would do some things in the restart and vscode the rest. Leaving this open as a feature request

Ping @roblourens to also get his opinion

[roblourens]

I think it should act the same as when running without supportsRestartRequest. If your task just does a build, then it would be really useful to have it run the build then refresh the browser. If it's a background task, then it should just refresh the browser.

[roblourens]

I thought maybe there was another issue for this where we discussed it, but I can't find it. I don't think it would be possible for the debug extension to implement this.

[isidorn]

@roblourens yes I agree what would be the ideal solution, however as mentioned above due to the current implentation we are not doing that yet (running a prelaucnh task and starting a new debug session are coupled).
Leaving this in the backlog until more users complain

[weinand]

@isidorn When running a postDebugTask in the "supportsRestartRequest == false" case, you use code that checks for postDebugTask being defined and you notify about errors:

if (session.configuration.postDebugTask) {
	this.runTask(session.getId(), session.raw.root, session.configuration.postDebugTask).done(undefined, err =>
		this.notificationService.error(err)
	);
}

For preLaunchTask there is even more error handling:

return this.runTask(sessionId, workspace, resolvedConfig.preLaunchTask).then((taskSummary: ITaskSummary) => {
	const errorCount = resolvedConfig.preLaunchTask ? this.markerService.getStatistics().errors : 0;
	const successExitCode = taskSummary && taskSummary.exitCode === 0;
	const failureExitCode = taskSummary && taskSummary.exitCode !== undefined && taskSummary.exitCode !== 0;
	if (successExitCode || (errorCount === 0 && !failureExitCode)) {
		return this.doCreateSession(workspace, { resolved: resolvedConfig, unresolved: unresolvedConfig }, sessionId);
	}

	const message = errorCount > 1 ? nls.localize('preLaunchTaskErrors', "Build errors have been detected during preLaunchTask '{0}'.", resolvedConfig.preLaunchTask) :
		errorCount === 1 ? nls.localize('preLaunchTaskError', "Build error has been detected during preLaunchTask '{0}'.", resolvedConfig.preLaunchTask) :
			nls.localize('preLaunchTaskExitCode', "The preLaunchTask '{0}' terminated with exit code {1}.", resolvedConfig.preLaunchTask, taskSummary.exitCode);

	const showErrorsAction = new Action('debug.showErrors', nls.localize('showErrors', "Show Errors"), undefined, true, () => {
		return this.panelService.openPanel(Constants.MARKERS_PANEL_ID).then(() => undefined);
	});

	return this.showError(message, [debugAnywayAction, showErrorsAction]);
}, (err: TaskError) => {
	return this.showError(err.message, [debugAnywayAction, this.taskService.configureAction()]);
});

In the "supportsRestartRequest == true" case there is no check for postDebugTask and preLaunchTask being undefined and there is surprising little error notification code:

return this.runTask(session.getId(), session.raw.root, session.configuration.postDebugTask)
	.then(() => this.runTask(session.getId(), session.raw.root, session.configuration.preLaunchTask))
	.then(() => <TPromise>session.raw.custom('restart', null));

Why this discrepancy?

Since the "supportsRestartRequest" is really an implementation detail of a debug adapter, an end user should not be able to observe any discernible difference between DAs supporting "supportsRestartRequest" and those that do not.

I suggest that you refactor the pre- and post-launch code in two separate methods to ensure that the same code is run in both cases.

[isidorn]

@weinand good point, I have tackled this via 27d0fc2

[chrmarti]

I still see the preLaunchTask only run once with the Chrome debug adapter. What is the expected behavior?

[isidorn]

That every time you restart the preLauncTask fires, if that is not the case please reopen and assign to July

[chrmarti]

It works with the Node adapter, but not with the Chrome adapter. My launches:

        {
            "type": "node",
            "request": "launch",
            "name": "runawhile.js",
            "program": "${workspaceFolder}/runawhile.js",
            "preLaunchTask": "npm: hello1",
            "postDebugTask": "npm: hello2",
        },
        {
            "type": "chrome",
            "request": "launch",
            "name": "Chrome server.js",
            "url": "http://localhost:3000",
            "webRoot": "${workspaceFolder}/wwwroot",
            "preLaunchTask": "npm: hello1",
            "postDebugTask": "npm: hello2",
        },

[isidorn]

@chrmarti good catch