Skip to content

Commit fa75337

Browse files
author
Kartik Raj
authored
Fix duplicated environments in interpreter list (#22964)
Failing tests are due to #22965, this can still be reviewed. Closes #22918 closes #22146
1 parent 4fca030 commit fa75337

File tree

9 files changed

+74
-91
lines changed

9 files changed

+74
-91
lines changed

‎src/client/pythonEnvironments/base/locator.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,7 @@ export type PythonEnvUpdatedEvent<I = PythonEnvInfo> = {
2020
/**
2121
* The iteration index of The env info that was previously provided.
2222
*/
23-
index?: number;
23+
index: number;
2424
/**
2525
* The env info that was previously provided.
2626
*/
@@ -243,7 +243,7 @@ export interface IDiscoveryAPI {
243243
resolveEnv(path: string): Promise<PythonEnvInfo | undefined>;
244244
}
245245

246-
interface IEmitter<E> {
246+
export interface IEmitter<E> {
247247
fire(e: E): void;
248248
}
249249

‎src/client/pythonEnvironments/base/locatorUtils.ts

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -95,8 +95,6 @@ export async function getEnvs<I = PythonEnvInfo>(iterator: IPythonEnvsIterator<I
9595
}
9696
// We don't worry about if envs[index] is set already.
9797
envs[index] = update;
98-
} else if (event.update) {
99-
envs.push(event.update);
10098
}
10199
});
102100
}

‎src/client/pythonEnvironments/base/locators/common/resourceBasedLocator.ts

Lines changed: 2 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -43,18 +43,9 @@ export abstract class LazyResourceBasedLocator extends Locator<BasicEnvInfo> imp
4343
await this.disposables.dispose();
4444
}
4545

