Skip to content

Commit 48abee1

Browse files
jakebaileyjakebailey
authored
Switch to local env var cache, remove broken (now unused) InMemoryInterpreterSpecificCache (microsoft#15098)
1 parent fbfbf60 commit 48abee1

11 files changed

Lines changed: 114 additions & 424 deletions

File tree

‎news/2 Fixes/3805.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
Fix environment variables not refreshing on env file edits.

‎src/client/common/utils/cacheUtils.ts‎

Lines changed: 24 additions & 114 deletions
Original file line numberDiff line numberDiff line change
@@ -3,88 +3,6 @@
33

44
'use strict';
55

6-
import { Uri } from 'vscode';
7-
import '../../common/extensions';
8-
import { IServiceContainer } from '../../ioc/types';
9-
import { DEFAULT_INTERPRETER_SETTING } from '../constants';
10-
import { DeprecatePythonPath } from '../experiments/groups';
11-
import { IExperimentsManager, IInterpreterPathService, Resource } from '../types';
12-
13-
type VSCodeType = typeof import('vscode');
14-
type CacheData = {
15-
value: unknown;
16-
expiry: number;
17-
};
18-
const resourceSpecificCacheStores = new Map<string, Map<string, CacheData>>();
19-
20-
/**
21-
* Get a cache key specific to a resource (i.e. workspace)
22-
* This key will be used to cache interpreter related data, hence the Python Path
23-
* used in a workspace will affect the cache key.
24-
* @param {Resource} resource
25-
* @param {VSCodeType} [vscode=require('vscode')]
26-
* @param serviceContainer
27-
* @returns
28-
*/
29-
function getCacheKey(
30-
resource: Resource,
31-
vscode: VSCodeType = require('vscode'),
32-
serviceContainer: IServiceContainer | undefined,
33-
) {
34-
const section = vscode.workspace.getConfiguration('python', vscode.Uri.file(__filename));
35-
if (!section) {
36-
return 'python';
37-
}
38-
let interpreterPathService: IInterpreterPathService | undefined;
39-
let inExperiment: boolean | undefined;
40-
if (serviceContainer) {
41-
interpreterPathService = serviceContainer.get<IInterpreterPathService>(IInterpreterPathService);
42-
const abExperiments = serviceContainer.get<IExperimentsManager>(IExperimentsManager);
43-
inExperiment = abExperiments.inExperiment(DeprecatePythonPath.experiment);
44-
abExperiments.sendTelemetryIfInExperiment(DeprecatePythonPath.control);
45-
}
46-
const globalPythonPath =
47-
inExperiment && interpreterPathService
48-
? interpreterPathService.inspect(vscode.Uri.file(__filename)).globalValue || DEFAULT_INTERPRETER_SETTING
49-
: section.inspect<string>('pythonPath')!.globalValue || DEFAULT_INTERPRETER_SETTING;
50-
// Get the workspace related to this resource.
51-
if (
52-
!resource ||
53-
!Array.isArray(vscode.workspace.workspaceFolders) ||
54-
vscode.workspace.workspaceFolders.length === 0
55-
) {
56-
return globalPythonPath;
57-
}
58-
const folder = resource ? vscode.workspace.getWorkspaceFolder(resource) : vscode.workspace.workspaceFolders[0];
59-
if (!folder) {
60-
return globalPythonPath;
61-
}
62-
const workspacePythonPath =
63-
inExperiment && interpreterPathService
64-
? interpreterPathService.get(resource)
65-
: vscode.workspace.getConfiguration('python', resource).get<string>('pythonPath') ||
66-
DEFAULT_INTERPRETER_SETTING;
67-
return `${folder.uri.fsPath}-${workspacePythonPath}`;
68-
}
69-
/**
70-
* Gets the cache store for a resource that's specific to the interpreter.
71-
* @param {Resource} resource
72-
* @param {VSCodeType} [vscode=require('vscode')]
73-
* @param serviceContainer
74-
* @returns
75-
*/
76-
function getCacheStore(
77-
resource: Resource,
78-
vscode: VSCodeType = require('vscode'),
79-
serviceContainer: IServiceContainer | undefined,
80-
) {
81-
const key = getCacheKey(resource, vscode, serviceContainer);
82-
if (!resourceSpecificCacheStores.has(key)) {
83-
resourceSpecificCacheStores.set(key, new Map<string, CacheData>());
84-
}
85-
return resourceSpecificCacheStores.get(key)!;
86-
}
87-
886
const globalCacheStore = new Map<string, { expiry: number; data: any }>();
897

908
/**
@@ -103,18 +21,23 @@ export function getCacheKeyFromFunctionArgs(keyPrefix: string, fnArgs: any[]): s
10321

10422
export function clearCache() {
10523
globalCacheStore.clear();
106-
resourceSpecificCacheStores.clear();
10724
}
10825

26+
type CacheData<T> = {
27+
value: T;
28+
expiry: number;
29+
};
30+
31+
/**
32+
* InMemoryCache caches a single value up until its expiry.
33+
*/
10934
export class InMemoryCache<T> {
110-
private readonly _store = new Map<string, CacheData>();
111-
protected get store(): Map<string, CacheData> {
112-
return this._store;
113-
}
114-
constructor(protected readonly expiryDurationMs: number, protected readonly cacheKey: string = '') {}
35+
private cacheData?: CacheData<T>;
36+
37+
constructor(protected readonly expiryDurationMs: number) {}
11538
public get hasData() {
116-
if (!this.store.get(this.cacheKey) || this.hasExpired(this.store.get(this.cacheKey)!.expiry)) {
117-
this.store.delete(this.cacheKey);
39+
if (!this.cacheData || this.hasExpired(this.cacheData.expiry)) {
40+
this.cacheData = undefined;
11841
return false;
11942
}
12043
return true;
@@ -128,19 +51,23 @@ export class InMemoryCache<T> {
12851
* @memberof InMemoryCache
12952
*/
13053
public get data(): T | undefined {
131-
if (!this.hasData || !this.store.has(this.cacheKey)) {
54+
if (!this.hasData) {
13255
return;
13356
}
134-
return this.store.get(this.cacheKey)?.value as T;
57+
return this.cacheData?.value;
13558
}
13659
public set data(value: T | undefined) {
137-
this.store.set(this.cacheKey, {
138-
expiry: this.calculateExpiry(),
139-
value,
140-
});
60+
if (value !== undefined) {
61+
this.cacheData = {
62+
expiry: this.calculateExpiry(),
63+
value,
64+
};
65+
} else {
66+
this.cacheData = undefined;
67+
}
14168
}
14269
public clear() {
143-
this.store.clear();
70+
this.cacheData = undefined;
14471
}
14572

14673
/**
@@ -164,20 +91,3 @@ export class InMemoryCache<T> {
16491
return Date.now() + this.expiryDurationMs;
16592
}
16693
}
167-
168-
export class InMemoryInterpreterSpecificCache<T> extends InMemoryCache<T> {
169-
private readonly resource: Resource;
170-
protected get store() {
171-
return getCacheStore(this.resource, this.vscode, this.serviceContainer);
172-
}
173-
constructor(
174-
keyPrefix: string,
175-
expiryDurationMs: number,
176-
args: [Uri | undefined, ...any[]],
177-
private readonly serviceContainer: IServiceContainer | undefined,
178-
private readonly vscode: VSCodeType = require('vscode'),
179-
) {
180-
super(expiryDurationMs, getCacheKeyFromFunctionArgs(keyPrefix, args.slice(1)));
181-
this.resource = args[0];
182-
}
183-
}

‎src/client/common/utils/decorators.ts‎

Lines changed: 1 addition & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,9 @@
11
import { ProgressLocation, ProgressOptions, window } from 'vscode';
22
import '../../common/extensions';
3-
import { IServiceContainer } from '../../ioc/types';
43
import { isTestExecution } from '../constants';
54
import { traceError, traceVerbose } from '../logger';
6-
import { Resource } from '../types';
75
import { createDeferred, Deferred } from './async';
8-
import { getCacheKeyFromFunctionArgs, getGlobalCacheStore, InMemoryInterpreterSpecificCache } from './cacheUtils';
6+
import { getCacheKeyFromFunctionArgs, getGlobalCacheStore } from './cacheUtils';
97
import { TraceInfo, tracing } from './misc';
108

119
const _debounce = require('lodash/debounce') as typeof import('lodash/debounce');
@@ -122,18 +120,6 @@ export function makeDebounceAsyncDecorator(wait?: number) {
122120
};
123121
}
124122

125-
type VSCodeType = typeof import('vscode');
126-
127-
export function clearCachedResourceSpecificIngterpreterData(
128-
key: string,
129-
resource: Resource,
130-
serviceContainer: IServiceContainer,
131-
vscode: VSCodeType = require('vscode'),
132-
) {
133-
const cacheStore = new InMemoryInterpreterSpecificCache(key, 0, [resource], serviceContainer, vscode);
134-
cacheStore.clear();
135-
}
136-
137123
type PromiseFunctionWithAnyArgs = (...any: any) => Promise<any>;
138124
const cacheStoreForMethods = getGlobalCacheStore();
139125
export function cache(expiryDurationMs: number) {

‎src/client/common/variables/environmentVariablesProvider.ts‎

Lines changed: 23 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -3,14 +3,12 @@
33

44
import { inject, injectable, optional } from 'inversify';
55
import { ConfigurationChangeEvent, Disposable, Event, EventEmitter, FileSystemWatcher, Uri } from 'vscode';
6-
import { IServiceContainer } from '../../ioc/types';
76
import { sendFileCreationTelemetry } from '../../telemetry/envFileTelemetry';
87
import { IWorkspaceService } from '../application/types';
98
import { traceVerbose } from '../logger';
109
import { IPlatformService } from '../platform/types';
1110
import { IConfigurationService, ICurrentProcess, IDisposableRegistry } from '../types';
12-
import { InMemoryInterpreterSpecificCache } from '../utils/cacheUtils';
13-
import { clearCachedResourceSpecificIngterpreterData } from '../utils/decorators';
11+
import { InMemoryCache } from '../utils/cacheUtils';
1412
import { EnvironmentVariables, IEnvironmentVariablesProvider, IEnvironmentVariablesService } from './types';
1513

1614
const CACHE_DURATION = 60 * 60 * 1000;
@@ -20,14 +18,15 @@ export class EnvironmentVariablesProvider implements IEnvironmentVariablesProvid
2018
private fileWatchers = new Map<string, FileSystemWatcher>();
2119
private disposables: Disposable[] = [];
2220
private changeEventEmitter: EventEmitter<Uri | undefined>;
21+
private readonly envVarCaches = new Map<string, InMemoryCache<EnvironmentVariables>>();
22+
2323
constructor(
2424
@inject(IEnvironmentVariablesService) private envVarsService: IEnvironmentVariablesService,
2525
@inject(IDisposableRegistry) disposableRegistry: Disposable[],
2626
@inject(IPlatformService) private platformService: IPlatformService,
2727
@inject(IWorkspaceService) private workspaceService: IWorkspaceService,
2828
@inject(IConfigurationService) private readonly configurationService: IConfigurationService,
2929
@inject(ICurrentProcess) private process: ICurrentProcess,
30-
@inject(IServiceContainer) private serviceContainer: IServiceContainer,
3130
@optional() private cacheDuration: number = CACHE_DURATION,
3231
) {
3332
disposableRegistry.push(this);
@@ -50,20 +49,25 @@ export class EnvironmentVariablesProvider implements IEnvironmentVariablesProvid
5049
}
5150

5251
public async getEnvironmentVariables(resource?: Uri): Promise<EnvironmentVariables> {
53-
// Cache resource specific interpreter data
54-
const cacheStore = new InMemoryInterpreterSpecificCache(
55-
'getEnvironmentVariables',
56-
this.cacheDuration,
57-
[resource],
58-
this.serviceContainer,
59-
);
60-
if (cacheStore.hasData) {
61-
traceVerbose(`Cached data exists getEnvironmentVariables, ${resource ? resource.fsPath : '<No Resource>'}`);
62-
return Promise.resolve(cacheStore.data) as Promise<EnvironmentVariables>;
52+
const cacheKey = this.getWorkspaceFolderUri(resource)?.fsPath ?? '';
53+
let cache = this.envVarCaches.get(cacheKey);
54+
55+
if (cache) {
56+
const cachedData = cache.data;
57+
if (cachedData) {
58+
traceVerbose(
59+
`Cached data exists getEnvironmentVariables, ${resource ? resource.fsPath : '<No Resource>'}`,
60+
);
61+
return { ...cachedData };
62+
}
63+
} else {
64+
cache = new InMemoryCache(this.cacheDuration);
65+
this.envVarCaches.set(cacheKey, cache);
6366
}
64-
const promise = this._getEnvironmentVariables(resource);
65-
promise.then((result) => (cacheStore.data = result)).ignoreErrors();
66-
return promise;
67+
68+
const vars = await this._getEnvironmentVariables(resource);
69+
cache.data = { ...vars };
70+
return vars;
6771
}
6872
public async _getEnvironmentVariables(resource?: Uri): Promise<EnvironmentVariables> {
6973
let mergedVars = await this.getCustomEnvironmentVariables(resource);
@@ -122,16 +126,8 @@ export class EnvironmentVariablesProvider implements IEnvironmentVariablesProvid
122126
}
123127

124128
private onEnvironmentFileChanged(workspaceFolderUri?: Uri) {
125-
clearCachedResourceSpecificIngterpreterData(
126-
'getEnvironmentVariables',
127-
workspaceFolderUri,
128-
this.serviceContainer,
129-
);
130-
clearCachedResourceSpecificIngterpreterData(
131-
'CustomEnvironmentVariables',
132-
workspaceFolderUri,
133-
this.serviceContainer,
134-
);
129+
// An environment file changing can affect multiple workspaces; clear everything and reparse later.
130+
this.envVarCaches.clear();
135131
this.changeEventEmitter.fire(workspaceFolderUri);
136132
}
137133
}

‎src/client/interpreter/activation/service.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -134,7 +134,7 @@ export class EnvironmentActivationService implements IEnvironmentActivationServi
134134
}
135135

136136
// Cache only if successful, else keep trying & failing if necessary.
137-
const cache = new InMemoryCache<NodeJS.ProcessEnv | undefined>(cacheDuration, '');
137+
const cache = new InMemoryCache<NodeJS.ProcessEnv | undefined>(cacheDuration);
138138
return this.getActivatedEnvironmentVariablesImpl(resource, interpreter, allowExceptions).then((vars) => {
139139
cache.data = vars;
140140
this.activatedEnvVariablesCache.set(cacheKey, cache);

0 commit comments

Comments
 (0)