diff --git a/src/client/common/platform/fileSystemWatcher.ts b/src/client/common/platform/fileSystemWatcher.ts index 61016b77fcd0..596214832a29 100644 --- 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 new file mode 100644 index 000000000000..bacffe51dccd --- /dev/null +++ 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 index a1b372fb6988..0824f2130753 100644 --- 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,12 +181,17 @@ interface IEmitter { * should be used. Only in low-level cases should you consider using * `BasicPythonEnvsChangedEvent`. */ -export abstract class LocatorBase implements ILocator { +abstract class LocatorBase +implements IDisposable, ILocator { public readonly onChanged: Event; - protected readonly emitter: IEmitter; + + protected readonly emitter: IPythonEnvsWatcher & IEmitter; + + protected readonly disposables = new DisposableRegistry(); + constructor(watcher: IPythonEnvsWatcher & IEmitter) { this.emitter = watcher; - this.onChanged = watcher.onChanged; + this.onChanged = this.emitter.onChanged; } public abstract iterEnvs(query?: QueryForEvent): IPythonEnvsIterator; @@ -191,6 +199,10 @@ export abstract class LocatorBase { 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 new file mode 100644 index 000000000000..bbdb78cfdd5f --- /dev/null +++ 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[], + /** + * Returns the kind of environment specific to locator given the path to exectuable. + */ + private readonly getKind: (executable: string) => Promise, + 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 { + if (this.initialized) { + return; + } + this.initialized = true; + await this.startWatchers(); + } + + public dispose(): void { + super.dispose(); + this.initialized = false; + } + + private async startWatchers(): Promise { + 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 index 3008fb20366a..ba4c3fb5c20f 100644 --- 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 index 7662a746b67e..b7cdfbcd60cf 100644 --- 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,7 +79,7 @@ async function getVirtualEnvKind(interpreterPath: string): Promise { - 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 index a095d06e1023..f4f2697df274 100644 --- a/src/client/pythonEnvironments/discovery/locators/services/windowsStoreLocator.ts +++ b/src/client/pythonEnvironments/discovery/locators/services/windowsStoreLocator.ts @@ -5,14 +5,13 @@ import * as fsapi from 'fs-extra'; import * as minimatch from 'minimatch'; import * as path from 'path'; import { traceWarning } from '../../../../common/logger'; -import { FileChangeType } from '../../../../common/platform/fileSystemWatcher'; import { Architecture, getEnvironmentVariable } from '../../../../common/utils/platform'; 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/fsWatchingLocator'; import { getFileInfo } from '../../../common/externalDependencies'; -import { watchLocationForPythonBinaries } from '../../../common/pythonBinariesWatcher'; /** * Gets path to the Windows Apps directory. @@ -92,14 +91,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,11 +138,15 @@ export async function getWindowsStorePythonExes(): Promise { .filter(isWindowsStorePythonExe); } -export class WindowsStoreLocator extends Locator { +export class WindowsStoreLocator extends FSWatchingLocator { private readonly kind: PythonEnvKind = PythonEnvKind.WindowsStore; - public initialize(): void { - this.startWatcher(); + constructor() { + super( + getWindowsStoreAppsRoot, + async () => this.kind, + { executableBaseGlob: pythonExeGlob }, + ); } public iterEnvs(): IPythonEnvsIterator { @@ -173,15 +178,4 @@ export class WindowsStoreLocator extends Locator { } return undefined; } - - private startWatcher(): void { - const windowsAppsRoot = getWindowsStoreAppsRoot(); - watchLocationForPythonBinaries( - windowsAppsRoot, - (type: FileChangeType) => { - this.emitter.fire({ type, kind: this.kind }); - }, - pythonExeGlob, - ); - } } diff --git a/src/test/pythonEnvironments/discovery/locators/globalVirtualEnvironmentLocator.testvirtualenvs.ts b/src/test/pythonEnvironments/discovery/locators/globalVirtualEnvironmentLocator.testvirtualenvs.ts index a4650c3cf751..d6bedf364a8b 100644 --- a/src/test/pythonEnvironments/discovery/locators/globalVirtualEnvironmentLocator.testvirtualenvs.ts +++ b/src/test/pythonEnvironments/discovery/locators/globalVirtualEnvironmentLocator.testvirtualenvs.ts @@ -62,6 +62,7 @@ suite('GlobalVirtualEnvironment Locator', async () => { setup(async () => { process.env.WORKON_HOME = testWorkOnHomePath; locator = new GlobalVirtualEnvironmentLocator(); + await locator.initialize(); // Wait for watchers to get ready await sleep(1000); }); diff --git a/src/test/pythonEnvironments/discovery/locators/windowsStoreLocator.test.ts b/src/test/pythonEnvironments/discovery/locators/windowsStoreLocator.test.ts index cb71c063c981..491bcd8065a7 100644 --- 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) { locator = new WindowsStoreLocator(); - locator.initialize(); + await locator.initialize(); // Wait for watchers to get ready await sleep(1000); locator.onChanged(onChanged);