fix: don't overwrite custom NODE_OPTIONS (#1771)

Fixes #1746
microsoft · Aug 2, 2023 · 670bb82 · 670bb82
1 parent 4d7c704
commit 670bb82
Show file tree

Hide file tree

Showing 6 changed files with 53 additions and 64 deletions.
diff --git a/src/targets/node/autoAttachLauncher.ts b/src/targets/node/autoAttachLauncher.ts
@@ -26,7 +26,6 @@ import {
   BootloaderEnvironment,
   IAutoAttachInfo,
   IBootloaderEnvironment,
-  variableDelimiter,
 } from './bootloader/environment';
 import { bootloaderDefaultPath, watchdogPath } from './bundlePaths';
 import { Capability, INodeBinaryProvider, NodeBinary } from './nodeBinaryProvider';
@@ -154,11 +153,8 @@ export class AutoAttachLauncher
         ],
       }),
     );
-    variables.prepend('NODE_OPTIONS', bootloaderEnv.NODE_OPTIONS + ' ');
-    variables.append(
-      'VSCODE_INSPECTOR_OPTIONS',
-      variableDelimiter + bootloaderEnv.VSCODE_INSPECTOR_OPTIONS,
-    );
+    variables.prepend('NODE_OPTIONS', bootloaderEnv.NODE_OPTIONS);
+    variables.append('VSCODE_INSPECTOR_OPTIONS', bootloaderEnv.VSCODE_INSPECTOR_OPTIONS);
   }
 
   private readSmartPatterns() {

diff --git a/src/targets/node/extensionHostAttacher.ts b/src/targets/node/extensionHostAttacher.ts
@@ -3,11 +3,9 @@
  *--------------------------------------------------------*/
 
 import { injectable } from 'inversify';
-import { getSourceSuffix } from '../../adapter/templates';
 import Cdp from '../../cdp/api';
 import { DebugType } from '../../common/contributionUtils';
-import { ILogger, LogTag } from '../../common/logging';
-import { delay } from '../../common/promiseUtil';
+import { ILogger } from '../../common/logging';
 import { Semver } from '../../common/semver';
 import {
   AnyLaunchConfiguration,
@@ -143,35 +141,12 @@ export class ExtensionHostAttacher extends NodeAttacherBase<IExtensionHostAttach
       return;
     }
 
-    let vars = await this.resolveEnvironment(
+    const vars = await this.resolveEnvironment(
       run,
       new NodeBinary('node', Semver.parse(process.versions.node)),
       { openerId: targetId },
     );
 
-    vars = vars.update('ELECTRON_RUN_AS_NODE', null);
-
-    for (let retries = 0; retries < 200; retries++) {
-      const result = await cdp.Runtime.evaluate({
-        contextId: 1,
-        returnByValue: true,
-        expression:
-          `Object.assign(process.env, ${JSON.stringify(vars.defined())})` + getSourceSuffix(),
-      });
-
-      if (!result) {
-        this.logger.error(LogTag.RuntimeTarget, 'Undefined result setting child environment vars');
-      } else if (result.exceptionDetails) {
-        this.logger.error(
-          LogTag.RuntimeTarget,
-          'Error setting child environment vars',
-          result.exceptionDetails,
-        );
-      } else {
-        return;
-      }
-
-      await delay(50);
-    }
+    return this.appendEnvironmentVariables(cdp, vars.update('ELECTRON_RUN_AS_NODE', null));
   }
 }
diff --git a/src/targets/node/nodeAttacher.ts b/src/targets/node/nodeAttacher.ts
@@ -5,7 +5,6 @@
 import * as l10n from '@vscode/l10n';
 import { inject, injectable } from 'inversify';
 import { IPortLeaseTracker } from '../../adapter/portLeaseTracker';
-import { getSourceSuffix } from '../../adapter/templates';
 import Cdp from '../../cdp/api';
 import { CancellationTokenSource } from '../../common/cancellation';
 import { DebugType } from '../../common/contributionUtils';
@@ -217,27 +216,6 @@ export class NodeAttacher extends NodeAttacherBase<INodeAttachConfiguration> {
     }
 
     const vars = await this.resolveEnvironment(run, binary, { requireLease: leasePath, openerId });
-    for (let retries = 0; retries < 5; retries++) {
-      const result = await cdp.Runtime.evaluate({
-        contextId: 1,
-        returnByValue: true,
-        expression:
-          `typeof process === 'undefined' || process.pid === undefined ? 'process not defined' : Object.assign(process.env, ${JSON.stringify(
-            vars.defined(),
-          )})` + getSourceSuffix(),
-      });
-
-      if (!result) {
-        this.logger.error(LogTag.RuntimeTarget, 'Undefined result setting child environment vars');
-        return;
-      }
-
-      if (!result.exceptionDetails && result.result.value !== 'process not defined') {
-        return;
-      }
-
-      this.logger.error(LogTag.RuntimeTarget, 'Error setting child environment vars', result);
-      await delay(50);
-    }
+    return this.appendEnvironmentVariables(cdp, vars);
   }
 }