46-
public iterEnvs(query?: PythonLocatorQuery): IPythonEnvsIterator<BasicEnvInfo> {
47-
const iterator = this.doIterEnvs(query);
48-
const it = this._iterEnvs(iterator, query);
49-
it.onUpdated = iterator.onUpdated;
50-
return it;
51-
}
52-
53-
private async *_iterEnvs(
54-
iterator: IPythonEnvsIterator<BasicEnvInfo>,
55-
query?: PythonLocatorQuery,
56-
): IPythonEnvsIterator<BasicEnvInfo> {
46+
public async *iterEnvs(query?: PythonLocatorQuery): IPythonEnvsIterator<BasicEnvInfo> {
5747
await this.activate();
48+
const iterator = this.doIterEnvs(query);
5849
if (query?.envPath) {
5950
let result = await iterator.next();
6051
while (!result.done) {

‎src/client/pythonEnvironments/base/locators/composite/envsCollectionService.ts

Lines changed: 2 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -141,12 +141,12 @@ export class EnvsCollectionService extends PythonEnvsWatcher<PythonEnvCollection
141141
const updatesDone = createDeferred<void>();
142142

143143
if (iterator.onUpdated !== undefined) {
144-
iterator.onUpdated(async (event) => {
144+
const listener = iterator.onUpdated(async (event) => {
145145
if (isProgressEvent(event)) {
146146
switch (event.stage) {
147147
case ProgressReportStage.discoveryFinished:
148148
state.done = true;
149-
// listener.dispose();
149+
listener.dispose();
150150
break;
151151
case ProgressReportStage.allPathsDiscovered:
152152
if (!query) {
@@ -164,10 +164,6 @@ export class EnvsCollectionService extends PythonEnvsWatcher<PythonEnvCollection
164164
seen[event.index] = event.update;
165165
}
166166
state.pending -= 1;
167-
} else if (event.update) {
168-
// New env, add it to cache.
169-
seen.push(event.update);
170-
this.cache.addEnv(event.update);
171167
}
172168
if (state.done && state.pending === 0) {
173169
updatesDone.resolve();

‎src/client/pythonEnvironments/base/locators/composite/envsReducer.ts

Lines changed: 13 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -49,15 +49,12 @@ async function* iterEnvsIterator(
4949
};
5050
const seen: BasicEnvInfo[] = [];
5151

52-
didUpdate.fire({ stage: ProgressReportStage.discoveryStarted });
5352
if (iterator.onUpdated !== undefined) {
54-
iterator.onUpdated((event) => {
53+
const listener = iterator.onUpdated((event) => {
5554
if (isProgressEvent(event)) {
5655
if (event.stage === ProgressReportStage.discoveryFinished) {
5756
state.done = true;
58-
// For super slow locators such as Windows registry, we expect updates even after discovery
59-
// is "officially" finished, hence do not dispose listeners.
60-
// listener.dispose();
57+
listener.dispose();
6158
} else {
6259
didUpdate.fire(event);
6360
}
@@ -69,11 +66,15 @@ async function* iterEnvsIterator(
6966
const oldEnv = seen[event.index];
7067
seen[event.index] = event.update;
7168
didUpdate.fire({ index: event.index, old: oldEnv, update: event.update });
72-
} else if (event.update) {
73-
didUpdate.fire({ update: event.update });
69+
} else {
70+
// This implies a problem in a downstream locator
71+
traceVerbose(`Expected already iterated env, got ${event.old} (#${event.index})`);
7472
}
73+
state.pending -= 1;
7574
checkIfFinishedAndNotify(state, didUpdate);
7675
});
76+
} else {
77+
didUpdate.fire({ stage: ProgressReportStage.discoveryStarted });
7778
}
7879

7980
let result = await iterator.next();
@@ -89,8 +90,10 @@ async function* iterEnvsIterator(
8990
}
9091
result = await iterator.next();
9192
}
92-
state.done = true;
93-
checkIfFinishedAndNotify(state, didUpdate);
93+
if (iterator.onUpdated === undefined) {
94+
state.done = true;
95+
checkIfFinishedAndNotify(state, didUpdate);
96+
}
9497
}
9598

9699
async function resolveDifferencesInBackground(
@@ -124,8 +127,8 @@ function checkIfFinishedAndNotify(
124127
) {
125128
if (state.done && state.pending === 0) {
126129
didUpdate.fire({ stage: ProgressReportStage.discoveryFinished });
130+
didUpdate.dispose();
127131
traceVerbose(`Finished with environment reducer`);
128-
state.done = false; // No need to notify again.
129132
}
130133
}
131134

‎src/client/pythonEnvironments/base/locators/composite/envsResolver.ts

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -81,30 +81,29 @@ export class PythonEnvsResolver implements IResolvingLocator {
8181
const seen: PythonEnvInfo[] = [];
8282

8383
if (iterator.onUpdated !== undefined) {
84-
iterator.onUpdated(async (event) => {
84+
const listener = iterator.onUpdated(async (event) => {
8585
state.pending += 1;
8686
if (isProgressEvent(event)) {
8787
if (event.stage === ProgressReportStage.discoveryFinished) {
8888
didUpdate.fire({ stage: ProgressReportStage.allPathsDiscovered });
8989
state.done = true;
90-
// For super slow locators such as Windows registry, we expect updates even after discovery
91-
// is "officially" finished, hence do not dispose listeners.
92-
// listener.dispose();
90+
listener.dispose();
9391
} else {
9492
didUpdate.fire(event);
9593
}
9694
} else if (event.update === undefined) {
9795
throw new Error(
9896
'Unsupported behavior: `undefined` environment updates are not supported from downstream locators in resolver',
9997
);
100-
} else if (event.index && seen[event.index] !== undefined) {
98+
} else if (event.index !== undefined && seen[event.index] !== undefined) {
10199
const old = seen[event.index];
102100
await setKind(event.update, environmentKinds);
103101
seen[event.index] = await resolveBasicEnv(event.update);
104102
didUpdate.fire({ old, index: event.index, update: seen[event.index] });
105103
this.resolveInBackground(event.index, state, didUpdate, seen).ignoreErrors();
106104
} else {
107-
didUpdate.fire({ update: await this.resolveEnv(event.update.executablePath) });
105+
// This implies a problem in a downstream locator
106+
traceVerbose(`Expected already iterated env, got ${event.old} (#${event.index})`);
108107
}
109108
state.pending -= 1;
110109
checkIfFinishedAndNotify(state, didUpdate);
@@ -174,6 +173,7 @@ function checkIfFinishedAndNotify(
174173
) {
175174
if (state.done && state.pending === 0) {
176175
didUpdate.fire({ stage: ProgressReportStage.discoveryFinished });
176+
didUpdate.dispose();
177177
traceVerbose(`Finished with environment resolver`);
178178
}
179179
}

‎src/client/pythonEnvironments/base/locators/lowLevel/windowsRegistryLocator.ts

Lines changed: 23 additions & 46 deletions
Original file line numberDiff line numberDiff line change
@@ -3,76 +3,53 @@
33
// Copyright (c) Microsoft Corporation. All rights reserved.
44
// Licensed under the MIT License.
55

6-
import { EventEmitter } from 'vscode';
76
import { PythonEnvKind, PythonEnvSource } from '../../info';
8-
import {
9-
BasicEnvInfo,
10-
IPythonEnvsIterator,
11-
Locator,
12-
ProgressNotificationEvent,
13-
ProgressReportStage,
14-
PythonEnvUpdatedEvent,
15-
} from '../../locator';
7+
import { BasicEnvInfo, IPythonEnvsIterator, Locator, PythonLocatorQuery, IEmitter } from '../../locator';
168
import { getRegistryInterpreters } from '../../../common/windowsUtils';
179
import { traceError, traceVerbose } from '../../../../logging';
1810
import { isMicrosoftStoreDir } from '../../../common/environmentManagers/microsoftStoreEnv';
1911
import { inExperiment } from '../../../common/externalDependencies';
2012
import { DiscoveryUsingWorkers } from '../../../../common/experiments/groups';
13+
import { PythonEnvsChangedEvent } from '../../watcher';
14+
15+
export const WINDOWS_REG_PROVIDER_ID = 'windows-registry';
2116

2217
export class WindowsRegistryLocator extends Locator<BasicEnvInfo> {
23-
public readonly providerId: string = 'windows-registry';
18+
public readonly providerId: string = WINDOWS_REG_PROVIDER_ID;
2419

2520
// eslint-disable-next-line class-methods-use-this
2621
public iterEnvs(
27-
_?: unknown,
22+
query?: PythonLocatorQuery,
2823
useWorkerThreads = inExperiment(DiscoveryUsingWorkers.experiment),
2924
): IPythonEnvsIterator<BasicEnvInfo> {
30-
const didUpdate = new EventEmitter<PythonEnvUpdatedEvent<BasicEnvInfo> | ProgressNotificationEvent>();
31-
const iterator = useWorkerThreads ? iterEnvsIterator(didUpdate) : oldIterEnvsIterator();
3225
if (useWorkerThreads) {
33-
iterator.onUpdated = didUpdate.event;
26+
/**
27+
* Windows registry is slow and often not necessary, so notify completion immediately, but use watcher
28+
* change events to signal for any new envs which are found.
29+
*/
30+
if (query?.providerId === this.providerId) {
31+
// Query via change event, so iterate all envs.
32+
return iterateEnvs(true);
33+
}
34+
return iterateEnvsLazily(this.emitter);
3435
}
35-
return iterator;
36+
return iterateEnvs(false);
3637
}
3738
}
3839

39-
/**
40-
* Windows registry is slow and often not necessary, so notify completion immediately, while still updating lazily as we find stuff.
41-
* To accomplish this, use an empty iterator while lazily firing environments using updates.
42-
*/
43-
async function* iterEnvsIterator(
44-
didUpdate: EventEmitter<PythonEnvUpdatedEvent<BasicEnvInfo> | ProgressNotificationEvent>,
45-
): IPythonEnvsIterator<BasicEnvInfo> {
46-
updateLazily(didUpdate).ignoreErrors();
40+
async function* iterateEnvsLazily(changed: IEmitter<PythonEnvsChangedEvent>): IPythonEnvsIterator<BasicEnvInfo> {
41+
loadAllEnvs(changed).ignoreErrors();
4742
}
4843

49-
async function updateLazily(didUpdate: EventEmitter<PythonEnvUpdatedEvent<BasicEnvInfo> | ProgressNotificationEvent>) {
44+
async function loadAllEnvs(changed: IEmitter<PythonEnvsChangedEvent>) {
5045
traceVerbose('Searching for windows registry interpreters');
51-
const interpreters = await getRegistryInterpreters(true);
52-
for (const interpreter of interpreters) {
53-
try {
54-
// Filter out Microsoft Store app directories. We have a store app locator that handles this.
55-
// The python.exe available in these directories might not be python. It can be a store install
56-
// shortcut that takes you to microsoft store.
57-
if (isMicrosoftStoreDir(interpreter.interpreterPath)) {
58-
continue;
59-
}
60-
const env: BasicEnvInfo = {
61-
kind: PythonEnvKind.OtherGlobal,
62-
executablePath: interpreter.interpreterPath,
63-
source: [PythonEnvSource.WindowsRegistry],
64-
};
65-
didUpdate.fire({ update: env });
66-
} catch (ex) {
67-
traceError(`Failed to process environment: ${interpreter}`, ex);
68-
}
69-
}
70-
didUpdate.fire({ stage: ProgressReportStage.discoveryFinished });
46+
await getRegistryInterpreters(true);
47+
changed.fire({ providerId: WINDOWS_REG_PROVIDER_ID });
7148
traceVerbose('Finished searching for windows registry interpreters');
7249
}
7350

74-
async function* oldIterEnvsIterator(): IPythonEnvsIterator<BasicEnvInfo> {
75-
const interpreters = await getRegistryInterpreters(false);
51+
async function* iterateEnvs(useWorkerThreads: boolean): IPythonEnvsIterator<BasicEnvInfo> {
52+
const interpreters = await getRegistryInterpreters(useWorkerThreads);
7653
for (const interpreter of interpreters) {
7754
try {
7855
// Filter out Microsoft Store app directories. We have a store app locator that handles this.

‎src/test/pythonEnvironments/base/locators/lowLevel/windowsRegistryLocator.testvirtualenvs.ts

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,10 @@
22
// Licensed under the MIT License.
33

44
import { getEnvs } from '../../../../../client/pythonEnvironments/base/locatorUtils';
5-
import { WindowsRegistryLocator } from '../../../../../client/pythonEnvironments/base/locators/lowLevel/windowsRegistryLocator';
5+
import {
6+
WindowsRegistryLocator,
7+
WINDOWS_REG_PROVIDER_ID,
8+
} from '../../../../../client/pythonEnvironments/base/locators/lowLevel/windowsRegistryLocator';
69
import { assertBasicEnvsEqual } from '../envTestUtils';
710
import { TEST_TIMEOUT } from '../../../../constants';
811
import { getOSType, OSType } from '../../../../../client/common/utils/platform';
@@ -19,8 +22,8 @@ suite('Windows Registry Locator', async () => {
1922
});
2023

2124
test('Worker thread to fetch registry interpreters is working', async () => {
22-
const items = await getEnvs(locator.iterEnvs(undefined, false));
23-
const workerItems = await getEnvs(locator.iterEnvs(undefined, true));
25+
const items = await getEnvs(locator.iterEnvs({ providerId: WINDOWS_REG_PROVIDER_ID }, false));
26+
const workerItems = await getEnvs(locator.iterEnvs({ providerId: WINDOWS_REG_PROVIDER_ID }, true));
2427
console.log('Number of items Windows registry locator returned:', items.length);
2528
// Make sure items returned when using worker threads v/s not are the same.
2629
assertBasicEnvsEqual(items, workerItems);

‎src/test/pythonEnvironments/base/locators/lowLevel/windowsRegistryLocator.unit.test.ts

Lines changed: 20 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -4,10 +4,14 @@
44
import * as assert from 'assert';
55
import * as path from 'path';
66
import * as sinon from 'sinon';
7+
import { expect } from 'chai';
78
import { PythonEnvKind, PythonEnvSource } from '../../../../../client/pythonEnvironments/base/info';
89
import { getEnvs } from '../../../../../client/pythonEnvironments/base/locatorUtils';
910
import * as winreg from '../../../../../client/pythonEnvironments/common/windowsRegistry';
10-
import { WindowsRegistryLocator } from '../../../../../client/pythonEnvironments/base/locators/lowLevel/windowsRegistryLocator';
11+
import {
12+
WindowsRegistryLocator,
13+
WINDOWS_REG_PROVIDER_ID,
14+
} from '../../../../../client/pythonEnvironments/base/locators/lowLevel/windowsRegistryLocator';
1115
import { createBasicEnv } from '../../common';
1216
import { TEST_LAYOUT_ROOT } from '../../../common/commonTestConstants';
1317
import { assertBasicEnvsEqual } from '../envTestUtils';
@@ -201,7 +205,7 @@ suite('Windows Registry', () => {
201205
}
202206

203207
setup(async () => {
204-
sinon.stub(externalDependencies, 'inExperiment').returns(false);
208+
sinon.stub(externalDependencies, 'inExperiment').returns(true);
205209
stubReadRegistryValues = sinon.stub(winreg, 'readRegistryValues');
206210
stubReadRegistryKeys = sinon.stub(winreg, 'readRegistryKeys');
207211
stubReadRegistryValues.callsFake(fakeRegistryValues);
@@ -222,18 +226,29 @@ suite('Windows Registry', () => {
222226
createBasicEnv(PythonEnvKind.OtherGlobal, path.join(regTestRoot, 'python38', 'python.exe')),
223227
].map((e) => ({ ...e, source: [PythonEnvSource.WindowsRegistry] }));
224228

225-
const iterator = locator.iterEnvs(undefined, true);
229+
const lazyIterator = locator.iterEnvs(undefined, true);
230+
const envs = await getEnvs(lazyIterator);
231+
expect(envs.length).to.equal(0);
232+
233+
const iterator = locator.iterEnvs({ providerId: WINDOWS_REG_PROVIDER_ID }, true);
226234
const actualEnvs = await getEnvs(iterator);
227235

228236
assertBasicEnvsEqual(actualEnvs, expectedEnvs);
229237
});
230238

239+
test('iterEnvs(): query is undefined', async () => {
240+
// Iterate no envs when query is `undefined`, i.e notify completion immediately.
241+
const lazyIterator = locator.iterEnvs(undefined, true);
242+
const envs = await getEnvs(lazyIterator);
243+
expect(envs.length).to.equal(0);
244+
});
245+
231246
test('iterEnvs(): no registry permission', async () => {
232247
stubReadRegistryKeys.callsFake(() => {
233248
throw Error();
234249
});
235250

236-
const iterator = locator.iterEnvs(undefined, true);
251+
const iterator = locator.iterEnvs({ providerId: WINDOWS_REG_PROVIDER_ID }, true);
237252
const actualEnvs = await getEnvs(iterator);
238253

239254
assert.deepStrictEqual(actualEnvs, []);
@@ -252,7 +267,7 @@ suite('Windows Registry', () => {
252267
createBasicEnv(PythonEnvKind.OtherGlobal, path.join(regTestRoot, 'python38', 'python.exe')),
253268
].map((e) => ({ ...e, source: [PythonEnvSource.WindowsRegistry] }));
254269

255-
const iterator = locator.iterEnvs(undefined, true);
270+
const iterator = locator.iterEnvs({ providerId: WINDOWS_REG_PROVIDER_ID }, true);
256271
const actualEnvs = await getEnvs(iterator);
257272

258273
assertBasicEnvsEqual(actualEnvs, expectedEnvs);

0 commit comments

Comments
 (0)