src/goDebugFactory,goDebugConfiguration: adjust env computation

When spawning delve, the extension wants to run it in the environment
computed by merging
   1. process.env
   2. go.toolsEnvVars from settings.json
   3. envFile from launch.json
   4. env from launch.json

This is true for both when we use legacy debug adapter and dlv dap
mode.

For some time, we did this environment variable merging from
goDebugConfigurationProvider.

Recently in https://go-review.googlesource.com/c/vscode-go/+/349749,
we changed this logic for dlv dap mode
  - goDebugConfigurationProvider merges only 3 and 4 and stuff the
    result to the launch configuration.
  - from DelveDAPOutputAdapter (thin adapter), we merge 1, 2, and
    the info from the launch configuration (3, 4) and use the result
    when spawning the `dlv dap` process.

We made the change because including 1 and 2 in the launch configuration
resulted in transmitting all the process.env over the wire as launch
parameter unncessarily, and that made users reluctant to share their
debug traces or, made users accidentally share too much info including
sensitive information.

Now with the changes to support the `console` mode, the extension
delegates spawning of `dlv dap` to the VS Code using VS Code's
RunInTerminal implementation. Before this change, we sent environment
variables merging all 1, 2, 3, and 4 as RunInTerminal's env property.
It turned out VS Code's RunInTerminal formulates the final command
to run using the `env` command (in Unix) and lists all the environment
variables. As a result, including process.env (1) in the env property
is problematic:

 - The terminal is crowded and hurts user experience.
 - It may result in a long command and that will suffer from problems
   like microsoft/vscode#134324

Usually, users add only a handful of extra environment variables
in the launch configuration and the go.toolsEnvVars configuration.
And most likely, the terminal will inherit most of the process.env
so we don't need to carry them around.

This change thus skips process.env in RunInTerminal's env property,
but includes only 2, and 3, 4 from goDebugConfigurationProvider.

We still need to merge 1 when the extension spawns 'dlv dap' without
RunInTerminal mode, because Node.JS child_process requires all env
variables should be included.

Updates #124

Change-Id: I33ad5490c6d11a3afb0e569563689be08aefcd06
Reviewed-on: https://go-review.googlesource.com/c/vscode-go/+/361101
Trust: Hyang-Ah Hana Kim <hyangah@gmail.com>
Run-TryBot: Hyang-Ah Hana Kim <hyangah@gmail.com>
Reviewed-by: Suzy Mueller <suzmue@golang.org>

src/goDebugConfiguration.ts

@@ -401,10 +401,11 @@ export class GoDebugConfigurationProvider implements vscode.DebugConfigurationPr
 		// For legacy mode, we merge the environment variables on top of
 		// the tools execution environment variables and update the debugConfiguration
 		// because VS Code directly handles launch of the legacy debug adapter.
-		// For dlv-dap mode, we do not merge the tools execution environment
+		// For dlv-dap mode, we do not merge process.env environment
 		// variables here to reduce the number of environment variables passed
 		// as launch/attach parameters.
-		const goToolsEnvVars = debugAdapter === 'legacy' ? toolExecutionEnvironment(folder?.uri) : {};
+		const mergeProcessEnv = debugAdapter === 'legacy';
+		const goToolsEnvVars = toolExecutionEnvironment(folder?.uri, mergeProcessEnv);
 		const fileEnvs = parseEnvFiles(debugConfiguration['envFile']);
 		const env = debugConfiguration['env'] || {};
 

src/goDebugFactory.ts

@@ -15,7 +15,6 @@ import * as net from 'net';
 import { Logger, logVerbose, TimestampedLogger } from './goLogging';
 import { DebugProtocol } from 'vscode-debugprotocol';
 import { getWorkspaceFolderPath } from './util';
-import { toolExecutionEnvironment } from './goEnv';
 import { envPath, getBinPathFromEnvVar } from './utils/pathUtils';
 
 export class GoDebugAdapterDescriptorFactory implements vscode.DebugAdapterDescriptorFactory {
@@ -460,13 +459,18 @@ function waitForDAPServer(port: number, timeoutMs: number): Promise<net.Socket>
 		}, timeoutMs);
 
 		s = net.createServer({ pauseOnConnect: true }, (socket) => {
-			logVerbose(`connected: ${port}`);
+			logVerbose(
+				`connected: ${port} (remote: ${socket.remoteAddress}:${socket.remotePort} local: ${socket.localAddress}:${socket.localPort})`
+			);
 			clearTimeout(timeoutToken);
 			s.close(); // accept no more connection
 			socket.resume();
 			resolve(socket);
 		});
-		s.on('error', (err) => reject(err));
+		s.on('error', (err) => {
+			logVerbose(`connection error ${err}`);
+			reject(err);
+		});
 		s.maxConnections = 1;
 		s.listen(port);
 	});
@@ -481,6 +485,9 @@ function spawnDlvDapServerProcess(
 	logConsole: (msg: string) => void
 ): Promise<ChildProcess> {
 	const { dlvArgs, dlvPath, dir, env } = getSpawnConfig(launchAttachArgs, logErr);
+	// env does not include process.env. Construct the new env for process spawning
+	// by combining process.env.
+	const envForSpawn = env ? Object.assign({}, process.env, env) : undefined;
 
 	dlvArgs.push(`--listen=${host}:${port}`);
 
@@ -519,7 +526,7 @@ function spawnDlvDapServerProcess(
 	return new Promise<ChildProcess>((resolve, reject) => {
 		const p = spawn(dlvPath, dlvArgs, {
 			cwd: dir,
-			env,
+			env: envForSpawn,
 			stdio: onWindows ? ['pipe', 'pipe', 'pipe'] : ['pipe', 'pipe', 'pipe', 'pipe'] // --log-dest=3 if !onWindows.
 		});
 		let started = false;
@@ -590,12 +597,13 @@ function spawnDlvDapServerProcess(
 		});
 	});
 }
