Fix to return env variables of interpreter that is not current interpreter (#10251)

DonJayamanne · web-flow · commit ae86ae46f6c2 · 2020-02-21T08:55:42.000-08:00
For #10250
* Bug fix
diff --git a/news/2 Fixes/10250.md b/news/2 Fixes/10250.md
@@ -0,0 +1 @@
+Ensure to correctly return env variables of the activated interpreter, when dealing with non-workspace interpreters.
diff --git a/src/client/common/logger.ts b/src/client/common/logger.ts
@@ -290,7 +290,11 @@ function trace(message: string, options: LogOptions = LogOptions.None, logLevel?
             // tslint:disable-next-line:no-any
             function writeToLog(elapsedTime: number, returnValue?: any, ex?: Error) {
                 const messagesToLog = [message];
-                messagesToLog.push(`Class name = ${className}, completed in ${elapsedTime}ms`);
+                messagesToLog.push(
+                    `Class name = ${className}, completed in ${elapsedTime}ms, has a ${
+                        returnValue ? 'truthy' : 'falsy'
+                    } return value`
+                );
                 if ((options && LogOptions.Arguments) === LogOptions.Arguments) {
                     messagesToLog.push(argsToLogString(args));
                 }
diff --git a/src/client/interpreter/activation/service.ts b/src/client/interpreter/activation/service.ts
@@ -6,22 +6,20 @@ import '../../common/extensions';
 import { inject, injectable } from 'inversify';
 import * as path from 'path';
 
+import { IWorkspaceService } from '../../common/application/types';
 import { PYTHON_WARNINGS } from '../../common/constants';
 import { LogOptions, traceDecorators, traceError, traceVerbose } from '../../common/logger';
 import { IPlatformService } from '../../common/platform/types';
 import { IProcessServiceFactory } from '../../common/process/types';
 import { ITerminalHelper, TerminalShellType } from '../../common/terminal/types';
 import { ICurrentProcess, IDisposable, Resource } from '../../common/types';
-import {
-    cacheResourceSpecificInterpreterData,
-    clearCachedResourceSpecificIngterpreterData
-} from '../../common/utils/decorators';
+import { InMemoryCache } from '../../common/utils/cacheUtils';
 import { OSType } from '../../common/utils/platform';
 import { IEnvironmentVariablesProvider } from '../../common/variables/types';
 import { EXTENSION_ROOT_DIR } from '../../constants';
 import { captureTelemetry, sendTelemetryEvent } from '../../telemetry';
 import { EventName } from '../../telemetry/constants';
-import { PythonInterpreter } from '../contracts';
+import { IInterpreterService, PythonInterpreter } from '../contracts';
 import { IEnvironmentActivationService } from './types';
 
 const getEnvironmentPrefix = 'e8b39361-0157-4923-80e1-22d70d46dee6';
@@ -39,15 +37,24 @@ const defaultShells = {
 @injectable()
 export class EnvironmentActivationService implements IEnvironmentActivationService, IDisposable {
     private readonly disposables: IDisposable[] = [];
+    private readonly activatedEnvVariablesCache = new Map<string, InMemoryCache<NodeJS.ProcessEnv | undefined>>();
     constructor(
         @inject(ITerminalHelper) private readonly helper: ITerminalHelper,
         @inject(IPlatformService) private readonly platform: IPlatformService,
         @inject(IProcessServiceFactory) private processServiceFactory: IProcessServiceFactory,
         @inject(ICurrentProcess) private currentProcess: ICurrentProcess,
+        @inject(IWorkspaceService) private workspace: IWorkspaceService,
+        @inject(IInterpreterService) private interpreterService: IInterpreterService,
         @inject(IEnvironmentVariablesProvider) private readonly envVarsService: IEnvironmentVariablesProvider
     ) {
         this.envVarsService.onDidEnvironmentVariablesChange(
-            this.onDidEnvironmentVariablesChange,
+            () => this.activatedEnvVariablesCache.clear(),
+            this,
+            this.disposables
+        );
+
+        this.interpreterService.onDidChangeInterpreter(
+            () => this.activatedEnvVariablesCache.clear(),
             this,
             this.disposables
         );
@@ -58,11 +65,33 @@ export class EnvironmentActivationService implements IEnvironmentActivationServi
     }
     @traceDecorators.verbose('getActivatedEnvironmentVariables', LogOptions.Arguments)
     @captureTelemetry(EventName.PYTHON_INTERPRETER_ACTIVATION_ENVIRONMENT_VARIABLES, { failed: false }, true)
-    @cacheResourceSpecificInterpreterData('ActivatedEnvironmentVariables', cacheDuration)
     public async getActivatedEnvironmentVariables(
         resource: Resource,
         interpreter?: PythonInterpreter,
         allowExceptions?: boolean
+    ): Promise<NodeJS.ProcessEnv | undefined> {
+        // Cache key = resource + interpreter.
+        const workspaceKey = this.workspace.getWorkspaceFolderIdentifier(resource);
+        const interpreterPath = this.platform.isWindows ? interpreter?.path.toLowerCase() : interpreter?.path;
+        const cacheKey = `${workspaceKey}_${interpreterPath}`;
+
+        if (this.activatedEnvVariablesCache.get(cacheKey)?.hasData) {
+            return this.activatedEnvVariablesCache.get(cacheKey)!.data;
+        }
+
+        // Cache only if successful, else keep trying & failing if necessary.
+        const cache = new InMemoryCache<NodeJS.ProcessEnv | undefined>(cacheDuration, '');
+        return this.getActivatedEnvironmentVariablesImpl(resource, interpreter, allowExceptions).then(vars => {
+            cache.data = vars;
+            this.activatedEnvVariablesCache.set(cacheKey, cache);
+            return vars;
+        });
+    }
+
+    public async getActivatedEnvironmentVariablesImpl(
+        resource: Resource,
+        interpreter?: PythonInterpreter,
+        allowExceptions?: boolean
     ): Promise<NodeJS.ProcessEnv | undefined> {
         const shellInfo = defaultShells[this.platform.osType];
         if (!shellInfo) {
@@ -138,9 +167,7 @@ export class EnvironmentActivationService implements IEnvironmentActivationServi
             }
         }
     }
-    protected onDidEnvironmentVariablesChange(affectedResource: Resource) {
-        clearCachedResourceSpecificIngterpreterData('ActivatedEnvironmentVariables', affectedResource);
-    }
+
     protected fixActivationCommands(commands: string[]): string[] {
         // Replace 'source ' with '. ' as that works in shell exec
         return commands.map(cmd => cmd.replace(/^source\s+/, '. '));
diff --git a/src/test/interpreters/activation/service.unit.test.ts b/src/test/interpreters/activation/service.unit.test.ts
@@ -7,8 +7,9 @@ import { EOL } from 'os';
 import * as path from 'path';
 import { SemVer } from 'semver';
 import { anything, capture, instance, mock, verify, when } from 'ts-mockito';
-import * as typemoq from 'typemoq';
-import { EventEmitter, Uri, workspace as workspaceType, WorkspaceConfiguration } from 'vscode';
+import { EventEmitter, Uri } from 'vscode';
+import { IWorkspaceService } from '../../../client/common/application/types';
+import { WorkspaceService } from '../../../client/common/application/workspace';
 import { PlatformService } from '../../../client/common/platform/platformService';
 import { IPlatformService } from '../../../client/common/platform/types';
 import { CurrentProcess } from '../../../client/common/process/currentProcess';
@@ -18,15 +19,14 @@ import { IProcessService, IProcessServiceFactory } from '../../../client/common/
 import { TerminalHelper } from '../../../client/common/terminal/helper';
 import { ITerminalHelper } from '../../../client/common/terminal/types';
 import { ICurrentProcess } from '../../../client/common/types';
-import { clearCache } from '../../../client/common/utils/cacheUtils';
 import { getNamesAndValues } from '../../../client/common/utils/enum';
 import { Architecture, OSType } from '../../../client/common/utils/platform';
 import { EnvironmentVariablesProvider } from '../../../client/common/variables/environmentVariablesProvider';
 import { IEnvironmentVariablesProvider } from '../../../client/common/variables/types';
 import { EXTENSION_ROOT_DIR } from '../../../client/constants';
 import { EnvironmentActivationService } from '../../../client/interpreter/activation/service';
-import { InterpreterType, PythonInterpreter } from '../../../client/interpreter/contracts';
-import { mockedVSCodeNamespaces } from '../../vscode-mock';
+import { IInterpreterService, InterpreterType, PythonInterpreter } from '../../../client/interpreter/contracts';
+import { InterpreterService } from '../../../client/interpreter/interpreterService';
 
 const getEnvironmentPrefix = 'e8b39361-0157-4923-80e1-22d70d46dee6';
 const defaultShells = {
@@ -45,8 +45,10 @@ suite('Interpreters Activation - Python Environment Variables', () => {
     let processService: IProcessService;
     let currentProcess: ICurrentProcess;
     let envVarsService: IEnvironmentVariablesProvider;
-    let workspace: typemoq.IMock<typeof workspaceType>;
-
+    let workspace: IWorkspaceService;
+    let interpreterService: IInterpreterService;
+    let onDidChangeEnvVariables: EventEmitter<Uri | undefined>;
+    let onDidChangeInterpreter: EventEmitter<void>;
     const pythonInterpreter: PythonInterpreter = {
         path: '/foo/bar/python.exe',
         version: new SemVer('3.6.6-final'),
@@ -63,29 +65,22 @@ suite('Interpreters Activation - Python Environment Variables', () => {
         processService = mock(ProcessService);
         currentProcess = mock(CurrentProcess);
         envVarsService = mock(EnvironmentVariablesProvider);
-        workspace = mockedVSCodeNamespaces.workspace!;
-        when(envVarsService.onDidEnvironmentVariablesChange).thenReturn(new EventEmitter<Uri | undefined>().event);
+        interpreterService = mock(InterpreterService);
+        workspace = mock(WorkspaceService);
+        onDidChangeEnvVariables = new EventEmitter<Uri | undefined>();
+        onDidChangeInterpreter = new EventEmitter<void>();
+        when(envVarsService.onDidEnvironmentVariablesChange).thenReturn(onDidChangeEnvVariables.event);
+        when(interpreterService.onDidChangeInterpreter).thenReturn(onDidChangeInterpreter.event);
         service = new EnvironmentActivationService(
             instance(helper),
             instance(platform),
             instance(processServiceFactory),
             instance(currentProcess),
+            instance(workspace),
+            instance(interpreterService),
             instance(envVarsService)
         );
-
-        const cfg = typemoq.Mock.ofType<WorkspaceConfiguration>();
-        workspace
-            .setup(w => w.getConfiguration(typemoq.It.isValue('python'), typemoq.It.isAny()))
-            .returns(() => cfg.object);
-        workspace.setup(w => w.workspaceFolders).returns(() => []);
-        cfg.setup(c => c.inspect(typemoq.It.isValue('pythonPath'))).returns(() => {
-            return { globalValue: 'GlobalValuepython' } as any;
-        });
-        clearCache();
     }
-    teardown(() => {
-        mockedVSCodeNamespaces.workspace!.reset();
-    });
 
     function title(resource?: Uri, interpreter?: PythonInterpreter) {
         return `${resource ? 'With a resource' : 'Without a resource'}${interpreter ? ' and an interpreter' : ''}`;
@@ -262,6 +257,72 @@ suite('Interpreters Activation - Python Environment Variables', () => {
                             verify(envVarsService.getEnvironmentVariables(resource)).once();
                             verify(processService.shellExec(anything(), anything())).once();
                         });
+                        test('Cache Variables', async () => {
+                            const cmd = ['1', '2'];
+                            const varsFromEnv = { one: '11', two: '22', HELLO: 'xxx' };
+                            const stdout = `${getEnvironmentPrefix}${EOL}${JSON.stringify(varsFromEnv)}`;
+                            when(platform.osType).thenReturn(osType.value);
+                            when(
+                                helper.getEnvironmentActivationShellCommands(resource, anything(), interpreter)
+                            ).thenResolve(cmd);
+                            when(processServiceFactory.create(resource)).thenResolve(instance(processService));
+                            when(envVarsService.getEnvironmentVariables(resource)).thenResolve({});
+                            when(processService.shellExec(anything(), anything())).thenResolve({ stdout: stdout });
+
+                            const env = await service.getActivatedEnvironmentVariables(resource, interpreter);
+                            const env2 = await service.getActivatedEnvironmentVariables(resource, interpreter);
+                            const env3 = await service.getActivatedEnvironmentVariables(resource, interpreter);
+
+                            expect(env).to.deep.equal(varsFromEnv);
+                            // All same objects.
+                            expect(env)
+                                .to.equal(env2)
+                                .to.equal(env3);
+
+                            // All methods invoked only once.
+                            verify(
+                                helper.getEnvironmentActivationShellCommands(resource, anything(), interpreter)
+                            ).once();
+                            verify(processServiceFactory.create(resource)).once();
+                            verify(envVarsService.getEnvironmentVariables(resource)).once();
+                            verify(processService.shellExec(anything(), anything())).once();
+                        });
+                        async function testClearingCache(bustCache: Function) {
+                            const cmd = ['1', '2'];
+                            const varsFromEnv = { one: '11', two: '22', HELLO: 'xxx' };
+                            const stdout = `${getEnvironmentPrefix}${EOL}${JSON.stringify(varsFromEnv)}`;
+                            when(platform.osType).thenReturn(osType.value);
+                            when(
+                                helper.getEnvironmentActivationShellCommands(resource, anything(), interpreter)
+                            ).thenResolve(cmd);
+                            when(processServiceFactory.create(resource)).thenResolve(instance(processService));
+                            when(envVarsService.getEnvironmentVariables(resource)).thenResolve({});
+                            when(processService.shellExec(anything(), anything())).thenResolve({ stdout: stdout });
+
+                            const env = await service.getActivatedEnvironmentVariables(resource, interpreter);
+                            bustCache();
+                            const env2 = await service.getActivatedEnvironmentVariables(resource, interpreter);
+
+                            expect(env).to.deep.equal(varsFromEnv);
+                            // Objects are different (not same reference).
+                            expect(env).to.not.equal(env2);
+                            // However variables are the same.
+                            expect(env).to.deep.equal(env2);
+
+                            // All methods invoked twice as cache was blown.
+                            verify(
+                                helper.getEnvironmentActivationShellCommands(resource, anything(), interpreter)
+                            ).twice();
+                            verify(processServiceFactory.create(resource)).twice();
+                            verify(envVarsService.getEnvironmentVariables(resource)).twice();
+                            verify(processService.shellExec(anything(), anything())).twice();
+                        }
+                        test('Cache Variables get cleared when changing interpreter', async () => {
+                            await testClearingCache(onDidChangeInterpreter.fire.bind(onDidChangeInterpreter));
+                        });
+                        test('Cache Variables get cleared when changing env variables file', async () => {
+                            await testClearingCache(onDidChangeEnvVariables.fire.bind(onDidChangeEnvVariables));
+                        });
                     });
                 });
             });

Original file line number	Diff line number	Diff line change
`@@ -0,0 +1 @@`
	`1`	`+Ensure to correctly return env variables of the activated interpreter, when dealing with non-workspace interpreters.`