Simplify buffer decoder. (microsoft/vscode-python#19836)

Author: karthiknadig

extensions/positron-python/src/client/common/process/decoder.ts

@@ -2,14 +2,9 @@
 // Licensed under the MIT License.
 
 import * as iconv from 'iconv-lite';
-import { injectable } from 'inversify';
 import { DEFAULT_ENCODING } from './constants';
-import { IBufferDecoder } from './types';
 
-@injectable()
-export class BufferDecoder implements IBufferDecoder {
-    public decode(buffers: Buffer[], encoding: string = DEFAULT_ENCODING): string {
-        encoding = iconv.encodingExists(encoding) ? encoding : DEFAULT_ENCODING;
-        return iconv.decode(Buffer.concat(buffers), encoding);
-    }
+export function decodeBuffer(buffers: Buffer[], encoding: string = DEFAULT_ENCODING): string {
+    encoding = iconv.encodingExists(encoding) ? encoding : DEFAULT_ENCODING;
+    return iconv.decode(Buffer.concat(buffers), encoding);
 }

extensions/positron-python/src/client/common/process/proc.ts

@@ -6,19 +6,12 @@ import { traceError } from '../../logging';
 import { IDisposable } from '../types';
 import { EnvironmentVariables } from '../variables/types';
 import { execObservable, killPid, plainExec, shellExec } from './rawProcessApis';
-import {
-    ExecutionResult,
-    IBufferDecoder,
-    IProcessService,
-    ObservableExecutionResult,
-    ShellOptions,
-    SpawnOptions,
-} from './types';
+import { ExecutionResult, IProcessService, ObservableExecutionResult, ShellOptions, SpawnOptions } from './types';
 
 export class ProcessService extends EventEmitter implements IProcessService {
     private processesToKill = new Set<IDisposable>();
 
-    constructor(private readonly decoder: IBufferDecoder, private readonly env?: EnvironmentVariables) {
+    constructor(private readonly env?: EnvironmentVariables) {
         super();
     }
 
@@ -47,13 +40,13 @@ export class ProcessService extends EventEmitter implements IProcessService {
     }
 
     public execObservable(file: string, args: string[], options: SpawnOptions = {}): ObservableExecutionResult<string> {
-        const result = execObservable(file, args, options, this.decoder, this.env, this.processesToKill);
+        const result = execObservable(file, args, options, this.env, this.processesToKill);
         this.emit('exec', file, args, options);
         return result;
     }
 
     public exec(file: string, args: string[], options: SpawnOptions = {}): Promise<ExecutionResult<string>> {
-        const promise = plainExec(file, args, options, this.decoder, this.env, this.processesToKill);
+        const promise = plainExec(file, args, options, this.env, this.processesToKill);
         this.emit('exec', file, args, options);
         return promise;
     }

extensions/positron-python/src/client/common/process/processFactory.ts

@@ -8,19 +8,18 @@ import { Uri } from 'vscode';
 import { IDisposableRegistry } from '../types';
 import { IEnvironmentVariablesProvider } from '../variables/types';
 import { ProcessService } from './proc';
-import { IBufferDecoder, IProcessLogger, IProcessService, IProcessServiceFactory } from './types';
+import { IProcessLogger, IProcessService, IProcessServiceFactory } from './types';
 
 @injectable()
 export class ProcessServiceFactory implements IProcessServiceFactory {
     constructor(
         @inject(IEnvironmentVariablesProvider) private readonly envVarsService: IEnvironmentVariablesProvider,
         @inject(IProcessLogger) private readonly processLogger: IProcessLogger,
-        @inject(IBufferDecoder) private readonly decoder: IBufferDecoder,
         @inject(IDisposableRegistry) private readonly disposableRegistry: IDisposableRegistry,
     ) {}
     public async create(resource?: Uri): Promise<IProcessService> {
         const customEnvVars = await this.envVarsService.getEnvironmentVariables(resource);
-        const proc: IProcessService = new ProcessService(this.decoder, customEnvVars);
+        const proc: IProcessService = new ProcessService(customEnvVars);
         this.disposableRegistry.push(proc);
         return proc.on('exec', this.processLogger.logProcess.bind(this.processLogger));
     }

extensions/positron-python/src/client/common/process/pythonExecutionFactory.ts

@@ -15,7 +15,6 @@ import { createPythonProcessService } from './pythonProcess';
 import {
     ExecutionFactoryCreateWithEnvironmentOptions,
     ExecutionFactoryCreationOptions,
-    IBufferDecoder,
     IProcessLogger,
     IProcessService,
     IProcessServiceFactory,
@@ -40,7 +39,6 @@ export class PythonExecutionFactory implements IPythonExecutionFactory {
         @inject(IEnvironmentActivationService) private readonly activationHelper: IEnvironmentActivationService,
         @inject(IProcessServiceFactory) private readonly processServiceFactory: IProcessServiceFactory,
         @inject(IConfigurationService) private readonly configService: IConfigurationService,
-        @inject(IBufferDecoder) private readonly decoder: IBufferDecoder,
         @inject(IComponentAdapter) private readonly pyenvs: IComponentAdapter,
         @inject(IInterpreterAutoSelectionService) private readonly autoSelection: IInterpreterAutoSelectionService,
         @inject(IInterpreterPathService) private readonly interpreterPathExpHelper: IInterpreterPathService,
@@ -110,7 +108,7 @@ export class PythonExecutionFactory implements IPythonExecutionFactory {
         const pythonPath = options.interpreter
             ? options.interpreter.path
             : this.configService.getSettings(options.resource).pythonPath;
-        const processService: IProcessService = new ProcessService(this.decoder, { ...envVars });
+        const processService: IProcessService = new ProcessService({ ...envVars });
         processService.on('exec', this.logger.logProcess.bind(this.logger));
         this.disposables.push(processService);
 

extensions/positron-python/src/client/common/process/rawProcessApis.ts

@@ -8,16 +8,9 @@ import { IDisposable } from '../types';
 import { createDeferred } from '../utils/async';
 import { EnvironmentVariables } from '../variables/types';
 import { DEFAULT_ENCODING } from './constants';
-import {
-    ExecutionResult,
-    IBufferDecoder,
-    ObservableExecutionResult,
-    Output,
-    ShellOptions,
-    SpawnOptions,
-    StdErrError,
-} from './types';
+import { ExecutionResult, ObservableExecutionResult, Output, ShellOptions, SpawnOptions, StdErrError } from './types';
 import { noop } from '../utils/misc';
+import { decodeBuffer } from './decoder';
 
 function getDefaultOptions<T extends ShellOptions | SpawnOptions>(options: T, defaultEnv?: EnvironmentVariables): T {
     const defaultOptions = { ...options };
@@ -90,7 +83,6 @@ export function plainExec(
     file: string,
     args: string[],
     options: SpawnOptions = {},
-    decoder?: IBufferDecoder,
     defaultEnv?: EnvironmentVariables,
     disposables?: Set<IDisposable>,
 ): Promise<ExecutionResult<string>> {
@@ -141,11 +133,11 @@ export function plainExec(
             return;
         }
         const stderr: string | undefined =
-            stderrBuffers.length === 0 ? undefined : decoder?.decode(stderrBuffers, encoding);
+            stderrBuffers.length === 0 ? undefined : decodeBuffer(stderrBuffers, encoding);
         if (stderr && stderr.length > 0 && options.throwOnStdErr) {
             deferred.reject(new StdErrError(stderr));
         } else {
-            let stdout = decoder ? decoder.decode(stdoutBuffers, encoding) : '';
+            let stdout = decodeBuffer(stdoutBuffers, encoding);
             stdout = filterOutputUsingCondaRunMarkers(stdout);
             deferred.resolve({ stdout, stderr });
         }
@@ -177,7 +169,6 @@ export function execObservable(
     file: string,
     args: string[],
     options: SpawnOptions = {},
-    decoder?: IBufferDecoder,
     defaultEnv?: EnvironmentVariables,
     disposables?: Set<IDisposable>,
 ): ObservableExecutionResult<string> {
@@ -220,7 +211,7 @@ export function execObservable(
         }
 
         const sendOutput = (source: 'stdout' | 'stderr', data: Buffer) => {
-            let out = decoder ? decoder.decode([data], encoding) : '';
+            let out = decodeBuffer([data], encoding);
             if (source === 'stderr' && options.throwOnStdErr) {
                 subscriber.error(new StdErrError(out));
             } else {

extensions/positron-python/src/client/common/process/serviceRegistry.ts

@@ -2,14 +2,12 @@
 // Licensed under the MIT License.
 
 import { IServiceManager } from '../../ioc/types';
-import { BufferDecoder } from './decoder';
 import { ProcessServiceFactory } from './processFactory';
 import { PythonExecutionFactory } from './pythonExecutionFactory';
 import { PythonToolExecutionService } from './pythonToolService';
-import { IBufferDecoder, IProcessServiceFactory, IPythonExecutionFactory, IPythonToolExecutionService } from './types';
+import { IProcessServiceFactory, IPythonExecutionFactory, IPythonToolExecutionService } from './types';
 
 export function registerTypes(serviceManager: IServiceManager) {
-    serviceManager.addSingleton<IBufferDecoder>(IBufferDecoder, BufferDecoder);
     serviceManager.addSingleton<IProcessServiceFactory>(IProcessServiceFactory, ProcessServiceFactory);
     serviceManager.addSingleton<IPythonExecutionFactory>(IPythonExecutionFactory, PythonExecutionFactory);
     serviceManager.addSingleton<IPythonToolExecutionService>(IPythonToolExecutionService, PythonToolExecutionService);

extensions/positron-python/src/client/common/process/types.ts

@@ -8,11 +8,6 @@ import { PythonExecInfo } from '../../pythonEnvironments/exec';
 import { InterpreterInformation, PythonEnvironment } from '../../pythonEnvironments/info';
 import { ExecutionInfo, IDisposable } from '../types';
 
-export const IBufferDecoder = Symbol('IBufferDecoder');
-export interface IBufferDecoder {
-    decode(buffers: Buffer[], encoding: string): string;
-}
-
 export type Output<T extends string | Buffer> = {
     source: 'stdout' | 'stderr';
     out: T;

extensions/positron-python/src/test/common.ts

@@ -303,10 +303,9 @@ export function correctPathForOsType(pathToCorrect: string, os?: OSType): string
  * @return `SemVer` version of the Python interpreter, or `undefined` if an error occurs.
  */
 export async function getPythonSemVer(procService?: IProcessService): Promise<SemVer | undefined> {
-    const decoder = await import('../client/common/process/decoder');
     const proc = await import('../client/common/process/proc');
 
-    const pythonProcRunner = procService ? procService : new proc.ProcessService(new decoder.BufferDecoder());
+    const pythonProcRunner = procService ? procService : new proc.ProcessService();
     const pyVerArgs = ['-c', 'import sys;print("{0}.{1}.{2}".format(*sys.version_info[:3]))'];
 
     return pythonProcRunner

extensions/positron-python/src/test/common/process/decoder.test.ts

@@ -3,7 +3,7 @@
 
 import { expect } from 'chai';
 import { encode, encodingExists } from 'iconv-lite';
-import { BufferDecoder } from '../../../client/common/process/decoder';
+import { decodeBuffer } from '../../../client/common/process/decoder';
 import { initialize } from './../../initialize';
 
 suite('Decoder', () => {
@@ -13,8 +13,7 @@ suite('Decoder', () => {
     test('Test decoding utf8 strings', () => {
         const value = 'Sample input string Сделать это';
         const buffer = encode(value, 'utf8');
-        const decoder = new BufferDecoder();
-        const decodedValue = decoder.decode([buffer]);
+        const decodedValue = decodeBuffer([buffer]);
         expect(decodedValue).equal(value, 'Decoded string is incorrect');
     });
 
@@ -24,11 +23,10 @@ suite('Decoder', () => {
         }
         const value = 'Sample input string Сделать это';
         const buffer = encode(value, 'cp866');
-        const decoder = new BufferDecoder();
-        let decodedValue = decoder.decode([buffer]);
+        let decodedValue = decodeBuffer([buffer]);
         expect(decodedValue).not.equal(value, 'Decoded string is the same');
 
-        decodedValue = decoder.decode([buffer], 'cp866');
+        decodedValue = decodeBuffer([buffer], 'cp866');
         expect(decodedValue).equal(value, 'Decoded string is incorrect');
     });
 });

extensions/positron-python/src/test/common/process/proc.exec.test.ts

@@ -6,7 +6,6 @@
 import { expect, use } from 'chai';
 import * as chaiAsPromised from 'chai-as-promised';
 import { CancellationTokenSource } from 'vscode';
-import { BufferDecoder } from '../../../client/common/process/decoder';
 import { ProcessService } from '../../../client/common/process/proc';
 import { StdErrError } from '../../../client/common/process/types';
 import { OSType } from '../../../client/common/utils/platform';
@@ -26,7 +25,7 @@ suite('ProcessService Observable', () => {
     teardown(initialize);
 
     test('exec should output print statements', async () => {
-        const procService = new ProcessService(new BufferDecoder());
+        const procService = new ProcessService();
         const printOutput = '1234';
         const result = await procService.exec(pythonPath, ['-c', `print("${printOutput}")`]);
 
@@ -42,7 +41,7 @@ suite('ProcessService Observable', () => {
             return this.skip();
         }
 
-        const procService = new ProcessService(new BufferDecoder());
+        const procService = new ProcessService();
         const printOutput = 'öä';
         const result = await procService.exec(pythonPath, ['-c', `print("${printOutput}")`]);
 
@@ -53,7 +52,7 @@ suite('ProcessService Observable', () => {
 
     test('exec should wait for completion of program with new lines', async function () {
         this.timeout(5000);
-        const procService = new ProcessService(new BufferDecoder());
+        const procService = new ProcessService();
         const pythonCode = [
             'import sys',
             'import time',
@@ -79,7 +78,7 @@ suite('ProcessService Observable', () => {
 
     test('exec should wait for completion of program without new lines', async function () {
         this.timeout(5000);
-        const procService = new ProcessService(new BufferDecoder());
+        const procService = new ProcessService();
         const pythonCode = [
             'import sys',
             'import time',
@@ -105,7 +104,7 @@ suite('ProcessService Observable', () => {
 
     test('exec should end when cancellationToken is cancelled', async function () {
         this.timeout(15000);
-        const procService = new ProcessService(new BufferDecoder());
+        const procService = new ProcessService();
         const pythonCode = [
             'import sys',
             'import time',
@@ -133,7 +132,7 @@ suite('ProcessService Observable', () => {
 
     test('exec should stream stdout and stderr separately and filter output using conda related markers', async function () {
         this.timeout(7000);
-        const procService = new ProcessService(new BufferDecoder());
+        const procService = new ProcessService();
         const pythonCode = [
             'print(">>>PYTHON-EXEC-OUTPUT")',
             'import sys',
@@ -176,7 +175,7 @@ suite('ProcessService Observable', () => {
 
     test('exec should merge stdout and stderr streams', async function () {
         this.timeout(7000);
-        const procService = new ProcessService(new BufferDecoder());
+        const procService = new ProcessService();
         const pythonCode = [
             'import sys',
             'import time',
@@ -210,29 +209,29 @@ suite('ProcessService Observable', () => {
     });
 
     test('exec should throw an error with stderr output', async () => {
-        const procService = new ProcessService(new BufferDecoder());
+        const procService = new ProcessService();
         const pythonCode = ['import sys', 'sys.stderr.write("a")', 'sys.stderr.flush()'];
         const result = procService.exec(pythonPath, ['-c', pythonCode.join(';')], { throwOnStdErr: true });
 
         await expect(result).to.eventually.be.rejectedWith(StdErrError, 'a', 'Expected error to be thrown');
     });
 
     test('exec should throw an error when spawn file not found', async () => {
-        const procService = new ProcessService(new BufferDecoder());
+        const procService = new ProcessService();
         const result = procService.exec(Date.now().toString(), []);
 
         await expect(result).to.eventually.be.rejected.and.to.have.property('code', 'ENOENT', 'Invalid error code');
     });
 
     test('exec should exit without no output', async () => {
-        const procService = new ProcessService(new BufferDecoder());
+        const procService = new ProcessService();
         const result = await procService.exec(pythonPath, ['-c', 'import sys', 'sys.exit()']);
 
         expect(result.stdout).equals('', 'stdout is invalid');
         expect(result.stderr).equals(undefined, 'stderr is invalid');
     });
     test('shellExec should be able to run python and filter output using conda related markers', async () => {
-        const procService = new ProcessService(new BufferDecoder());
+        const procService = new ProcessService();
         const printOutput = '1234';
         const result = await procService.shellExec(
             `"${pythonPath}" -c "print('>>>PYTHON-EXEC-OUTPUT');print('${printOutput}');print('<<<PYTHON-EXEC-OUTPUT')"`,
@@ -243,12 +242,12 @@ suite('ProcessService Observable', () => {
         expect(result.stdout.trim()).to.be.equal(printOutput, 'Invalid output');
     });
     test('shellExec should fail on invalid command', async () => {
-        const procService = new ProcessService(new BufferDecoder());
+        const procService = new ProcessService();
         const result = procService.shellExec('invalid command');
         await expect(result).to.eventually.be.rejectedWith(Error, 'a', 'Expected error to be thrown');
     });
     test('variables can be changed after the fact', async () => {
-        const procService = new ProcessService(new BufferDecoder(), process.env);
+        const procService = new ProcessService(process.env);
         let result = await procService.exec(pythonPath, ['-c', `import os;print(os.environ.get("MY_TEST_VARIABLE"))`], {
             extraVariables: { MY_TEST_VARIABLE: 'foo' },
         });