-function getSpawnConfig(launchAttachArgs: vscode.DebugConfiguration, logErr: (msg: string) => void) {
-	const launchArgsEnv = launchAttachArgs.env || {};
-	const goToolsEnvVars = toolExecutionEnvironment();
-	// launchArgsEnv is user-requested env vars (envFiles + env).
-	const env = Object.assign(goToolsEnvVars, launchArgsEnv);
 
+// getSpawnConfig returns the dlv args, directory, and dlv path necessary to spawn the dlv command.
+// It also returns `env` that is the additional environment variables users want to run the dlv
+// and the debuggee (i.e., go.toolsEnvVars, launch configuration's env and envFile) with.
+function getSpawnConfig(launchAttachArgs: vscode.DebugConfiguration, logErr: (msg: string) => void) {
+	// launchArgsEnv is user-requested env vars (envFiles + env + toolsEnvVars).
+	const env = launchAttachArgs.env;
 	const dlvPath = launchAttachArgs.dlvToolPath ?? 'dlv';
 
 	if (!fs.existsSync(dlvPath)) {

src/goEnv.ts

@@ -54,8 +54,8 @@ export function toolInstallationEnvironment(): NodeJS.Dict<string> {
 
 // toolExecutionEnvironment returns the environment in which tools should
 // be executed. It always returns a new object.
-export function toolExecutionEnvironment(uri?: vscode.Uri): NodeJS.Dict<string> {
-	const env = newEnvironment();
+export function toolExecutionEnvironment(uri?: vscode.Uri, addProcessEnv = true): NodeJS.Dict<string> {
+	const env = newEnvironment(addProcessEnv);
 	const gopath = getCurrentGoPath(uri);
 	if (gopath) {
 		env['GOPATH'] = gopath;
@@ -69,9 +69,9 @@ export function toolExecutionEnvironment(uri?: vscode.Uri): NodeJS.Dict<string>
 	return env;
 }
 
-function newEnvironment(): NodeJS.Dict<string> {
+function newEnvironment(addProcessEnv = true): NodeJS.Dict<string> {
 	const toolsEnvVars = getGoConfig()['toolsEnvVars'];
-	const env = Object.assign({}, process.env, toolsEnvVars);
+	const env = addProcessEnv ? Object.assign({}, process.env, toolsEnvVars) : Object.assign({}, toolsEnvVars);
 	if (toolsEnvVars && typeof toolsEnvVars === 'object') {
 		Object.keys(toolsEnvVars).forEach(
 			(key) =>

test/integration/goDebug.test.ts

@@ -2309,9 +2309,10 @@ class DelveDAPDebugAdapterOnSocket extends proxy.DelveDAPOutputAdapter {
 
 		if (!this._dlvInTerminal && m0['arguments']?.args?.length > 0) {
 			const args = m0['arguments'].args as string[];
+			const env = m0['arguments'].env ? Object.assign({}, process.env, m0['arguments'].env) : undefined;
 			const p = cp.spawn(args[0], args.slice(1), {
 				cwd: m0['arguments'].cwd,
-				env: m0['arguments'].env
+				env
 			});
 			// stdout/stderr are supposed to appear in the terminal, but
 			// some of noDebug tests depend on access to stdout/stderr.

test/integration/goDebugConfiguration.test.ts

@@ -91,7 +91,7 @@ suite('Debug Environment Variable Merge Test', () => {
 		);
 	});
 
-	test('toolsEnvVars is not propagated', () => {
+	test('toolsEnvVars is propagated', () => {
 		const toolsEnv = {
 			GOPATH: '/gopath',
 			GOOS: 'valueFromToolsEnv'
@@ -101,7 +101,7 @@ suite('Debug Environment Variable Merge Test', () => {
 			{
 				toolsEnv
 			},
-			{}
+			toolsEnv
 		);
 	});
 
@@ -155,7 +155,7 @@ suite('Debug Environment Variable Merge Test', () => {
 		);
 	});
 
-	test('launchArgs.env is respected, toolsEnvVar is ignored (dlv-dap)', () => {
+	test('launchArgs.env and toolsEnvVar is respected', () => {
 		const toolsEnv = {
 			GOPATH: '/gopath',
 			SOMEVAR1: 'valueFromToolsEnvVar1',
@@ -166,7 +166,9 @@ suite('Debug Environment Variable Merge Test', () => {
 		runTest(
 			{ env, toolsEnv },
 			{
-				SOMEVAR1: 'valueFromEnv'
+				GOPATH: '/gopath',
+				SOMEVAR1: 'valueFromEnv',
+				SOMEVAR2: 'valueFromToolsEnvVar2'
 			}
 		);
 	});
@@ -191,7 +193,7 @@ suite('Debug Environment Variable Merge Test', () => {
 		);
 	});
 
-	test('launchArgs.envFile is repected, and toolsEnvVar is ignored (dlv-dap)', () => {
+	test('launchArgs.envFile and toolsEnvVar are repected (dlv-dap)', () => {
 		const toolsEnv = {
 			GOPATH: '/gopath',
 			SOMEVAR1: 'valueFromToolsEnvVar1',
@@ -204,6 +206,8 @@ suite('Debug Environment Variable Merge Test', () => {
 		runTest(
 			{ debugAdapter, toolsEnv, envFile },
 			{
+				GOPATH: '/gopath',
+				SOMEVAR1: 'valueFromToolsEnvVar1',
 				SOMEVAR2: 'valueFromEnvFile2'
 			}
 		);