Add disposable registry

Author: Kartik Raj

src/client/common/platform/fileSystemWatcher.ts

@@ -5,8 +5,10 @@
 
 import * as chokidar from 'chokidar';
 import * as path from 'path';
-import { Disposable, RelativePattern, workspace } from 'vscode';
+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,7 +25,7 @@ export function watchLocationForPattern(
     baseDir: string,
     pattern: string,
     callback: (type: FileChangeType, absPath: string) => void
-): Disposable {
+): 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) {
@@ -37,26 +39,22 @@ function watchLocationUsingVSCodeAPI(
     baseDir: string,
     pattern: string,
     callback: (type: FileChangeType, absPath: string) => void
-) {
+): IDisposable {
     const globPattern = new RelativePattern(baseDir, pattern);
-    const disposables: Disposable[] = [];
+    const disposables = new DisposableRegistry();
     traceVerbose(`Start watching: ${baseDir} with pattern ${pattern} using VSCode API`);
     const watcher = workspace.createFileSystemWatcher(globPattern);
     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());
-        }
-    };
+    return disposables;
 }
 
 function watchLocationUsingChokidar(
     baseDir: string,
     pattern: string,
     callback: (type: FileChangeType, absPath: string) => void
-) {
+): IDisposable {
     const watcherOpts: chokidar.WatchOptions = {
         cwd: baseDir,
         ignoreInitial: true,
@@ -103,10 +101,15 @@ function watchLocationUsingChokidar(
         callback(eventType, absPath);
     });
 
-    const dispose = async () => {
+    const stopWatcher = async () => {
         if (watcher) {
-            await watcher.close();
+            const obj = watcher;
             watcher = null;
+            try {
+                await obj.close();
+            } catch (err) {
+                traceError(`Failed to close FS watcher (${err})`);
+            }
         }
     };
 
@@ -119,12 +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}`);
-                await dispose();
+                await stopWatcher();
             } else {
                 traceWarning(error.toString());
             }
         }
     });
 
-    return { dispose };
+    return { dispose: () => stopWatcher().ignoreErrors() };
 }

src/client/common/syncDisposableRegistry.ts

@@ -0,0 +1,32 @@
+// Copyright (c) Microsoft Corporation. All rights reserved.
+// Licensed under the MIT License.
+
+'use strict';
+
+import { traceWarning } from './logger';
+import { IDisposable } from './types';
+
+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);
+        }
+    }
+
+    public get list(): IDisposable[] {
+        return this._list;
+    }
+}

src/client/pythonEnvironments/base/locator.ts

@@ -3,8 +3,8 @@
 
 // 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 { DisposableRegistry } from '../../common/syncDisposableRegistry';
+import { IDisposable } from '../../common/types';
 import { iterEmpty } from '../../common/utils/async';
 import { PythonEnvInfo, PythonEnvKind } from './info';
 import {
@@ -170,28 +170,32 @@ interface IEmitter<E extends BasicPythonEnvsChangedEvent> {
 }
 
 /**
- * The generic base for Python envs locators.
+ * The base for most Python envs locators.
  *
  * By default `resolveEnv()` returns undefined.  Subclasses may override
  * the method to provide an implementation.
  *
- * Subclasses will call `this.emitter.fire()` to emit events.
+ * Subclasses will call `this.emitter.fire()` * to emit events.
+ *
+ * In most cases this is the class you will want to subclass.
+ * Only in low-level cases should you consider subclassing `LocatorBase`
+ * using `BasicPythonEnvsChangedEvent.
  *
  * Also, in most cases the default event type (`PythonEnvsChangedEvent`)
  * should be used.  Only in low-level cases should you consider using
  * `BasicPythonEnvsChangedEvent`.
  */
-export abstract class LocatorBase<E extends BasicPythonEnvsChangedEvent = PythonEnvsChangedEvent>
+export abstract class Locator<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: IAsyncDisposableRegistry = new AsyncDisposableRegistry();
+    protected readonly disposables = new DisposableRegistry();
 
-    constructor(watcher: IPythonEnvsWatcher<E> & IEmitter<E>) {
-        this.emitter = watcher;
-        this.onChanged = watcher.onChanged;
+    constructor() {
+        this.emitter = new PythonEnvsWatcher();
+        this.onChanged = this.emitter.onChanged;
     }
 
     public abstract iterEnvs(query?: QueryForEvent<E>): IPythonEnvsIterator;
@@ -201,24 +205,6 @@ implements IDisposable, ILocator<E> {
     }
 
     public dispose(): void {
-        this.disposables.dispose().ignoreErrors();
-    }
-}
-
-/**
- * The base for most Python envs locators.
- *
- * By default `resolveEnv()` returns undefined.  Subclasses may override
- * the method to provide an implementation.
- *
- * Subclasses will call `this.emitter.fire()` * to emit events.
- *
- * In most cases this is the class you will want to subclass.
- * Only in low-level cases should you consider subclassing `LocatorBase`
- * using `BasicPythonEnvsChangedEvent.
- */
-export abstract class Locator extends LocatorBase {
-    constructor() {
-        super(new PythonEnvsWatcher());
+        this.disposables.dispose();
     }
 }

src/client/pythonEnvironments/base/locators/lowLevel/locator.ts renamed to src/client/pythonEnvironments/base/locators/lowLevel/fsWatchingLocator.ts

@@ -5,27 +5,31 @@
 
 import * as minimatch from 'minimatch';
 import * as path from 'path';
-import { Disposable } from 'vscode';
 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 executableSuffixGlob Glob which represents suffix of the full executable file path to watch.
+ * @param executableBaseGlob Glob which represents basename of the executable to watch.
  */
 export function watchLocationForPythonBinaries(
     baseDir: string,
     callback: (type: FileChangeType, absPath: string) => void,
-    executableSuffixGlob: string = executable,
-): Disposable {
-    const patterns = [executableSuffixGlob, `*/${executableSuffixGlob}`, `*/${binName}/${executableSuffixGlob}`];
-    const disposables: Disposable[] = [];
+    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) {
         disposables.push(watchLocationForPattern(baseDir, pattern, (type: FileChangeType, e: string) => {
-            const isMatch = minimatch(e, path.join('**', executableSuffixGlob), { nocase: getOSType() === OSType.Windows });
+            const isMatch = minimatch(e, path.join('**', 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
@@ -34,9 +38,5 @@ export function watchLocationForPythonBinaries(
             callback(type, e);
         }));
     }
-    return {
-        dispose: async () => {
-            disposables.forEach((d) => d.dispose());
-        },
-    };
+    return disposables;
 }

src/client/pythonEnvironments/common/pythonBinariesWatcher.ts

@@ -7,20 +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 } from '../../../base/locator';
-import { FSWatchingLocator } from '../../../base/locators/lowLevel/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,
     isVirtualenvEnvironment,
-    isVirtualenvwrapperEnvironment,
+    isVirtualenvwrapperEnvironment
 } from './virtualEnvironmentIdentifier';
 
 const DEFAULT_SEARCH_DEPTH = 2;

src/client/pythonEnvironments/discovery/locators/services/globalVirtualEnvronmentLocator.ts

@@ -11,7 +11,7 @@ import { PythonEnvInfo, PythonEnvKind } from '../../../base/info';
 import { buildEnvInfo } from '../../../base/info/env';
 import { getPythonVersionFromPath } from '../../../base/info/pythonVersion';
 import { IPythonEnvsIterator } from '../../../base/locator';
-import { FSWatchingLocator } from '../../../base/locators/lowLevel/locator';
+import { FSWatchingLocator } from '../../../base/locators/lowLevel/fsWatchingLocator';
 import { getFileInfo } from '../../../common/externalDependencies';
 import { watchLocationForPythonBinaries } from '../../../common/pythonBinariesWatcher';