Cherry pic fixes into release for tests. (#14673)

karthiknadig · Kartik Raj · web-flow · commit 5eab81dd3669 · 2020-11-09T13:40:19.000-08:00
* Added FSWatching base class and made related changes (#14605) * Add FSWatching locator base class * Correct glob pattern to not match python3.2whoa * Add documentation of python binary watcher * Fix lint errors * Update ignore list * Add disposable registry * Modify FSWatching Locator * Code reviews * Use string[] * Remove list disposable getter * Fix failing global virtual env watcher tests (#14633) Co-authored-by: Kartik Raj <karraj@microsoft.com>
diff --git a/src/client/common/platform/fileSystemWatcher.ts b/src/client/common/platform/fileSystemWatcher.ts
@@ -7,6 +7,8 @@ import * as chokidar from 'chokidar';
 import * as path from 'path';
 import { RelativePattern, workspace } from 'vscode';
 import { traceError, traceVerbose, traceWarning } from '../logger';
+import { DisposableRegistry } from '../syncDisposableRegistry';
+import { IDisposable } from '../types';
 import { normCasePath } from './fs-paths';
 
 /**
@@ -23,35 +25,36 @@ export function watchLocationForPattern(
     baseDir: string,
     pattern: string,
     callback: (type: FileChangeType, absPath: string) => void
-): void {
+): IDisposable {
     // 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(
     baseDir: string,
     pattern: string,
     callback: (type: FileChangeType, absPath: string) => void
-) {
+): IDisposable {
     const globPattern = new RelativePattern(baseDir, pattern);
+    const disposables = new DisposableRegistry();
     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 disposables;
 }
 
 function watchLocationUsingChokidar(
     baseDir: string,
     pattern: string,
     callback: (type: FileChangeType, absPath: string) => void
-) {
+): IDisposable {
     const watcherOpts: chokidar.WatchOptions = {
         cwd: baseDir,
         ignoreInitial: true,
@@ -69,7 +72,9 @@ function watchLocationUsingChokidar(
             '**/.hg/store/**',
             '/dev/**',
             '/proc/**',
-            '/sys/**'
+            '/sys/**',
+            '**/lib/**',
+            '**/includes/**'
         ], // https://github.com/microsoft/vscode/issues/23954
         followSymlinks: false
     };
@@ -96,6 +101,18 @@ function watchLocationUsingChokidar(
         callback(eventType, absPath);
     });
 
+    const stopWatcher = async () => {
+        if (watcher) {
+            const obj = watcher;
+            watcher = null;
+            try {
+                await obj.close();
+            } catch (err) {
+                traceError(`Failed to close FS watcher (${err})`);
+            }
+        }
+    };
+
     watcher.on('error', async (error: NodeJS.ErrnoException) => {
         if (error) {
             // Specially handle ENOSPC errors that can happen when
@@ -105,13 +122,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 stopWatcher();
             } else {
                 traceWarning(error.toString());
             }
         }
     });
+
+    return { dispose: () => stopWatcher().ignoreErrors() };
 }
diff --git a/src/client/common/syncDisposableRegistry.ts b/src/client/common/syncDisposableRegistry.ts
@@ -0,0 +1,31 @@
+// Copyright (c) Microsoft Corporation. All rights reserved.
+// Licensed under the MIT License.
+
+'use strict';
+
+import { traceWarning } from './logger';
+import { IDisposable } from './types';
+
+/**
+ * Responsible for disposing a list of disposables synchronusly.
+ */
+export class DisposableRegistry implements IDisposable {
+    private _list: IDisposable[] = [];
+
+    public dispose(): void {
+        this._list.forEach((l, index) => {
+            try {
+                l.dispose();
+            } catch (err) {
+                traceWarning(`dispose() #${index} failed (${err})`);
+            }
+        });
+        this._list = [];
+    }
+
+    public push(disposable?: IDisposable): void {
+        if (disposable) {
+            this._list.push(disposable);
+        }
+    }
+}
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 { DisposableRegistry } from '../../common/syncDisposableRegistry';
+import { IDisposable } from '../../common/types';
 import { iterEmpty } from '../../common/utils/async';
 import { PythonEnvInfo, PythonEnvKind } from './info';
 import {
@@ -178,19 +181,28 @@ 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> {
+abstract class LocatorBase<E extends BasicPythonEnvsChangedEvent = PythonEnvsChangedEvent>
+implements IDisposable, ILocator<E> {
     public readonly onChanged: Event<E>;
-    protected readonly emitter: IEmitter<E>;
+
+    protected readonly emitter: IPythonEnvsWatcher<E> & IEmitter<E>;
+
+    protected readonly disposables = new DisposableRegistry();
+
     constructor(watcher: IPythonEnvsWatcher<E> & IEmitter<E>) {
         this.emitter = watcher;
-        this.onChanged = watcher.onChanged;
+        this.onChanged = this.emitter.onChanged;
     }
 
     public abstract iterEnvs(query?: QueryForEvent<E>): IPythonEnvsIterator;
 
     public async resolveEnv(_env: string | PythonEnvInfo): Promise<PythonEnvInfo | undefined> {
         return undefined;
     }
+
+    public dispose(): void {
+        this.disposables.dispose();
+    }
 }
 
 /**
diff --git a/src/client/pythonEnvironments/base/locators/lowLevel/fsWatchingLocator.ts b/src/client/pythonEnvironments/base/locators/lowLevel/fsWatchingLocator.ts
@@ -0,0 +1,82 @@
+// Copyright (c) Microsoft Corporation. All rights reserved.
+// Licensed under the MIT License.
+
+import { FileChangeType } from '../../../../common/platform/fileSystemWatcher';
+import { sleep } from '../../../../common/utils/async';
+import { watchLocationForPythonBinaries } from '../../../common/pythonBinariesWatcher';
+import { PythonEnvKind } from '../../info';
+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 {
+    private initialized = false;
+
+    constructor(
+        /**
+         * Location(s) to watch for python binaries.
+         */
+        private readonly getRoots: () => Promise<string[]> | string | string[],
+        /**
+         * Returns the kind of environment specific to locator given the path to exectuable.
+         */
+        private readonly getKind: (executable: string) => Promise<PythonEnvKind>,
+        private readonly opts: {
+            /**
+             * Glob which represents basename of the executable to watch.
+             */
+            executableBaseGlob?: string,
+            /**
+             * Time to wait before handling an environment-created event.
+             */
+            delayOnCreated?: number, // milliseconds
+        } = {},
+    ) {
+        super();
+    }
+
+    public async initialize(): Promise<void> {
+        if (this.initialized) {
+            return;
+        }
+        this.initialized = true;
+        await this.startWatchers();
+    }
+
+    public dispose(): void {
+        super.dispose();
+        this.initialized = false;
+    }
+
+    private async startWatchers(): Promise<void> {
+        let roots = await this.getRoots();
+        if (typeof roots === 'string') {
+            roots = [roots];
+        }
+        roots.forEach((root) => this.startWatcher(root));
+    }
+
+    private startWatcher(root: string): void {
+        this.disposables.push(
+            watchLocationForPythonBinaries(
+                root,
+                async (type: FileChangeType, executable: string) => {
+                    if (type === FileChangeType.Created) {
+                        if (this.opts.delayOnCreated !== undefined) {
+                            // Note detecting kind of env depends on the file structure around the
+                            // executable, so we need to wait before attempting to detect it.
+                            await sleep(this.opts.delayOnCreated);
+                        }
+                    }
+                    const kind = await this.getKind(executable);
+                    this.emitter.fire({ type, kind });
+                },
+                this.opts.executableBaseGlob,
+            ),
+        );
+    }
+}
diff --git a/src/client/pythonEnvironments/common/pythonBinariesWatcher.ts b/src/client/pythonEnvironments/common/pythonBinariesWatcher.ts
@@ -6,25 +6,37 @@
 import * as minimatch from 'minimatch';
 import * as path from 'path';
 import { FileChangeType, watchLocationForPattern } from '../../common/platform/fileSystemWatcher';
+import { DisposableRegistry } from '../../common/syncDisposableRegistry';
+import { IDisposable } from '../../common/types';
 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 executableBaseGlob Glob which represents basename of the executable to watch.
+ */
 export function watchLocationForPythonBinaries(
     baseDir: string,
     callback: (type: FileChangeType, absPath: string) => void,
-    executableGlob: string = executable,
-): void {
-    const patterns = [executableGlob, `*/${executableGlob}`, `*/${binName}/${executableGlob}`];
+    executableBaseGlob: string = executable,
+): IDisposable {
+    if (executableBaseGlob.includes(path.sep)) {
+        throw new Error('Glob basename contains invalid characters');
+    }
+    const patterns = [executableBaseGlob, `*/${executableBaseGlob}`, `*/${binName}/${executableBaseGlob}`];
+    const disposables = new DisposableRegistry();
     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(path.basename(e), executableBaseGlob, { 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 disposables;
 }
diff --git a/src/client/pythonEnvironments/discovery/locators/services/globalVirtualEnvronmentLocator.ts b/src/client/pythonEnvironments/discovery/locators/services/globalVirtualEnvronmentLocator.ts
@@ -4,17 +4,16 @@
 import { uniq } from 'lodash';
 import * as path from 'path';
 import { traceVerbose } from '../../../../common/logger';
-import { FileChangeType } from '../../../../common/platform/fileSystemWatcher';
-import { chain, iterable, sleep } from '../../../../common/utils/async';
+import { chain, iterable } from '../../../../common/utils/async';
 import {
     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/fsWatchingLocator';
 import { findInterpretersInDir } from '../../../common/commonUtils';
 import { getFileInfo, pathExists } from '../../../common/externalDependencies';
-import { watchLocationForPythonBinaries } from '../../../common/pythonBinariesWatcher';
 import { isPipenvEnvironment } from './pipEnvHelper';
 import {
     isVenvEnvironment,
@@ -80,17 +79,22 @@ 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,
         PythonEnvKind.VirtualEnvWrapper,
         PythonEnvKind.Pipenv,
     ];
 
-    public constructor(private readonly searchDepth?: number) {
-        super();
-        this.registerWatchers().ignoreErrors();
+    constructor(private readonly searchDepth?: number) {
+        super(getGlobalVirtualEnvDirs, getVirtualEnvKind, {
+            // 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.)
+            delayOnCreated: 1000,
+        });
     }
 
     public iterEnvs(): IPythonEnvsIterator {
@@ -167,16 +171,4 @@ export class GlobalVirtualEnvironmentLocator extends Locator {
         }
         return undefined;
     }
-
-    private async registerWatchers(): 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 });
-        }));
-    }
 }
diff --git a/src/client/pythonEnvironments/discovery/locators/services/windowsStoreLocator.ts b/src/client/pythonEnvironments/discovery/locators/services/windowsStoreLocator.ts
diff --git a/src/test/pythonEnvironments/discovery/locators/globalVirtualEnvironmentLocator.testvirtualenvs.ts b/src/test/pythonEnvironments/discovery/locators/globalVirtualEnvironmentLocator.testvirtualenvs.ts
diff --git a/src/test/pythonEnvironments/discovery/locators/windowsStoreLocator.test.ts b/src/test/pythonEnvironments/discovery/locators/windowsStoreLocator.test.ts
