From b7e8cd307b0151b893a63abe63dca31b8735841d Mon Sep 17 00:00:00 2001 From: Karthik Nadig Date: Tue, 9 Feb 2021 23:59:10 -0800 Subject: [PATCH 1/6] Use child process apis directly. --- src/client/common/process/rawProcessApis.ts | 75 +++++++++++++++++++ .../common/externalDependencies.ts | 13 ++-- .../info/environmentInfoService.ts | 6 +- 3 files changed, 88 insertions(+), 6 deletions(-) create mode 100644 src/client/common/process/rawProcessApis.ts diff --git a/src/client/common/process/rawProcessApis.ts b/src/client/common/process/rawProcessApis.ts new file mode 100644 index 000000000000..455a6ec53cda --- /dev/null +++ b/src/client/common/process/rawProcessApis.ts @@ -0,0 +1,75 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +import { exec } from 'child_process'; +import { IDisposable } from '../types'; +import { EnvironmentVariables } from '../variables/types'; +import { DEFAULT_ENCODING } from './constants'; +import { ExecutionResult, ShellOptions, SpawnOptions } from './types'; + +export function getDefaultOptions( + options: T, + defaultEnv?: EnvironmentVariables, +): T { + const defaultOptions = { ...options }; + const execOptions = defaultOptions as SpawnOptions; + if (execOptions) { + execOptions.encoding = + typeof execOptions.encoding === 'string' && execOptions.encoding.length > 0 + ? execOptions.encoding + : DEFAULT_ENCODING; + const { encoding } = execOptions; + delete execOptions.encoding; + execOptions.encoding = encoding; + } + if (!defaultOptions.env || Object.keys(defaultOptions.env).length === 0) { + const env = defaultEnv || process.env; + defaultOptions.env = { ...env }; + } else { + defaultOptions.env = { ...defaultOptions.env }; + } + + if (execOptions && execOptions.extraVariables) { + defaultOptions.env = { ...defaultOptions.env, ...execOptions.extraVariables }; + } + + // Always ensure we have unbuffered output. + defaultOptions.env.PYTHONUNBUFFERED = '1'; + if (!defaultOptions.env.PYTHONIOENCODING) { + defaultOptions.env.PYTHONIOENCODING = 'utf-8'; + } + + return defaultOptions; +} + +export function shellExec( + command: string, + options: ShellOptions = {}, + disposables?: Set, +): Promise> { + const shellOptions = getDefaultOptions(options); + return new Promise((resolve, reject) => { + // eslint-disable-next-line @typescript-eslint/no-explicit-any + const proc = exec(command, shellOptions, (e: any, stdout: any, stderr: any) => { + if (e && e !== null) { + reject(e); + } else if (shellOptions.throwOnStdErr && stderr && stderr.length) { + reject(new Error(stderr)); + } else { + // Make sure stderr is undefined if we actually had none. This is checked + // elsewhere because that's how exec behaves. + resolve({ stderr: stderr && stderr.length > 0 ? stderr : undefined, stdout }); + } + }); // NOSONAR + const disposable: IDisposable = { + dispose: () => { + if (!proc.killed) { + proc.kill(); + } + }, + }; + if (disposables) { + disposables.add(disposable); + } + }); +} diff --git a/src/client/pythonEnvironments/common/externalDependencies.ts b/src/client/pythonEnvironments/common/externalDependencies.ts index 219cdecac503..b23bc50970b6 100644 --- a/src/client/pythonEnvironments/common/externalDependencies.ts +++ b/src/client/pythonEnvironments/common/externalDependencies.ts @@ -5,12 +5,12 @@ import * as fsapi from 'fs-extra'; import * as path from 'path'; import * as vscode from 'vscode'; import { ExecutionResult, IProcessServiceFactory, SpawnOptions } from '../../common/process/types'; -import { IExperimentService } from '../../common/types'; +import { IExperimentService, IDisposable } from '../../common/types'; import { chain, iterable } from '../../common/utils/async'; import { normalizeFilename } from '../../common/utils/filesystem'; import { getOSType, OSType } from '../../common/utils/platform'; -import { IDisposable } from '../../common/utils/resourceLifecycle'; import { IServiceContainer } from '../../ioc/types'; +import { shellExec } from '../../common/process/rawProcessApis'; let internalServiceContainer: IServiceContainer; export function initializeExternalDependencies(serviceContainer: IServiceContainer): void { @@ -23,9 +23,12 @@ function getProcessFactory(): IProcessServiceFactory { return internalServiceContainer.get(IProcessServiceFactory); } -export async function shellExecute(command: string, timeout: number): Promise> { - const proc = await getProcessFactory().create(); - return proc.shellExec(command, { timeout }); +export async function shellExecute( + command: string, + timeout: number, + disposables?: Set, +): Promise> { + return shellExec(command, { timeout }, disposables); } export async function exec(file: string, args: string[], options: SpawnOptions = {}): Promise> { diff --git a/src/client/pythonEnvironments/info/environmentInfoService.ts b/src/client/pythonEnvironments/info/environmentInfoService.ts index 09715b888b8f..04c3b126cd8c 100644 --- a/src/client/pythonEnvironments/info/environmentInfoService.ts +++ b/src/client/pythonEnvironments/info/environmentInfoService.ts @@ -1,6 +1,7 @@ // Copyright (c) Microsoft Corporation. All rights reserved. // Licensed under the MIT License. +import { traceVerbose } from '../../common/logger'; import { createDeferred, Deferred } from '../../common/utils/async'; import { createRunningWorkerPool, IWorkerPool, QueuePosition } from '../../common/utils/workerPool'; import { getInterpreterInfo, InterpreterInformation } from '../base/info/interpreter'; @@ -22,7 +23,10 @@ export interface IEnvironmentInfoService { async function buildEnvironmentInfo(interpreterPath: string): Promise { const interpreterInfo = await getInterpreterInfo(buildPythonExecInfo(interpreterPath), shellExecute).catch( - () => undefined, + (reason) => { + traceVerbose(reason); + return undefined; + }, ); if (interpreterInfo === undefined || interpreterInfo.version === undefined) { return undefined; From ef983de50ce3585791373e5c5122b887bc89679d Mon Sep 17 00:00:00 2001 From: Karthik Nadig Date: Wed, 10 Feb 2021 00:04:10 -0800 Subject: [PATCH 2/6] Use raw API in process service --- src/client/common/process/proc.ts | 57 +++---------------------------- 1 file changed, 4 insertions(+), 53 deletions(-) diff --git a/src/client/common/process/proc.ts b/src/client/common/process/proc.ts index 667eafa5d1ee..af6b86ed44f6 100644 --- a/src/client/common/process/proc.ts +++ b/src/client/common/process/proc.ts @@ -1,6 +1,6 @@ // Copyright (c) Microsoft Corporation. All rights reserved. // Licensed under the MIT License. -import { exec, execSync, spawn } from 'child_process'; +import { execSync, spawn } from 'child_process'; import { EventEmitter } from 'events'; import { Observable } from 'rxjs/Observable'; import { Readable } from 'stream'; @@ -8,7 +8,7 @@ import { Readable } from 'stream'; import { IDisposable } from '../types'; import { createDeferred } from '../utils/async'; import { EnvironmentVariables } from '../variables/types'; -import { DEFAULT_ENCODING } from './constants'; +import { getDefaultOptions, shellExec } from './rawProcessApis'; import { ExecutionResult, IBufferDecoder, @@ -201,59 +201,10 @@ export class ProcessService extends EventEmitter implements IProcessService { } public shellExec(command: string, options: ShellOptions = {}): Promise> { - const shellOptions = this.getDefaultOptions(options); - return new Promise((resolve, reject) => { - const proc = exec(command, shellOptions, (e, stdout, stderr) => { - if (e && e !== null) { - reject(e); - } else if (shellOptions.throwOnStdErr && stderr && stderr.length) { - reject(new Error(stderr)); - } else { - // Make sure stderr is undefined if we actually had none. This is checked - // elsewhere because that's how exec behaves. - resolve({ stderr: stderr && stderr.length > 0 ? stderr : undefined, stdout }); - } - }); // NOSONAR - const disposable: IDisposable = { - dispose: () => { - if (!proc.killed) { - proc.kill(); - } - }, - }; - this.processesToKill.add(disposable); - }); + return shellExec(command, options, this.processesToKill); } private getDefaultOptions(options: T): T { - const defaultOptions = { ...options }; - const execOptions = defaultOptions as SpawnOptions; - if (execOptions) { - execOptions.encoding = - typeof execOptions.encoding === 'string' && execOptions.encoding.length > 0 - ? execOptions.encoding - : DEFAULT_ENCODING; - const { encoding } = execOptions; - delete execOptions.encoding; - execOptions.encoding = encoding; - } - if (!defaultOptions.env || Object.keys(defaultOptions.env).length === 0) { - const env = this.env ? this.env : process.env; - defaultOptions.env = { ...env }; - } else { - defaultOptions.env = { ...defaultOptions.env }; - } - - if (execOptions && execOptions.extraVariables) { - defaultOptions.env = { ...defaultOptions.env, ...execOptions.extraVariables }; - } - - // Always ensure we have unbuffered output. - defaultOptions.env.PYTHONUNBUFFERED = '1'; - if (!defaultOptions.env.PYTHONIOENCODING) { - defaultOptions.env.PYTHONIOENCODING = 'utf-8'; - } - - return defaultOptions; + return getDefaultOptions(options, this.env); } } From d7f77fddeafe26c6463225c1f4e2e8ea9fbffad2 Mon Sep 17 00:00:00 2001 From: Karthik Nadig Date: Wed, 10 Feb 2021 00:23:24 -0800 Subject: [PATCH 3/6] Handle process cleanup --- .../base/info/interpreter.ts | 6 +++-- .../info/environmentInfoService.ts | 27 ++++++++++++++----- 2 files changed, 25 insertions(+), 8 deletions(-) diff --git a/src/client/pythonEnvironments/base/info/interpreter.ts b/src/client/pythonEnvironments/base/info/interpreter.ts index 8c436b6329ae..4e639fbaa4a4 100644 --- a/src/client/pythonEnvironments/base/info/interpreter.ts +++ b/src/client/pythonEnvironments/base/info/interpreter.ts @@ -6,6 +6,7 @@ import { interpreterInfo as getInterpreterInfoCommand, InterpreterInfoJson, } from '../../../common/process/internal/scripts'; +import { IDisposable } from '../../../common/types'; import { Architecture } from '../../../common/utils/platform'; import { copyPythonExecInfo, PythonExecInfo } from '../../exec'; import { parseVersion } from './pythonVersion'; @@ -45,7 +46,7 @@ type ShellExecResult = { stdout: string; stderr?: string; }; -type ShellExecFunc = (command: string, timeout: number) => Promise; +type ShellExecFunc = (command: string, timeout: number, disposables?: Set) => Promise; type Logger = { info(msg: string): void; @@ -64,6 +65,7 @@ export async function getInterpreterInfo( python: PythonExecInfo, shellExec: ShellExecFunc, logger?: Logger, + disposables?: Set, ): Promise { const [args, parse] = getInterpreterInfoCommand(); const info = copyPythonExecInfo(python, args); @@ -78,7 +80,7 @@ export async function getInterpreterInfo( // See these two bugs: // https://github.com/microsoft/vscode-python/issues/7569 // https://github.com/microsoft/vscode-python/issues/7760 - const result = await shellExec(quoted, 15000); + const result = await shellExec(quoted, 15000, disposables); if (result.stderr) { if (logger) { logger.error(`Failed to parse interpreter information for ${argv} stderr: ${result.stderr}`); diff --git a/src/client/pythonEnvironments/info/environmentInfoService.ts b/src/client/pythonEnvironments/info/environmentInfoService.ts index 04c3b126cd8c..675739245e58 100644 --- a/src/client/pythonEnvironments/info/environmentInfoService.ts +++ b/src/client/pythonEnvironments/info/environmentInfoService.ts @@ -2,6 +2,7 @@ // Licensed under the MIT License. import { traceVerbose } from '../../common/logger'; +import { IDisposable } from '../../common/types'; import { createDeferred, Deferred } from '../../common/utils/async'; import { createRunningWorkerPool, IWorkerPool, QueuePosition } from '../../common/utils/workerPool'; import { getInterpreterInfo, InterpreterInformation } from '../base/info/interpreter'; @@ -22,12 +23,26 @@ export interface IEnvironmentInfoService { } async function buildEnvironmentInfo(interpreterPath: string): Promise { - const interpreterInfo = await getInterpreterInfo(buildPythonExecInfo(interpreterPath), shellExecute).catch( - (reason) => { - traceVerbose(reason); - return undefined; - }, - ); + const disposables = new Set(); + const interpreterInfo = await getInterpreterInfo( + buildPythonExecInfo(interpreterPath), + shellExecute, + undefined, + disposables, + ).catch((reason) => { + traceVerbose(reason); + return undefined; + }); + + // Ensure the process we started is cleaned up. + disposables.forEach((p) => { + try { + p.dispose(); + } catch { + // ignore. + } + }); + if (interpreterInfo === undefined || interpreterInfo.version === undefined) { return undefined; } From 88f4153a80d060d276c498a89afb1a6c7c775b21 Mon Sep 17 00:00:00 2001 From: Karthik Nadig Date: Wed, 10 Feb 2021 00:40:25 -0800 Subject: [PATCH 4/6] Address sonar --- src/client/common/process/rawProcessApis.ts | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/client/common/process/rawProcessApis.ts b/src/client/common/process/rawProcessApis.ts index 455a6ec53cda..30eaa4901d17 100644 --- a/src/client/common/process/rawProcessApis.ts +++ b/src/client/common/process/rawProcessApis.ts @@ -50,7 +50,7 @@ export function shellExec( const shellOptions = getDefaultOptions(options); return new Promise((resolve, reject) => { // eslint-disable-next-line @typescript-eslint/no-explicit-any - const proc = exec(command, shellOptions, (e: any, stdout: any, stderr: any) => { + const callback = (e: any, stdout: any, stderr: any) => { if (e && e !== null) { reject(e); } else if (shellOptions.throwOnStdErr && stderr && stderr.length) { @@ -60,7 +60,8 @@ export function shellExec( // elsewhere because that's how exec behaves. resolve({ stderr: stderr && stderr.length > 0 ? stderr : undefined, stdout }); } - }); // NOSONAR + }; + const proc = exec(command, shellOptions, callback); // NOSONAR const disposable: IDisposable = { dispose: () => { if (!proc.killed) { From d2c51d8bef848d2f620a4251cb09544fb1aaa7b7 Mon Sep 17 00:00:00 2001 From: Karthik Nadig Date: Wed, 10 Feb 2021 12:08:59 -0800 Subject: [PATCH 5/6] Refactor process service by pulling the raw process APIs out of the class --- src/client/common/process/proc.ts | 161 +--------------- src/client/common/process/rawProcessApis.ts | 181 +++++++++++++++++- .../base/info/interpreter.ts | 10 +- .../common/externalDependencies.ts | 24 ++- .../discovery/locators/services/conda.ts | 14 +- 5 files changed, 220 insertions(+), 170 deletions(-) diff --git a/src/client/common/process/proc.ts b/src/client/common/process/proc.ts index af6b86ed44f6..61e06ed811d1 100644 --- a/src/client/common/process/proc.ts +++ b/src/client/common/process/proc.ts @@ -1,23 +1,17 @@ // Copyright (c) Microsoft Corporation. All rights reserved. // Licensed under the MIT License. -import { execSync, spawn } from 'child_process'; import { EventEmitter } from 'events'; -import { Observable } from 'rxjs/Observable'; -import { Readable } from 'stream'; import { IDisposable } from '../types'; -import { createDeferred } from '../utils/async'; import { EnvironmentVariables } from '../variables/types'; -import { getDefaultOptions, shellExec } from './rawProcessApis'; +import { execObservable, killPid, plainExec, shellExec } from './rawProcessApis'; import { ExecutionResult, IBufferDecoder, IProcessService, ObservableExecutionResult, - Output, ShellOptions, SpawnOptions, - StdErrError, } from './types'; export class ProcessService extends EventEmitter implements IProcessService { @@ -37,16 +31,7 @@ export class ProcessService extends EventEmitter implements IProcessService { } public static kill(pid: number): void { - try { - if (process.platform === 'win32') { - // Windows doesn't support SIGTERM, so execute taskkill to kill the process - execSync(`taskkill /pid ${pid} /T /F`); // NOSONAR - } else { - process.kill(pid); - } - } catch { - // Ignore. - } + killPid(pid); } public dispose(): void { @@ -61,150 +46,18 @@ export class ProcessService extends EventEmitter implements IProcessService { } public execObservable(file: string, args: string[], options: SpawnOptions = {}): ObservableExecutionResult { - const spawnOptions = this.getDefaultOptions(options); - const encoding = spawnOptions.encoding ? spawnOptions.encoding : 'utf8'; - const proc = spawn(file, args, spawnOptions); - let procExited = false; - const disposable: IDisposable = { - dispose() { - if (proc && !proc.killed && !procExited) { - ProcessService.kill(proc.pid); - } - if (proc) { - proc.unref(); - } - }, - }; - this.processesToKill.add(disposable); - - const output = new Observable>((subscriber) => { - const disposables: IDisposable[] = []; - - // eslint-disable-next-line @typescript-eslint/ban-types - const on = (ee: Readable | null, name: string, fn: Function) => { - // eslint-disable-next-line @typescript-eslint/no-explicit-any - ee?.on(name, fn as any); - // eslint-disable-next-line @typescript-eslint/no-explicit-any - disposables.push({ dispose: () => ee?.removeListener(name, fn as any) as any }); - }; - - if (options.token) { - disposables.push( - options.token.onCancellationRequested(() => { - if (!procExited && !proc.killed) { - proc.kill(); - procExited = true; - } - }), - ); - } - - const sendOutput = (source: 'stdout' | 'stderr', data: Buffer) => { - const out = this.decoder.decode([data], encoding); - if (source === 'stderr' && options.throwOnStdErr) { - subscriber.error(new StdErrError(out)); - } else { - subscriber.next({ source, out }); - } - }; - - on(proc.stdout, 'data', (data: Buffer) => sendOutput('stdout', data)); - on(proc.stderr, 'data', (data: Buffer) => sendOutput('stderr', data)); - - proc.once('close', () => { - procExited = true; - subscriber.complete(); - disposables.forEach((d) => d.dispose()); - }); - proc.once('exit', () => { - procExited = true; - subscriber.complete(); - disposables.forEach((d) => d.dispose()); - }); - proc.once('error', (ex) => { - procExited = true; - subscriber.error(ex); - disposables.forEach((d) => d.dispose()); - }); - }); - + const result = execObservable(file, args, options, this.decoder, this.env, this.processesToKill); this.emit('exec', file, args, options); - - return { - proc, - out: output, - dispose: disposable.dispose, - }; + return result; } public exec(file: string, args: string[], options: SpawnOptions = {}): Promise> { - const spawnOptions = this.getDefaultOptions(options); - const encoding = spawnOptions.encoding ? spawnOptions.encoding : 'utf8'; - const proc = spawn(file, args, spawnOptions); - const deferred = createDeferred>(); - const disposable: IDisposable = { - dispose: () => { - if (!proc.killed && !deferred.completed) { - proc.kill(); - } - }, - }; - this.processesToKill.add(disposable); - const disposables: IDisposable[] = []; - - // eslint-disable-next-line @typescript-eslint/ban-types - const on = (ee: Readable | null, name: string, fn: Function) => { - // eslint-disable-next-line @typescript-eslint/no-explicit-any - ee?.on(name, fn as any); - // eslint-disable-next-line @typescript-eslint/no-explicit-any - disposables.push({ dispose: () => ee?.removeListener(name, fn as any) as any }); - }; - - if (options.token) { - disposables.push(options.token.onCancellationRequested(disposable.dispose)); - } - - const stdoutBuffers: Buffer[] = []; - on(proc.stdout, 'data', (data: Buffer) => stdoutBuffers.push(data)); - const stderrBuffers: Buffer[] = []; - on(proc.stderr, 'data', (data: Buffer) => { - if (options.mergeStdOutErr) { - stdoutBuffers.push(data); - stderrBuffers.push(data); - } else { - stderrBuffers.push(data); - } - }); - - proc.once('close', () => { - if (deferred.completed) { - return; - } - const stderr: string | undefined = - stderrBuffers.length === 0 ? undefined : this.decoder.decode(stderrBuffers, encoding); - if (stderr && stderr.length > 0 && options.throwOnStdErr) { - deferred.reject(new StdErrError(stderr)); - } else { - const stdout = this.decoder.decode(stdoutBuffers, encoding); - deferred.resolve({ stdout, stderr }); - } - disposables.forEach((d) => d.dispose()); - }); - proc.once('error', (ex) => { - deferred.reject(ex); - disposables.forEach((d) => d.dispose()); - }); - + const promise = plainExec(file, args, options, this.decoder, this.env, this.processesToKill); this.emit('exec', file, args, options); - - return deferred.promise; + return promise; } public shellExec(command: string, options: ShellOptions = {}): Promise> { - return shellExec(command, options, this.processesToKill); - } - - private getDefaultOptions(options: T): T { - return getDefaultOptions(options, this.env); + return shellExec(command, options, this.env, this.processesToKill); } } diff --git a/src/client/common/process/rawProcessApis.ts b/src/client/common/process/rawProcessApis.ts index 30eaa4901d17..9cb0c40b9bca 100644 --- a/src/client/common/process/rawProcessApis.ts +++ b/src/client/common/process/rawProcessApis.ts @@ -1,11 +1,22 @@ // Copyright (c) Microsoft Corporation. All rights reserved. // Licensed under the MIT License. -import { exec } from 'child_process'; +import { exec, execSync, spawn } from 'child_process'; +import { Readable } from 'stream'; +import { Observable } from 'rxjs/Observable'; import { IDisposable } from '../types'; +import { createDeferred } from '../utils/async'; import { EnvironmentVariables } from '../variables/types'; import { DEFAULT_ENCODING } from './constants'; -import { ExecutionResult, ShellOptions, SpawnOptions } from './types'; +import { + ExecutionResult, + IBufferDecoder, + ObservableExecutionResult, + Output, + ShellOptions, + SpawnOptions, + StdErrError, +} from './types'; export function getDefaultOptions( options: T, @@ -45,9 +56,10 @@ export function getDefaultOptions( export function shellExec( command: string, options: ShellOptions = {}, + defaultEnv?: EnvironmentVariables, disposables?: Set, ): Promise> { - const shellOptions = getDefaultOptions(options); + const shellOptions = getDefaultOptions(options, defaultEnv); return new Promise((resolve, reject) => { // eslint-disable-next-line @typescript-eslint/no-explicit-any const callback = (e: any, stdout: any, stderr: any) => { @@ -74,3 +86,166 @@ export function shellExec( } }); } + +export function plainExec( + file: string, + args: string[], + options: SpawnOptions = {}, + decoder?: IBufferDecoder, + defaultEnv?: EnvironmentVariables, + disposables?: Set, +): Promise> { + const spawnOptions = getDefaultOptions(options, defaultEnv); + const encoding = spawnOptions.encoding ? spawnOptions.encoding : 'utf8'; + const proc = spawn(file, args, spawnOptions); + const deferred = createDeferred>(); + const disposable: IDisposable = { + dispose: () => { + if (!proc.killed && !deferred.completed) { + proc.kill(); + } + }, + }; + disposables?.add(disposable); + const internalDisposables: IDisposable[] = []; + + // eslint-disable-next-line @typescript-eslint/ban-types + const on = (ee: Readable | null, name: string, fn: Function) => { + // eslint-disable-next-line @typescript-eslint/no-explicit-any + ee?.on(name, fn as any); + // eslint-disable-next-line @typescript-eslint/no-explicit-any + internalDisposables.push({ dispose: () => ee?.removeListener(name, fn as any) as any }); + }; + + if (options.token) { + internalDisposables.push(options.token.onCancellationRequested(disposable.dispose)); + } + + const stdoutBuffers: Buffer[] = []; + on(proc.stdout, 'data', (data: Buffer) => stdoutBuffers.push(data)); + const stderrBuffers: Buffer[] = []; + on(proc.stderr, 'data', (data: Buffer) => { + if (options.mergeStdOutErr) { + stdoutBuffers.push(data); + stderrBuffers.push(data); + } else { + stderrBuffers.push(data); + } + }); + + proc.once('close', () => { + if (deferred.completed) { + return; + } + const stderr: string | undefined = + stderrBuffers.length === 0 ? undefined : decoder?.decode(stderrBuffers, encoding); + if (stderr && stderr.length > 0 && options.throwOnStdErr) { + deferred.reject(new StdErrError(stderr)); + } else { + const stdout = decoder ? decoder.decode(stdoutBuffers, encoding) : ''; + deferred.resolve({ stdout, stderr }); + } + internalDisposables.forEach((d) => d.dispose()); + }); + proc.once('error', (ex) => { + deferred.reject(ex); + internalDisposables.forEach((d) => d.dispose()); + }); + + return deferred.promise; +} + +export function execObservable( + file: string, + args: string[], + options: SpawnOptions = {}, + decoder?: IBufferDecoder, + defaultEnv?: EnvironmentVariables, + disposables?: Set, +): ObservableExecutionResult { + const spawnOptions = getDefaultOptions(options, defaultEnv); + const encoding = spawnOptions.encoding ? spawnOptions.encoding : 'utf8'; + const proc = spawn(file, args, spawnOptions); + let procExited = false; + const disposable: IDisposable = { + dispose() { + if (proc && !proc.killed && !procExited) { + killPid(proc.pid); + } + if (proc) { + proc.unref(); + } + }, + }; + disposables?.add(disposable); + + const output = new Observable>((subscriber) => { + const internalDisposables: IDisposable[] = []; + + // eslint-disable-next-line @typescript-eslint/ban-types + const on = (ee: Readable | null, name: string, fn: Function) => { + // eslint-disable-next-line @typescript-eslint/no-explicit-any + ee?.on(name, fn as any); + // eslint-disable-next-line @typescript-eslint/no-explicit-any + internalDisposables.push({ dispose: () => ee?.removeListener(name, fn as any) as any }); + }; + + if (options.token) { + internalDisposables.push( + options.token.onCancellationRequested(() => { + if (!procExited && !proc.killed) { + proc.kill(); + procExited = true; + } + }), + ); + } + + const sendOutput = (source: 'stdout' | 'stderr', data: Buffer) => { + const out = decoder ? decoder.decode([data], encoding) : ''; + if (source === 'stderr' && options.throwOnStdErr) { + subscriber.error(new StdErrError(out)); + } else { + subscriber.next({ source, out }); + } + }; + + on(proc.stdout, 'data', (data: Buffer) => sendOutput('stdout', data)); + on(proc.stderr, 'data', (data: Buffer) => sendOutput('stderr', data)); + + proc.once('close', () => { + procExited = true; + subscriber.complete(); + internalDisposables.forEach((d) => d.dispose()); + }); + proc.once('exit', () => { + procExited = true; + subscriber.complete(); + internalDisposables.forEach((d) => d.dispose()); + }); + proc.once('error', (ex) => { + procExited = true; + subscriber.error(ex); + internalDisposables.forEach((d) => d.dispose()); + }); + }); + + return { + proc, + out: output, + dispose: disposable.dispose, + }; +} + +export function killPid(pid: number): void { + try { + if (process.platform === 'win32') { + // Windows doesn't support SIGTERM, so execute taskkill to kill the process + execSync(`taskkill /pid ${pid} /T /F`); // NOSONAR + } else { + process.kill(pid); + } + } catch { + // Ignore. + } +} diff --git a/src/client/pythonEnvironments/base/info/interpreter.ts b/src/client/pythonEnvironments/base/info/interpreter.ts index 4e639fbaa4a4..834bb3f7f616 100644 --- a/src/client/pythonEnvironments/base/info/interpreter.ts +++ b/src/client/pythonEnvironments/base/info/interpreter.ts @@ -8,6 +8,7 @@ import { } from '../../../common/process/internal/scripts'; import { IDisposable } from '../../../common/types'; import { Architecture } from '../../../common/utils/platform'; +import { EnvironmentVariables } from '../../../common/variables/types'; import { copyPythonExecInfo, PythonExecInfo } from '../../exec'; import { parseVersion } from './pythonVersion'; @@ -46,7 +47,12 @@ type ShellExecResult = { stdout: string; stderr?: string; }; -type ShellExecFunc = (command: string, timeout: number, disposables?: Set) => Promise; +type ShellExecFunc = ( + command: string, + timeout: number, + defaultEnv?: EnvironmentVariables, + disposables?: Set, +) => Promise; type Logger = { info(msg: string): void; @@ -80,7 +86,7 @@ export async function getInterpreterInfo( // See these two bugs: // https://github.com/microsoft/vscode-python/issues/7569 // https://github.com/microsoft/vscode-python/issues/7760 - const result = await shellExec(quoted, 15000, disposables); + const result = await shellExec(quoted, 15000, undefined, disposables); if (result.stderr) { if (logger) { logger.error(`Failed to parse interpreter information for ${argv} stderr: ${result.stderr}`); diff --git a/src/client/pythonEnvironments/common/externalDependencies.ts b/src/client/pythonEnvironments/common/externalDependencies.ts index b23bc50970b6..6d783ab79dd1 100644 --- a/src/client/pythonEnvironments/common/externalDependencies.ts +++ b/src/client/pythonEnvironments/common/externalDependencies.ts @@ -4,13 +4,15 @@ import * as fsapi from 'fs-extra'; import * as path from 'path'; import * as vscode from 'vscode'; -import { ExecutionResult, IProcessServiceFactory, SpawnOptions } from '../../common/process/types'; +import { ExecutionResult, SpawnOptions } from '../../common/process/types'; import { IExperimentService, IDisposable } from '../../common/types'; import { chain, iterable } from '../../common/utils/async'; import { normalizeFilename } from '../../common/utils/filesystem'; import { getOSType, OSType } from '../../common/utils/platform'; import { IServiceContainer } from '../../ioc/types'; -import { shellExec } from '../../common/process/rawProcessApis'; +import { plainExec, shellExec } from '../../common/process/rawProcessApis'; +import { EnvironmentVariables } from '../../common/variables/types'; +import { BufferDecoder } from '../../common/process/decoder'; let internalServiceContainer: IServiceContainer; export function initializeExternalDependencies(serviceContainer: IServiceContainer): void { @@ -19,21 +21,23 @@ export function initializeExternalDependencies(serviceContainer: IServiceContain // processes -function getProcessFactory(): IProcessServiceFactory { - return internalServiceContainer.get(IProcessServiceFactory); -} - export async function shellExecute( command: string, timeout: number, + defaultEnv?: EnvironmentVariables, disposables?: Set, ): Promise> { - return shellExec(command, { timeout }, disposables); + return shellExec(command, { timeout }, defaultEnv, disposables); } -export async function exec(file: string, args: string[], options: SpawnOptions = {}): Promise> { - const proc = await getProcessFactory().create(); - return proc.exec(file, args, options); +export async function exec( + file: string, + args: string[], + options: SpawnOptions = {}, + defaultEnv?: EnvironmentVariables, + disposables?: Set, +): Promise> { + return plainExec(file, args, options, new BufferDecoder(), defaultEnv, disposables); } // filesystem diff --git a/src/client/pythonEnvironments/discovery/locators/services/conda.ts b/src/client/pythonEnvironments/discovery/locators/services/conda.ts index be23c549456d..e3fb275b9105 100644 --- a/src/client/pythonEnvironments/discovery/locators/services/conda.ts +++ b/src/client/pythonEnvironments/discovery/locators/services/conda.ts @@ -9,6 +9,7 @@ import { parseVersion } from '../../../base/info/pythonVersion'; import { getRegistryInterpreters } from '../../../common/windowsUtils'; import { EnvironmentType, PythonEnvironment } from '../../../info'; +import { IDisposable } from '../../../../common/types'; export const AnacondaCompanyNames = ['Anaconda, Inc.', 'Continuum Analytics, Inc.']; @@ -326,8 +327,19 @@ export class Conda { * Corresponds to "conda info --json". */ public async getInfo(): Promise { - const result = await exec(this.command, ['info', '--json']); + const disposables = new Set(); + const result = await exec(this.command, ['info', '--json'], undefined, undefined, disposables); traceVerbose(`conda info --json: ${result.stdout}`); + + // Ensure the process we started is cleaned up. + disposables.forEach((p) => { + try { + p.dispose(); + } catch { + // ignore. + } + }); + return JSON.parse(result.stdout); } From b1fe358bb17b5833935c6581e4ac13e9d4a13a18 Mon Sep 17 00:00:00 2001 From: Karthik Nadig Date: Wed, 10 Feb 2021 13:55:29 -0800 Subject: [PATCH 6/6] Address comments --- .../pythonEnvironments/base/info/interpreter.ts | 10 ++-------- .../common/externalDependencies.ts | 17 ++++++++++++----- .../discovery/locators/services/conda.ts | 2 +- 3 files changed, 15 insertions(+), 14 deletions(-) diff --git a/src/client/pythonEnvironments/base/info/interpreter.ts b/src/client/pythonEnvironments/base/info/interpreter.ts index 834bb3f7f616..4e639fbaa4a4 100644 --- a/src/client/pythonEnvironments/base/info/interpreter.ts +++ b/src/client/pythonEnvironments/base/info/interpreter.ts @@ -8,7 +8,6 @@ import { } from '../../../common/process/internal/scripts'; import { IDisposable } from '../../../common/types'; import { Architecture } from '../../../common/utils/platform'; -import { EnvironmentVariables } from '../../../common/variables/types'; import { copyPythonExecInfo, PythonExecInfo } from '../../exec'; import { parseVersion } from './pythonVersion'; @@ -47,12 +46,7 @@ type ShellExecResult = { stdout: string; stderr?: string; }; -type ShellExecFunc = ( - command: string, - timeout: number, - defaultEnv?: EnvironmentVariables, - disposables?: Set, -) => Promise; +type ShellExecFunc = (command: string, timeout: number, disposables?: Set) => Promise; type Logger = { info(msg: string): void; @@ -86,7 +80,7 @@ export async function getInterpreterInfo( // See these two bugs: // https://github.com/microsoft/vscode-python/issues/7569 // https://github.com/microsoft/vscode-python/issues/7760 - const result = await shellExec(quoted, 15000, undefined, disposables); + const result = await shellExec(quoted, 15000, disposables); if (result.stderr) { if (logger) { logger.error(`Failed to parse interpreter information for ${argv} stderr: ${result.stderr}`); diff --git a/src/client/pythonEnvironments/common/externalDependencies.ts b/src/client/pythonEnvironments/common/externalDependencies.ts index 6d783ab79dd1..062a8923245c 100644 --- a/src/client/pythonEnvironments/common/externalDependencies.ts +++ b/src/client/pythonEnvironments/common/externalDependencies.ts @@ -11,7 +11,6 @@ import { normalizeFilename } from '../../common/utils/filesystem'; import { getOSType, OSType } from '../../common/utils/platform'; import { IServiceContainer } from '../../ioc/types'; import { plainExec, shellExec } from '../../common/process/rawProcessApis'; -import { EnvironmentVariables } from '../../common/variables/types'; import { BufferDecoder } from '../../common/process/decoder'; let internalServiceContainer: IServiceContainer; @@ -21,23 +20,31 @@ export function initializeExternalDependencies(serviceContainer: IServiceContain // processes +/** + * Specialized version of the more generic shellExecute function to use only in + * cases where we don't need to pass custom environment variables read from env + * files or execution options. + */ export async function shellExecute( command: string, timeout: number, - defaultEnv?: EnvironmentVariables, disposables?: Set, ): Promise> { - return shellExec(command, { timeout }, defaultEnv, disposables); + return shellExec(command, { timeout }, undefined, disposables); } +/** + * Specialized version of the more generic exec function to use only in + * cases where we don't need to pass custom environment variables read from + * env files. + */ export async function exec( file: string, args: string[], options: SpawnOptions = {}, - defaultEnv?: EnvironmentVariables, disposables?: Set, ): Promise> { - return plainExec(file, args, options, new BufferDecoder(), defaultEnv, disposables); + return plainExec(file, args, options, new BufferDecoder(), undefined, disposables); } // filesystem diff --git a/src/client/pythonEnvironments/discovery/locators/services/conda.ts b/src/client/pythonEnvironments/discovery/locators/services/conda.ts index e3fb275b9105..537be256bd2f 100644 --- a/src/client/pythonEnvironments/discovery/locators/services/conda.ts +++ b/src/client/pythonEnvironments/discovery/locators/services/conda.ts @@ -328,7 +328,7 @@ export class Conda { */ public async getInfo(): Promise { const disposables = new Set(); - const result = await exec(this.command, ['info', '--json'], undefined, undefined, disposables); + const result = await exec(this.command, ['info', '--json'], {}, disposables); traceVerbose(`conda info --json: ${result.stdout}`); // Ensure the process we started is cleaned up.