Kartik Raj (karrtikr) · Nov 6, 2020 · Nov 4, 2020 · Nov 4, 2020 · Nov 4, 2020 · Nov 4, 2020
diff --git a/src/client/common/platform/fileSystemWatcher.ts b/src/client/common/platform/fileSystemWatcher.ts
@@ -5,7 +5,7 @@
 
 import * as chokidar from 'chokidar';
 import * as path from 'path';
-import { RelativePattern, workspace } from 'vscode';
+import { Disposable, RelativePattern, workspace } from 'vscode';
 import { traceError, traceVerbose, traceWarning } from '../logger';
 import { normCasePath } from './fs-paths';
 
@@ -23,15 +23,14 @@ export function watchLocationForPattern(
     baseDir: string,
     pattern: string,
     callback: (type: FileChangeType, absPath: string) => void
-): void {
+): Disposable {
     // Use VSCode API iff base directory to exists within the current workspace folders
     const found = workspace.workspaceFolders?.find((e) => normCasePath(baseDir).startsWith(normCasePath(e.uri.fsPath)));
     if (found) {
-        watchLocationUsingVSCodeAPI(baseDir, pattern, callback);
-    } else {
-        // Fallback to chokidar as base directory to lookup doesn't exist within the current workspace folders
-        watchLocationUsingChokidar(baseDir, pattern, callback);
+        return watchLocationUsingVSCodeAPI(baseDir, pattern, callback);
     }
+    // Fallback to chokidar as base directory to lookup doesn't exist within the current workspace folders
+    return watchLocationUsingChokidar(baseDir, pattern, callback);
 }
 
 function watchLocationUsingVSCodeAPI(
@@ -40,11 +39,17 @@ function watchLocationUsingVSCodeAPI(
     callback: (type: FileChangeType, absPath: string) => void
 ) {
     const globPattern = new RelativePattern(baseDir, pattern);
+    const disposables: Disposable[] = [];
     traceVerbose(`Start watching: ${baseDir} with pattern ${pattern} using VSCode API`);
     const watcher = workspace.createFileSystemWatcher(globPattern);
-    watcher.onDidCreate((e) => callback(FileChangeType.Created, e.fsPath));
-    watcher.onDidChange((e) => callback(FileChangeType.Changed, e.fsPath));
-    watcher.onDidDelete((e) => callback(FileChangeType.Deleted, e.fsPath));
+    disposables.push(watcher.onDidCreate((e) => callback(FileChangeType.Created, e.fsPath)));
+    disposables.push(watcher.onDidChange((e) => callback(FileChangeType.Changed, e.fsPath)));
+    disposables.push(watcher.onDidDelete((e) => callback(FileChangeType.Deleted, e.fsPath)));
+    return {
+        dispose: async () => {
+            disposables.forEach((d) => d.dispose());
+        }
+    };
 }
 
 function watchLocationUsingChokidar(
@@ -69,7 +74,9 @@ function watchLocationUsingChokidar(
             '**/.hg/store/**',
             '/dev/**',
             '/proc/**',
-            '/sys/**'
+            '/sys/**',
+            '**/lib/**',
+            '**/includes/**'
         ], // https://github.com/microsoft/vscode/issues/23954
         followSymlinks: false
     };
@@ -96,6 +103,13 @@ function watchLocationUsingChokidar(
         callback(eventType, absPath);
     });
 
+    const dispose = async () => {
+        if (watcher) {
+            await watcher.close();
+            watcher = null;
+        }
+    };
+
     watcher.on('error', async (error: NodeJS.ErrnoException) => {
         if (error) {
             // Specially handle ENOSPC errors that can happen when
@@ -105,13 +119,12 @@ function watchLocationUsingChokidar(
             // See https://github.com/Microsoft/vscode/issues/7950
             if (error.code === 'ENOSPC') {
                 traceError(`Inotify limit reached (ENOSPC) for ${baseDir} with pattern ${pattern}`);
-                if (watcher) {
-                    await watcher.close();
-                    watcher = null;
-                }
+                await dispose();
             } else {
                 traceWarning(error.toString());
             }
         }
     });
+
+    return { dispose };
 }