diff --git a/src/targets/node/nodeAttacherBase.ts b/src/targets/node/nodeAttacherBase.ts
@@ -3,13 +3,49 @@
  *--------------------------------------------------------*/
 
 import { injectable } from 'inversify';
+import { getSourceSuffix } from '../../adapter/templates';
+import Cdp from '../../cdp/api';
+import { EnvironmentVars } from '../../common/environmentVars';
+import { LogTag } from '../../common/logging';
+import { delay } from '../../common/promiseUtil';
 import { AnyNodeConfiguration } from '../../configuration';
 import { NodeLauncherBase } from './nodeLauncherBase';
 
 /**
  * Base class that implements common matters for attachment.
  */
 @injectable()
-export abstract class NodeAttacherBase<
-  T extends AnyNodeConfiguration,
-> extends NodeLauncherBase<T> {}
+export abstract class NodeAttacherBase<T extends AnyNodeConfiguration> extends NodeLauncherBase<T> {
+  protected async appendEnvironmentVariables(cdp: Cdp.Api, vars: EnvironmentVars) {
+    const expression =
+      `typeof process==='undefined'||process.pid===undefined?'process not defined':(()=>{` +
+      Object.entries(vars.defined())
+        .map(([key, value]) => {
+          const k = JSON.stringify(key);
+          return `process.env[${k}]=(process.env[${k}]||'')+${JSON.stringify(value)}`;
+        })
+        .join(';') +
+      '})()' +
+      getSourceSuffix();
+
+    for (let retries = 0; retries < 5; retries++) {
+      const result = await cdp.Runtime.evaluate({
+        contextId: 1,
+        returnByValue: true,
+        expression,
+      });
+
+      if (!result) {
+        this.logger.error(LogTag.RuntimeTarget, 'Undefined result setting child environment vars');
+        return;
+      }
+
+      if (!result.exceptionDetails && result.result.value !== 'process not defined') {
+        return;
+      }
+
+      this.logger.error(LogTag.RuntimeTarget, 'Error setting child environment vars', result);
+      await delay(50);
+    }
+  }
+}
diff --git a/src/targets/node/nodeLauncherBase.ts b/src/targets/node/nodeLauncherBase.ts
@@ -36,7 +36,11 @@ import {
 } from '../../targets/targets';
 import { ITelemetryReporter } from '../../telemetry/telemetryReporter';
 import { ISourcePathResolverFactory } from '../sourcePathResolverFactory';
-import { IBootloaderEnvironment, IBootloaderInfo } from './bootloader/environment';
+import {
+  IBootloaderEnvironment,
+  IBootloaderInfo,
+  variableDelimiter,
+} from './bootloader/environment';
 import { bootloaderDefaultPath } from './bundlePaths';
 import { Capability, INodeBinaryProvider, NodeBinary } from './nodeBinaryProvider';
 import { NodeSourcePathResolver } from './nodeSourcePathResolver';
@@ -312,8 +316,8 @@ export abstract class NodeLauncherBase<T extends AnyNodeConfiguration> implement
     const env = {
       // Require our bootloader first, to run it before any other bootloader
       // we could have injected in the parent process.
-      NODE_OPTIONS: `--require ${bootloader.interpolatedPath}`,
-      VSCODE_INSPECTOR_OPTIONS: JSON.stringify(bootloaderInfo),
+      NODE_OPTIONS: ` --require ${bootloader.interpolatedPath} `,
+      VSCODE_INSPECTOR_OPTIONS: variableDelimiter + JSON.stringify(bootloaderInfo),
       ELECTRON_RUN_AS_NODE: null,
     } as IBootloaderEnvironment;
 

diff --git a/src/test/node/node-runtime.test.ts b/src/test/node/node-runtime.test.ts
@@ -657,7 +657,7 @@ describe('node runtime', () => {
       const worker = await r.worker();
       const optionsOut = worker.dap.once('output', o => o.output.includes('NODE_OPTIONS'));
       handle.logger.logOutput(await optionsOut);
-      handle.assertLog({ customAssert: l => expect(l).to.contain('NODE_OPTIONS= --require') });
+      handle.assertLog({ customAssert: l => expect(l).to.contain('NODE_OPTIONS=  --require') });
     });
 
     itIntegrates('allows simple port attachment', async ({ r }) => {