diff --git a/docs/PLUGIN-COMPLETION-DETECTION.md b/docs/PLUGIN-COMPLETION-DETECTION.md new file mode 100644 index 00000000..6bf8ef57 --- /dev/null +++ b/docs/PLUGIN-COMPLETION-DETECTION.md @@ -0,0 +1,36 @@ +## Configure how `simple-git` determines the `git` tasks to be complete. + +Up until version `2.46.0`, `simple-git` used both `close` and `exit` events from the `git` child process to determine +whether the task was complete as follows: + +- the close / exit event fires +- if there is data on either `stderr` or `stdout`, or if the child process as a whole has thrown an exception (for + example when `git` is not installed) then the task is complete. +- otherwise wait `50ms` and then treat the task as complete. + +From version `2.46.0` onwards, you can configure this behaviour by using the +`completion` plugin: + +```typescript +import simpleGit, { SimpleGit } from 'simple-git'; + +const git: SimpleGit = simpleGit({ + completion: { + onExit: 50, + onClose: true, + }, +}); +``` + +The `completion` plugin accepts two properties `onClose` and `onExit` that can be either: + +- `false` to ignore this event from the child process +- `true` to treat the task as complete as soon as the event has fired +- `number` to wait an arbitrary number of `ms` after the event has fired before treating the task as complete. + +To ensure backward compatibility, version 2.x of `simple-git` uses a default of +`onClose = true, onExit = 50`. + +From version 3.x of `simple-git` the default will change to `onClose = true, onExit = false`, +it should only be necessary to handle the `exit` event when the child processes are +configured to not close (ie: with keep-alive). diff --git a/readme.md b/readme.md index a97bdc51..50523dae 100644 --- a/readme.md +++ b/readme.md @@ -79,6 +79,9 @@ await git.pull(); ## Configuring Plugins +- [Completion Detection](./docs/PLUGIN-COMPLETION-DETECTION.md) + Customise how `simple-git` detects the end of a `git` process. + - [Error Detection](./docs/PLUGIN-ERRORS.md) Customise the detection of errors from the underlying `git` process. diff --git a/src/lib/git-factory.ts b/src/lib/git-factory.ts index de1f89c1..dedf1e02 100644 --- a/src/lib/git-factory.ts +++ b/src/lib/git-factory.ts @@ -3,6 +3,7 @@ import { SimpleGitFactory } from '../../typings'; import api from './api'; import { commandConfigPrefixingPlugin, + completionDetectionPlugin, errorDetectionHandler, errorDetectionPlugin, PluginStore, @@ -52,6 +53,7 @@ export function gitInstanceFactory(baseDir?: string | Partial, plugins.add(commandConfigPrefixingPlugin(config.config)); } + plugins.add(completionDetectionPlugin(config.completion)); config.progress && plugins.add(progressMonitorPlugin(config.progress)); config.timeout && plugins.add(timeoutPlugin(config.timeout)); config.spawnOptions && plugins.add(spawnOptionsPlugin(config.spawnOptions)); diff --git a/src/lib/plugins/completion-detection.plugin.ts b/src/lib/plugins/completion-detection.plugin.ts new file mode 100644 index 00000000..9f714822 --- /dev/null +++ b/src/lib/plugins/completion-detection.plugin.ts @@ -0,0 +1,81 @@ +import deferred, { DeferredPromise } from '@kwsites/promise-deferred'; +import { SimpleGitPluginConfig } from '../types'; +import { delay } from '../utils'; +import { SimpleGitPlugin } from './simple-git-plugin'; + +const never = deferred().promise; + +export function completionDetectionPlugin({ + onClose = true, + onExit = 50 + }: SimpleGitPluginConfig['completion'] = {}): SimpleGitPlugin<'spawn.after'> { + + function createEvents() { + let exitCode = -1; + const events = { + close: deferred(), + closeTimeout: deferred(), + exit: deferred(), + exitTimeout: deferred(), + }; + + const result = Promise.race([ + onClose === false ? never : events.closeTimeout.promise, + onExit === false ? never : events.exitTimeout.promise, + ]); + + configureTimeout(onClose, events.close, events.closeTimeout); + configureTimeout(onExit, events.exit, events.exitTimeout); + + return { + close(code: number) { + exitCode = code; + events.close.done(); + }, + exit(code: number) { + exitCode = code; + events.exit.done(); + }, + get exitCode() { + return exitCode; + }, + result, + }; + } + + function configureTimeout(flag: boolean | number, event: DeferredPromise, timeout: DeferredPromise) { + if (flag === false) { + return; + } + + (flag === true ? event.promise : event.promise.then(() => delay(flag))).then(timeout.done); + } + + return { + type: 'spawn.after', + async action(_data, {spawned, close}) { + const events = createEvents(); + + let deferClose = true; + let quickClose = () => void (deferClose = false); + + spawned.stdout?.on('data', quickClose); + spawned.stderr?.on('data', quickClose); + spawned.on('error', quickClose); + + spawned.on('close', (code: number) => events.close(code)); + spawned.on('exit', (code: number) => events.exit(code)); + + try{ + await events.result; + if (deferClose) { + await delay(50); + } + close(events.exitCode); + } + catch (err) { + close(events.exitCode, err); + } + } + } +} diff --git a/src/lib/plugins/index.ts b/src/lib/plugins/index.ts index b290bce8..0cfb54eb 100644 --- a/src/lib/plugins/index.ts +++ b/src/lib/plugins/index.ts @@ -1,4 +1,5 @@ export * from './command-config-prefixing-plugin'; +export * from './completion-detection.plugin'; export * from './error-detection.plugin'; export * from './plugin-store'; export * from './progress-monitor-plugin'; diff --git a/src/lib/plugins/simple-git-plugin.ts b/src/lib/plugins/simple-git-plugin.ts index 84362f37..5dba71a4 100644 --- a/src/lib/plugins/simple-git-plugin.ts +++ b/src/lib/plugins/simple-git-plugin.ts @@ -19,6 +19,7 @@ export interface SimpleGitPluginTypes { data: void; context: SimpleGitTaskPluginContext & { spawned: ChildProcess; + close (exitCode: number, reason?: Error): void; kill (reason: Error): void; }; }, diff --git a/src/lib/runners/git-executor-chain.ts b/src/lib/runners/git-executor-chain.ts index 5a235616..abd75dc8 100644 --- a/src/lib/runners/git-executor-chain.ts +++ b/src/lib/runners/git-executor-chain.ts @@ -160,32 +160,8 @@ export class GitExecutorChain implements SimpleGitExecutor { const stdOut: Buffer[] = []; const stdErr: Buffer[] = []; - let attempted = false; let rejection: Maybe; - function attemptClose(exitCode: number, event: string = 'retry') { - - // closing when there is content, terminate immediately - if (attempted || stdErr.length || stdOut.length) { - logger.info(`exitCode=%s event=%s rejection=%o`, exitCode, event, rejection); - done({ - stdOut, - stdErr, - exitCode, - rejection, - }); - attempted = true; - } - - // first attempt at closing but no content yet, wait briefly for the close/exit that may follow - if (!attempted) { - attempted = true; - setTimeout(() => attemptClose(exitCode, 'deferred'), 50); - logger('received %s event before content on stdOut/stdErr', event) - } - - } - logger.info(`%s %o`, command, args); logger('%O', spawnOptions) const spawned = spawn(command, args, spawnOptions); @@ -195,9 +171,6 @@ export class GitExecutorChain implements SimpleGitExecutor { spawned.on('error', onErrorReceived(stdErr, logger)); - spawned.on('close', (code: number) => attemptClose(code, 'close')); - spawned.on('exit', (code: number) => attemptClose(code, 'exit')); - if (outputHandler) { logger(`Passing child process stdOut/stdErr to custom outputHandler`); outputHandler(command, spawned.stdout!, spawned.stderr!, [...args]); @@ -206,6 +179,14 @@ export class GitExecutorChain implements SimpleGitExecutor { this._plugins.exec('spawn.after', undefined, { ...pluginContext(task, args), spawned, + close(exitCode: number, reason?: Error) { + done({ + stdOut, + stdErr, + exitCode, + rejection: rejection || reason, + }); + }, kill(reason: Error) { if (spawned.killed) { return; @@ -213,9 +194,8 @@ export class GitExecutorChain implements SimpleGitExecutor { rejection = reason; spawned.kill('SIGINT'); - } + }, }); - }); } diff --git a/src/lib/types/index.ts b/src/lib/types/index.ts index c7206fab..b076b452 100644 --- a/src/lib/types/index.ts +++ b/src/lib/types/index.ts @@ -66,6 +66,23 @@ export interface GitExecutorResult { export interface SimpleGitPluginConfig { + /** + * Configures the events that should be used to determine when the unederlying child process has + * been terminated. + * + * Version 2 will default to use `onClose=true, onExit=50` to mean the `close` event will be + * used to immediately treat the child process as closed and start using the data from `stdOut` + * / `stdErr`, whereas the `exit` event will wait `50ms` before treating the child process + * as closed. + * + * This will be changed in version 3 to use `onClose=true, onExit=false` so that only the + * close event is used to determine the termination of the process. + */ + completion: { + onClose?: boolean | number; + onExit?: boolean | number; + }; + /** * Configures the content of errors thrown by the `simple-git` instance for each task */ diff --git a/src/lib/utils/util.ts b/src/lib/utils/util.ts index 94c18b52..38734db3 100644 --- a/src/lib/utils/util.ts +++ b/src/lib/utils/util.ts @@ -138,13 +138,17 @@ export function prefixedArray(input: T[], prefix: T): T[] { return output; } -export function bufferToString (input: Buffer | Buffer[]): string { +export function bufferToString(input: Buffer | Buffer[]): string { return (Array.isArray(input) ? Buffer.concat(input) : input).toString('utf-8'); } /** * Get a new object from a source object with only the listed properties. */ -export function pick (source: Record, properties: string[]) { +export function pick(source: Record, properties: string[]) { return Object.assign({}, ...properties.map((property) => property in source ? {[property]: source[property]} : {})); } + +export function delay(duration = 0): Promise { + return new Promise(done => setTimeout(done, duration)); +} diff --git a/test/integration/completion-plugin.spec.ts b/test/integration/completion-plugin.spec.ts new file mode 100644 index 00000000..12adf17f --- /dev/null +++ b/test/integration/completion-plugin.spec.ts @@ -0,0 +1,15 @@ +import { promiseError } from '@kwsites/promise-result'; +import { createTestContext, newSimpleGit, SimpleGitTestContext } from '../__fixtures__'; + +describe('progress-monitor', () => { + + let context: SimpleGitTestContext; + + beforeEach(async () => context = await createTestContext()); + + it('detects successful completion', async () => { + const git = newSimpleGit(context.root); + expect(await promiseError(git.init())).toBeUndefined(); + }); + +}); diff --git a/test/unit/__fixtures__/child-processes.ts b/test/unit/__fixtures__/child-processes.ts index fd40fb06..ef4bce5a 100644 --- a/test/unit/__fixtures__/child-processes.ts +++ b/test/unit/__fixtures__/child-processes.ts @@ -90,4 +90,5 @@ async function exitChildProcess(proc: MockChildProcess, data: string | null, exi // exit/close events are bound to the process itself proc.$emit('exit', exitSignal); + proc.$emit('close', exitSignal); } diff --git a/test/unit/completion-detection-plugin.spec.ts b/test/unit/completion-detection-plugin.spec.ts new file mode 100644 index 00000000..44a01be4 --- /dev/null +++ b/test/unit/completion-detection-plugin.spec.ts @@ -0,0 +1,55 @@ +import { newSimpleGit, wait } from '../__fixtures__'; +import { theChildProcessMatching } from './__fixtures__'; +import { MockChildProcess } from './__mocks__/mock-child-process'; + +describe('completionDetectionPlugin', () => { + + function process(proc: MockChildProcess, data: string, close = false, exit = false) { + proc.stdout.$emit('data', Buffer.from(data)); + close && proc.$emit('close', 1); + exit && proc.$emit('exit', 1); + } + + it('can respond to just close events', async () => { + const git = newSimpleGit({ + completion: { + onClose: true, + onExit: false, + }, + }); + + const output = Promise.race([ + git.raw('foo'), + git.raw('bar'), + ]); + + await wait(); + + process(theChildProcessMatching(['foo']), 'foo', false, true); + process(theChildProcessMatching(['bar']), 'bar', true, false); + + expect(await output).toBe('bar') + }); + + it('can respond to just exit events', async () => { + const git = newSimpleGit({ + completion: { + onClose: false, + onExit: true, + }, + }); + + const output = Promise.race([ + git.raw('foo'), + git.raw('bar'), + ]); + + await wait(); + + process(theChildProcessMatching(['foo']), 'foo', false, true); + process(theChildProcessMatching(['bar']), 'bar', true, false); + + expect(await output).toBe('foo') + }); + +});