diff --git a/src/client/pythonEnvironments/base/locator.ts b/src/client/pythonEnvironments/base/locator.ts
@@ -1,7 +1,10 @@
 // Copyright (c) Microsoft Corporation. All rights reserved.
 // Licensed under the MIT License.
 
+// eslint-disable-next-line max-classes-per-file
 import { Event, Uri } from 'vscode';
+import { AsyncDisposableRegistry } from '../../common/asyncDisposableRegistry';
+import { IAsyncDisposableRegistry, IDisposable } from '../../common/types';
 import { iterEmpty } from '../../common/utils/async';
 import { PythonEnvInfo, PythonEnvKind } from './info';
 import {
@@ -178,9 +181,14 @@ interface IEmitter<E extends BasicPythonEnvsChangedEvent> {
  * should be used.  Only in low-level cases should you consider using
  * `BasicPythonEnvsChangedEvent`.
  */
-export abstract class LocatorBase<E extends BasicPythonEnvsChangedEvent = PythonEnvsChangedEvent> implements ILocator<E> {
+export abstract class LocatorBase<E extends BasicPythonEnvsChangedEvent = PythonEnvsChangedEvent>
+implements IDisposable, ILocator<E> {
     public readonly onChanged: Event<E>;
+
     protected readonly emitter: IEmitter<E>;
+
+    protected readonly disposables: IAsyncDisposableRegistry = new AsyncDisposableRegistry();
+
     constructor(watcher: IPythonEnvsWatcher<E> & IEmitter<E>) {
         this.emitter = watcher;
         this.onChanged = watcher.onChanged;
@@ -191,6 +199,10 @@ export abstract class LocatorBase<E extends BasicPythonEnvsChangedEvent = Python
     public async resolveEnv(_env: string | PythonEnvInfo): Promise<PythonEnvInfo | undefined> {
         return undefined;
     }
+
+    public dispose(): void {
+        this.disposables.dispose().ignoreErrors();
+    }
 }
 
 /**

diff --git a/src/client/pythonEnvironments/base/locators/lowLevel/locator.ts b/src/client/pythonEnvironments/base/locators/lowLevel/locator.ts
@@ -0,0 +1,14 @@
+// Copyright (c) Microsoft Corporation. All rights reserved.
+// Licensed under the MIT License.
+
+import { Locator } from '../../locator';
+
+/**
+ * The base for Python envs locators who watch the file system.
+ * Most low-level locators should be using this.
+ *
+ * Subclasses can call `this.emitter.fire()` * to emit events.
+ */
+export abstract class FSWatchingLocator extends Locator {
+    public async abstract initialize(): Promise<void>;
+}
diff --git a/src/client/pythonEnvironments/common/pythonBinariesWatcher.ts b/src/client/pythonEnvironments/common/pythonBinariesWatcher.ts
@@ -5,26 +5,38 @@
 
 import * as minimatch from 'minimatch';
 import * as path from 'path';
+import { Disposable } from 'vscode';
 import { FileChangeType, watchLocationForPattern } from '../../common/platform/fileSystemWatcher';
 import { getOSType, OSType } from '../../common/utils/platform';
 
 const [executable, binName] = getOSType() === OSType.Windows ? ['python.exe', 'Scripts'] : ['python', 'bin'];
 
+/**
+ * @param baseDir The base directory from which watch paths are to be derived.
+ * @param callback The listener function will be called when the event happens.
+ * @param executableSuffixGlob Glob which represents suffix of the full executable file path to watch.
+ */
 export function watchLocationForPythonBinaries(
     baseDir: string,
     callback: (type: FileChangeType, absPath: string) => void,
-    executableGlob: string = executable,
-): void {
-    const patterns = [executableGlob, `*/${executableGlob}`, `*/${binName}/${executableGlob}`];
+    executableSuffixGlob: string = executable,
+): Disposable {
+    const patterns = [executableSuffixGlob, `*/${executableSuffixGlob}`, `*/${binName}/${executableSuffixGlob}`];
+    const disposables: Disposable[] = [];
     for (const pattern of patterns) {
-        watchLocationForPattern(baseDir, pattern, (type: FileChangeType, e: string) => {
-            const isMatch = minimatch(e, path.join('**', executableGlob), { nocase: getOSType() === OSType.Windows });
+        disposables.push(watchLocationForPattern(baseDir, pattern, (type: FileChangeType, e: string) => {
+            const isMatch = minimatch(e, path.join('**', executableSuffixGlob), { nocase: getOSType() === OSType.Windows });
             if (!isMatch) {
                 // When deleting the file for some reason path to all directories leading up to python are reported
                 // Skip those events
                 return;
             }
             callback(type, e);
-        });
+        }));
     }
+    return {
+        dispose: async () => {
+            disposables.forEach((d) => d.dispose());
+        },
+    };
 }
diff --git a/src/client/pythonEnvironments/discovery/locators/services/globalVirtualEnvronmentLocator.ts b/src/client/pythonEnvironments/discovery/locators/services/globalVirtualEnvronmentLocator.ts
@@ -7,19 +7,20 @@ import { traceVerbose } from '../../../../common/logger';
 import { FileChangeType } from '../../../../common/platform/fileSystemWatcher';
 import { chain, iterable, sleep } from '../../../../common/utils/async';
 import {
-    getEnvironmentVariable, getOSType, getUserHomeDir, OSType
+    getEnvironmentVariable, getOSType, getUserHomeDir, OSType,
 } from '../../../../common/utils/platform';
 import { PythonEnvInfo, PythonEnvKind, UNKNOWN_PYTHON_VERSION } from '../../../base/info';
 import { buildEnvInfo } from '../../../base/info/env';
-import { IPythonEnvsIterator, Locator } from '../../../base/locator';
+import { IPythonEnvsIterator } from '../../../base/locator';
+import { FSWatchingLocator } from '../../../base/locators/lowLevel/locator';
 import { findInterpretersInDir } from '../../../common/commonUtils';
 import { getFileInfo, pathExists } from '../../../common/externalDependencies';
 import { watchLocationForPythonBinaries } from '../../../common/pythonBinariesWatcher';
 import { isPipenvEnvironment } from './pipEnvHelper';
 import {
     isVenvEnvironment,
     isVirtualenvEnvironment,
-    isVirtualenvwrapperEnvironment
+    isVirtualenvwrapperEnvironment,
 } from './virtualEnvironmentIdentifier';
 
 const DEFAULT_SEARCH_DEPTH = 2;
@@ -80,7 +81,7 @@ async function getVirtualEnvKind(interpreterPath: string): Promise<PythonEnvKind
 /**
  * Finds and resolves virtual environments created in known global locations.
  */
-export class GlobalVirtualEnvironmentLocator extends Locator {
+export class GlobalVirtualEnvironmentLocator extends FSWatchingLocator {
     private virtualEnvKinds = [
         PythonEnvKind.Venv,
         PythonEnvKind.VirtualEnv,
@@ -90,7 +91,10 @@ export class GlobalVirtualEnvironmentLocator extends Locator {
 
     public constructor(private readonly searchDepth?: number) {
         super();
-        this.registerWatchers().ignoreErrors();
+    }
+
+    public async initialize(): Promise<void> {
+        await this.startWatchers();
     }
 
     public iterEnvs(): IPythonEnvsIterator {
@@ -168,15 +172,20 @@ export class GlobalVirtualEnvironmentLocator extends Locator {
         return undefined;
     }
 
-    private async registerWatchers(): Promise<void> {
+    private async startWatchers(): Promise<void> {
         const dirs = await getGlobalVirtualEnvDirs();
-        dirs.forEach((d) => watchLocationForPythonBinaries(d, async (type: FileChangeType, executablePath: string) => {
-            // Note detecting kind of virtual env depends on the file structure around the executable, so we need to
-            // wait before attempting to detect it. However even if the type detected is incorrect, it doesn't do any
-            // practical harm as kinds in this locator are used in the same way (same activation commands etc.)
-            await sleep(1000);
-            const kind = await getVirtualEnvKind(executablePath);
-            this.emitter.fire({ type, kind });
-        }));
+        dirs.forEach(
+            (d) => this.disposables.push(
+                watchLocationForPythonBinaries(d, async (type: FileChangeType, executablePath: string) => {
+                    // Note detecting kind of virtual env depends on the file structure around the
+                    // executable, so we need to wait before attempting to detect it. However even
+                    // if the type detected is incorrect, it doesn't do any practical harm as kinds
+                    // in this locator are used in the same way (same activation commands etc.)
+                    await sleep(1000);
+                    const kind = await getVirtualEnvKind(executablePath);
+                    this.emitter.fire({ type, kind });
+                }),
+            ),
+        );
     }
 }
diff --git a/src/client/pythonEnvironments/discovery/locators/services/windowsStoreLocator.ts b/src/client/pythonEnvironments/discovery/locators/services/windowsStoreLocator.ts
@@ -10,7 +10,8 @@ import { Architecture, getEnvironmentVariable } from '../../../../common/utils/p
 import { PythonEnvInfo, PythonEnvKind } from '../../../base/info';
 import { buildEnvInfo } from '../../../base/info/env';
 import { getPythonVersionFromPath } from '../../../base/info/pythonVersion';
-import { IPythonEnvsIterator, Locator } from '../../../base/locator';
+import { IPythonEnvsIterator } from '../../../base/locator';
+import { FSWatchingLocator } from '../../../base/locators/lowLevel/locator';
 import { getFileInfo } from '../../../common/externalDependencies';
 import { watchLocationForPythonBinaries } from '../../../common/pythonBinariesWatcher';
 
@@ -92,14 +93,16 @@ export async function isWindowsStoreEnvironment(interpreterPath: string): Promis
  * This is a glob pattern which matches following file names:
  * python3.8.exe
  * python3.9.exe
+ * python3.10.exe
  * This pattern does not match:
  * python.exe
  * python2.7.exe
  * python3.exe
  * python38.exe
- * 'python.exe', 'python3.exe', and 'python3.8.exe' can point to the same executable, hence only capture python3.*.exes.
+ * Note chokidar fails to match multiple digits using +([0-9]), even though the underlying glob pattern matcher
+ * they use (picomatch), or any other glob matcher does. Hence why we had to use {[0-9],[0-9][0-9]} instead.
  */
-const pythonExeGlob = 'python3\.[0-9]*\.exe';
+const pythonExeGlob = 'python3\.{[0-9],[0-9][0-9]}\.exe';
 
 /**
  * Checks if a given path ends with python3.*.exe. Not all python executables are matched as
@@ -137,10 +140,10 @@ export async function getWindowsStorePythonExes(): Promise<string[]> {
         .filter(isWindowsStorePythonExe);
 }
 
-export class WindowsStoreLocator extends Locator {
+export class WindowsStoreLocator extends FSWatchingLocator {
     private readonly kind: PythonEnvKind = PythonEnvKind.WindowsStore;
 
-    public initialize(): void {
+    public async initialize(): Promise<void> {
         this.startWatcher();
     }
 
@@ -176,12 +179,14 @@ export class WindowsStoreLocator extends Locator {
 
     private startWatcher(): void {
         const windowsAppsRoot = getWindowsStoreAppsRoot();
-        watchLocationForPythonBinaries(
-            windowsAppsRoot,
-            (type: FileChangeType) => {
-                this.emitter.fire({ type, kind: this.kind });
-            },
-            pythonExeGlob,
+        this.disposables.push(
+            watchLocationForPythonBinaries(
+                windowsAppsRoot,
+                (type: FileChangeType) => {
+                    this.emitter.fire({ type, kind: this.kind });
+                },
+                pythonExeGlob,
+            ),
         );
     }
 }
diff --git a/src/test/pythonEnvironments/discovery/locators/windowsStoreLocator.test.ts b/src/test/pythonEnvironments/discovery/locators/windowsStoreLocator.test.ts
@@ -83,7 +83,7 @@ suite('Windows Store Locator', async () => {
 
     async function setupLocator(onChanged: (e: PythonEnvsChangedEvent) => Promise<void>) {
         locator = new WindowsStoreLocator();
-        locator.initialize();
+        await locator.initialize();
         // Wait for watchers to get ready
         await sleep(1000);
         locator.onChanged(onChanged);