Skip to content

Commit

Permalink
fix: add workaround to address hanging spawned processes on windows
Browse files Browse the repository at this point in the history
The `useConpty` spawn option can be used to disable ConPTY usage until
microsoft/node-pty#437 is resolved.
  • Loading branch information
luciancooper committed Nov 23, 2022
1 parent 64f8549 commit fb6006a
Show file tree
Hide file tree
Showing 3 changed files with 36 additions and 15 deletions.
3 changes: 2 additions & 1 deletion __tests__/index.test.ts
Expand Up @@ -35,7 +35,7 @@ describe('renderFrames', () => {
describe('renderSpawn', () => {
test('promises a string when output type is `svg`', async () => {
await expect(
renderSpawn('node', ['-e', "process.stdout.write('Hello World!');"], dimensions)
renderSpawn('node', ['-e', "process.stdout.write('Hello World!');"], { ...dimensions, useConpty: false })
.then((value) => typeof value),
).resolves.toBe('string');
});
Expand All @@ -45,6 +45,7 @@ describe('renderSpawn', () => {
...dimensions,
output: 'png',
scaleFactor: 1,
useConpty: false,
}).then((value) => Buffer.isBuffer(value))).resolves.toBe(true);
});
});
Expand Down
40 changes: 26 additions & 14 deletions __tests__/spawn.test.ts
Expand Up @@ -36,14 +36,14 @@ afterEach(() => {
});

describe('readableSpawn', () => {
const dimensions = { columns: 20, rows: 5 };
const baseOptions = { columns: 20, rows: 5, useConpty: false };

test('creates readable source events from subprocess writes to stdout', async () => {
const source = readableSpawn('echo', ['echo to source stream'], dimensions),
const source = readableSpawn('node', ['-e', "process.stdout.write('echo to source stream');"], baseOptions),
events = await consume<SourceEvent>(source);
expect(events[0]).toEqual<SourceEvent>({
type: 'start',
command: 'echo "echo to source stream"',
command: 'node -e "process.stdout.write(\'echo to source stream\');"',
});
expect(events[events.length - 1]).toMatchObject<DeepPartial<SourceEvent>>({
type: 'finish',
Expand All @@ -53,7 +53,11 @@ describe('readableSpawn', () => {
});

test('kills spawned subprocess if parent process exits', async () => {
const source = readableSpawn('sleep', ['10'], dimensions);
const source = readableSpawn(
'node',
['-e', 'new Promise((resolve) => setTimeout(resolve, 5000));'],
baseOptions,
);
// mock the process exiting
(signalExit as MockSignalExit).flush();
await expect(source).resolves.toMatchObject<Partial<SpawnResult>>({
Expand All @@ -63,46 +67,54 @@ describe('readableSpawn', () => {
});

test('subprocess env will not extend process.env if `extendEnv` is false', async () => {
const source = readableSpawn('sleep', ['0'], { ...dimensions, env: {}, extendEnv: false });
const source = readableSpawn(
'node',
['-e', 'new Promise((resolve) => setTimeout(resolve, 0));'],
{ ...baseOptions, env: {}, extendEnv: false },
);
await expect(source).resolves.toMatchObject<Partial<SpawnResult>>({ exitCode: 0, failed: false });
expect(source.env).toEqual(colorEnv);
});

describe('validation', () => {
test('throws an error if `command` arg is an empty string', async () => {
expect(() => {
readableSpawn('', [], dimensions);
readableSpawn('', [], baseOptions);
}).toThrow("'command' cannot be empty");
});

test('throws type error if `command` arg is not a string', async () => {
expect(() => {
readableSpawn({} as unknown as string, [], dimensions);
readableSpawn({} as unknown as string, [], baseOptions);
}).toThrow("'command' must be a string. Received [object Object]");
});

test('throws error if timeout option is invalid', () => {
expect(() => {
readableSpawn('ls', [], { ...dimensions, timeout: -500 });
readableSpawn('ls', [], { ...baseOptions, timeout: -500 });
}).toThrow('`timeout` must be a non-negative integer');
});
});

describe('timeouts', () => {
test('will send `killSignal` signal to spawned process on timeout', async () => {
await expect(readableSpawn('sleep', ['1'], {
...dimensions,
timeout: 500,
killSignal: 'SIGKILL',
})).resolves.toMatchObject<Partial<SpawnResult>>({
await expect(readableSpawn(
'node',
['-e', 'new Promise((resolve) => setTimeout(resolve, 1000));'],
{ ...baseOptions, timeout: 500, killSignal: 'SIGKILL' },
)).resolves.toMatchObject<Partial<SpawnResult>>({
...process.platform !== 'win32' ? { signal: 'SIGKILL' } : {},
timedOut: true,
killed: true,
});
});

test('timeout is cleared if process completes before it is reached', async () => {
const source = readableSpawn('sleep', ['1'], { ...dimensions, timeout: 2000 });
const source = readableSpawn(
'node',
['-e', 'new Promise((resolve) => setTimeout(resolve, 1000));'],
{ ...baseOptions, timeout: 2000 },
);
await expect(source).resolves.toMatchObject<Partial<SpawnResult>>({
timedOut: false,
failed: false,
Expand Down
8 changes: 8 additions & 0 deletions src/spawn.ts
Expand Up @@ -57,6 +57,12 @@ export interface SpawnOptions {
* @defaultValue `'SIGTERM'`
*/
killSignal?: NodeJS.Signals

/**
* Windows only passed to `node-pty` concerning whether to use ConPTY over WinPTY.
* Added as a workaround until {@link https://github.com/microsoft/node-pty/issues/437} is resolved
*/
useConpty?: boolean
}

export const colorEnv = {
Expand Down Expand Up @@ -127,6 +133,7 @@ export default function readableSpawn(command: string, args: string[], {
extendEnv = true,
timeout = 0,
killSignal = 'SIGTERM',
useConpty,
}: Dimensions & SpawnOptions) {
// validate args
if (typeof command !== 'string') {
Expand All @@ -152,6 +159,7 @@ export default function readableSpawn(command: string, args: string[], {
name: term,
env,
cwd,
useConpty,
});
// emit stream start event
stream.start([command, ...args.map((arg) => (
Expand Down

0 comments on commit fb6006a

Please sign in to comment.