From b14bc3bc977fb64a94f380a61e601dff89ff8682 Mon Sep 17 00:00:00 2001 From: Jacky Zhao Date: Thu, 24 Oct 2024 14:24:48 -0700 Subject: [PATCH 01/60] bump repeats, add debug --- tests/index.test.ts | 3 ++- wrapper.ts | 6 ++++++ 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/tests/index.test.ts b/tests/index.test.ts index 3fde299..512ea0a 100644 --- a/tests/index.test.ts +++ b/tests/index.test.ts @@ -303,6 +303,7 @@ describe( test( 'ordering is correct', + { repeats: 100 }, () => new Promise((done) => { const oldFds = getOpenFds(); @@ -312,7 +313,7 @@ describe( command: '/bin/sh', args: [ '-c', - 'seq 0 1024' + `seq 0 ${n}` ], onExit: (err, exitCode) => { expect(err).toBeNull(); diff --git a/wrapper.ts b/wrapper.ts index e47f63f..78183f8 100644 --- a/wrapper.ts +++ b/wrapper.ts @@ -72,6 +72,7 @@ export class Pty { markFdClosed = resolve; }); const mockedExit = (error: NodeJS.ErrnoException | null, code: number) => { + console.log('mocked exit') markExited({ error, code }); }; @@ -97,13 +98,16 @@ export class Pty { this.#handledEndOfData = true; // must wait for fd close and exit result before calling real exit + console.log('handle end') await fdClosed; const result = await exitResult; realExit(result.error, result.code); + console.log('done') }; this.read.on('end', handleEnd); this.read.on('close', () => { + console.log('handle close') markFdClosed(); }); @@ -123,6 +127,8 @@ export class Pty { // is nothing left to read and we can start tearing things down. If we hadn't received an // error so far, we are considered to be in good standing. this.read.off('error', handleError); + + console.log('eio') handleEnd(); return; } From 92edf92335ddecdb4db0e1233f362b3432910bf8 Mon Sep 17 00:00:00 2001 From: Jacky Zhao Date: Thu, 24 Oct 2024 14:29:28 -0700 Subject: [PATCH 02/60] no manual eio --- tests/index.test.ts | 1 - wrapper.ts | 2 +- 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/tests/index.test.ts b/tests/index.test.ts index 512ea0a..6a3f976 100644 --- a/tests/index.test.ts +++ b/tests/index.test.ts @@ -347,7 +347,6 @@ describe( onExit: (err, exitCode) => { expect(err).toBeNull(); expect(exitCode).toBe(0); - // account for the newline expect(buffer.toString().length).toBe(payload.length); done(); }, diff --git a/wrapper.ts b/wrapper.ts index 78183f8..46714f6 100644 --- a/wrapper.ts +++ b/wrapper.ts @@ -129,7 +129,7 @@ export class Pty { this.read.off('error', handleError); console.log('eio') - handleEnd(); + // handleEnd(); return; } } From 4cce1842168c7a2618bb2b45937bb71164637b43 Mon Sep 17 00:00:00 2001 From: Jacky Zhao Date: Thu, 24 Oct 2024 14:41:45 -0700 Subject: [PATCH 03/60] more rearranging --- wrapper.ts | 21 ++++++++++++--------- 1 file changed, 12 insertions(+), 9 deletions(-) diff --git a/wrapper.ts b/wrapper.ts index 46714f6..ba49ed4 100644 --- a/wrapper.ts +++ b/wrapper.ts @@ -67,9 +67,10 @@ export class Pty { let exitResult: Promise = new Promise((resolve) => { markExited = resolve; }); - let markFdClosed: () => void; - let fdClosed = new Promise((resolve) => { - markFdClosed = resolve; + + let markReadFinished: () => void; + let readFinished = new Promise((resolve) => { + markReadFinished = resolve; }); const mockedExit = (error: NodeJS.ErrnoException | null, code: number) => { console.log('mocked exit') @@ -99,16 +100,19 @@ export class Pty { // must wait for fd close and exit result before calling real exit console.log('handle end') - await fdClosed; + await readFinished; const result = await exitResult; realExit(result.error, result.code); console.log('done') }; - this.read.on('end', handleEnd); + this.read.on('end', () => { + console.log('end') + markReadFinished(); + }); + this.read.on('close', () => { - console.log('handle close') - markFdClosed(); + handleEnd(); }); // PTYs signal their done-ness with an EIO error. we therefore need to filter them out (as well as @@ -127,9 +131,8 @@ export class Pty { // is nothing left to read and we can start tearing things down. If we hadn't received an // error so far, we are considered to be in good standing. this.read.off('error', handleError); - console.log('eio') - // handleEnd(); + this.#socket.emit('end'); return; } } From be250b36ea11e0abc3f77c9c253b8ed8be965f89 Mon Sep 17 00:00:00 2001 From: Jacky Zhao Date: Thu, 24 Oct 2024 14:59:08 -0700 Subject: [PATCH 04/60] fix the concurrent shells test --- tests/index.test.ts | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/tests/index.test.ts b/tests/index.test.ts index 6a3f976..dae8a87 100644 --- a/tests/index.test.ts +++ b/tests/index.test.ts @@ -414,15 +414,14 @@ describe( const writeStreams: Array = []; // We have local echo enabled, so we'll read the message twice. - const expectedResult = 'ready\r\nhello cat\r\nhello cat\r\n'; + const expectedResult = 'hello cat\r\nhello cat\r\n'; for (let i = 0; i < 10; i++) { donePromises.push( new Promise((accept) => { let buffer = Buffer.from(''); const pty = new Pty({ - command: '/bin/sh', - args: ['-c', 'echo ready ; exec cat'], + command: '/bin/cat', onExit: (err, exitCode) => { expect(err).toBeNull(); expect(exitCode).toBe(0); @@ -442,23 +441,21 @@ describe( const readStream = pty.read; readStream.on('data', (data) => { buffer = Buffer.concat([buffer, data]); - if (!readyMessageReceived) { + if (buffer.toString().includes('hello cat\r\n') && !readyMessageReceived) { readyMessageReceived = true; ready(); } }); }), ); + writeStreams.push(pty.write); + pty.write.write('hello cat\n'); }), ); } Promise.allSettled(readyPromises).then(() => { - // The message should end in newline so that the EOT can signal that the input has ended and not - // just the line. - const message = 'hello cat\n'; for (const writeStream of writeStreams) { - writeStream.write(message); writeStream.end(EOT); } }); From 6068305fc1d9c925bdd388f0130063cfd29d03b9 Mon Sep 17 00:00:00 2001 From: Jacky Zhao Date: Thu, 24 Oct 2024 15:05:02 -0700 Subject: [PATCH 05/60] log close path --- wrapper.ts | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/wrapper.ts b/wrapper.ts index ba49ed4..711da62 100644 --- a/wrapper.ts +++ b/wrapper.ts @@ -91,7 +91,7 @@ export class Pty { }); // catch end events - const handleEnd = async () => { + const handleClose = async () => { if (this.#handledEndOfData) { return; } @@ -112,7 +112,8 @@ export class Pty { }); this.read.on('close', () => { - handleEnd(); + console.log('close') + handleClose(); }); // PTYs signal their done-ness with an EIO error. we therefore need to filter them out (as well as From 9407e1c6f5e2fb1b8de8576ccc7b5fc8856b14da Mon Sep 17 00:00:00 2001 From: Jacky Zhao Date: Thu, 24 Oct 2024 15:16:09 -0700 Subject: [PATCH 06/60] use async pattern for the tests --- tests/index.test.ts | 718 +++++++++++++++++++++----------------------- 1 file changed, 339 insertions(+), 379 deletions(-) diff --git a/tests/index.test.ts b/tests/index.test.ts index dae8a87..039822f 100644 --- a/tests/index.test.ts +++ b/tests/index.test.ts @@ -1,7 +1,7 @@ import { Pty, getCloseOnExec, setCloseOnExec } from '../wrapper'; import { type Writable } from 'stream'; import { readdirSync, readlinkSync } from 'fs'; -import { describe, test, expect, beforeEach, afterEach } from 'vitest'; +import { describe, test, expect, beforeEach, afterEach, vi } from 'vitest'; import { exec as execAsync } from 'child_process'; import { promisify } from 'util'; const exec = promisify(execAsync); @@ -49,136 +49,120 @@ describe( 'PTY', { repeats: 50 }, () => { - test('spawns and exits', () => - new Promise((done) => { - const oldFds = getOpenFds(); - const message = 'hello from a pty'; - let buffer = ''; + test('spawns and exits', async () => { + const oldFds = getOpenFds(); + const message = 'hello from a pty'; + let buffer = ''; - const pty = new Pty({ - command: '/bin/echo', - args: [message], - onExit: (err, exitCode) => { - expect(err).toBeNull(); - expect(exitCode).toBe(0); - expect(buffer.trim()).toBe(message); - expect(getOpenFds()).toStrictEqual(oldFds); - done(); - }, - }); + const onExit = vi.fn(); + const pty = new Pty({ + command: '/bin/echo', + args: [message], + onExit, + }); - const readStream = pty.read; - readStream.on('data', (chunk) => { - buffer = chunk.toString(); - }); - })); + const readStream = pty.read; + readStream.on('data', (chunk) => { + buffer = chunk.toString(); + }); - test('captures an exit code', () => - new Promise((done) => { - const oldFds = getOpenFds(); - const pty = new Pty({ - command: '/bin/sh', - args: ['-c', 'exit 17'], - onExit: (err, exitCode) => { - expect(err).toBeNull(); - expect(exitCode).toBe(17); - expect(getOpenFds()).toStrictEqual(oldFds); - done(); - }, - }); + await vi.waitFor(() => expect(onExit).toHaveBeenCalledTimes(1)); + expect(onExit).toHaveBeenCalledWith(null, 0); + expect(buffer.trim()).toBe(message); + expect(getOpenFds()).toStrictEqual(oldFds); + }); - // set a pty reader so it can flow - pty.read.on('data', () => { }); - })); + test('captures an exit code', async () => { + const oldFds = getOpenFds(); + const onExit = vi.fn(); + const pty = new Pty({ + command: '/bin/sh', + args: ['-c', 'exit 17'], + onExit, + }); - test('can be written to', () => - new Promise((done) => { - const oldFds = getOpenFds(); + // set a pty reader so it can flow + pty.read.on('data', () => {}); - // The message should end in newline so that the EOT can signal that the input has ended and not - // just the line. - const message = 'hello cat\n'; - let buffer = ''; + await vi.waitFor(() => expect(onExit).toHaveBeenCalledTimes(1)); + expect(onExit).toHaveBeenCalledWith(null, 17); + expect(getOpenFds()).toStrictEqual(oldFds); + }); - // We have local echo enabled, so we'll read the message twice. - const expectedResult = 'hello cat\r\nhello cat\r\n'; + test('can be written to', async () => { + const oldFds = getOpenFds(); + const message = 'hello cat\n'; + let buffer = ''; + const expectedResult = 'hello cat\r\nhello cat\r\n'; + const onExit = vi.fn(); - const pty = new Pty({ - command: '/bin/cat', - onExit: (err, exitCode) => { - expect(err).toBeNull(); - expect(exitCode).toBe(0); - let result = buffer.toString(); - if (IS_DARWIN) { - // Darwin adds the visible EOT to the stream. - result = result.replace('^D\b\b', ''); - } - expect(result.trim()).toStrictEqual(expectedResult.trim()); - expect(getOpenFds()).toStrictEqual(oldFds); - done(); - }, - }); - - const writeStream = pty.write; - const readStream = pty.read; + const pty = new Pty({ + command: '/bin/cat', + onExit, + }); - readStream.on('data', (data) => { - buffer += data.toString(); - }); - writeStream.write(message); - writeStream.end(EOT); - })); + const writeStream = pty.write; + const readStream = pty.read; - test('can be started in non-interactive fashion', () => - new Promise((done) => { - const oldFds = getOpenFds(); + readStream.on('data', (data) => { + buffer += data.toString(); + }); + + writeStream.write(message); + writeStream.end(EOT); + + await vi.waitFor(() => expect(onExit).toHaveBeenCalledTimes(1)); + expect(onExit).toHaveBeenCalledWith(null, 0); + + let result = buffer.toString(); + if (IS_DARWIN) { + // Darwin adds the visible EOT to the stream. + result = result.replace('^D\b\b', ''); + } + expect(result.trim()).toStrictEqual(expectedResult.trim()); + expect(getOpenFds()).toStrictEqual(oldFds); + }); - let buffer = ''; + test('can be started in non-interactive fashion', async () => { + const oldFds = getOpenFds(); + let buffer = ''; + const expectedResult = '\r\n'; + const onExit = vi.fn(); - const expectedResult = '\r\n'; + const pty = new Pty({ + command: '/bin/cat', + interactive: false, + onExit, + }); - const pty = new Pty({ - command: '/bin/cat', - interactive: false, - onExit: (err, exitCode) => { - expect(err).toBeNull(); - expect(exitCode).toBe(0); - let result = buffer.toString(); - expect(result.trim()).toStrictEqual(expectedResult.trim()); - expect(getOpenFds()).toStrictEqual(oldFds); - done(); - }, - }); + const readStream = pty.read; + readStream.on('data', (data) => { + buffer += data.toString(); + }); - const readStream = pty.read; + await vi.waitFor(() => expect(onExit).toHaveBeenCalledTimes(1)); + expect(onExit).toHaveBeenCalledWith(null, 0); + let result = buffer.toString(); + expect(result.trim()).toStrictEqual(expectedResult.trim()); + expect(getOpenFds()).toStrictEqual(oldFds); + }); - readStream.on('data', (data) => { - buffer += data.toString(); - }); - })); + test('can be resized', async () => { + const oldFds = getOpenFds(); + let buffer = ''; + let state: 'expectPrompt' | 'expectDone1' | 'expectDone2' | 'done' = 'expectPrompt'; + const onExit = vi.fn(); - test('can be resized', () => - new Promise((done) => { - const oldFds = getOpenFds(); - let buffer = ''; - let state: 'expectPrompt' | 'expectDone1' | 'expectDone2' | 'done' = - 'expectPrompt'; - const pty = new Pty({ - command: '/bin/sh', - size: { rows: 24, cols: 80 }, - onExit: (err, exitCode) => { - expect(err).toBeNull(); - expect(exitCode).toBe(0); - - expect(state).toBe('done'); - expect(getOpenFds()).toStrictEqual(oldFds); - done(); - }, - }); + const pty = new Pty({ + command: '/bin/sh', + size: { rows: 24, cols: 80 }, + onExit, + }); - const writeStream = pty.write; - const readStream = pty.read; + const writeStream = pty.write; + const readStream = pty.read; + const statePromise = new Promise((resolve) => { readStream.on('data', (data) => { buffer += data.toString(); @@ -200,288 +184,267 @@ describe( if (state === 'expectDone2' && buffer.includes('done2\r\n')) { expect(buffer).toContain('60 100'); state = 'done'; - writeStream.write(EOT); - return; + resolve(); } }); - })); + }); - test('respects working directory', () => - new Promise((done) => { - const oldFds = getOpenFds(); - const cwd = process.cwd(); - let buffer = ''; + await statePromise; + await vi.waitFor(() => expect(onExit).toHaveBeenCalledTimes(1)); + expect(onExit).toHaveBeenCalledWith(null, 0); + expect(state).toBe('done'); + expect(getOpenFds()).toStrictEqual(oldFds); + }); - const pty = new Pty({ - command: '/bin/pwd', - dir: cwd, - onExit: (err, exitCode) => { - expect(err).toBeNull(); - expect(exitCode).toBe(0); - expect(buffer.trim()).toBe(cwd); - expect(getOpenFds()).toStrictEqual(oldFds); - done(); - }, - }); + test('respects working directory', async () => { + const oldFds = getOpenFds(); + const cwd = process.cwd(); + let buffer = ''; + const onExit = vi.fn(); - const readStream = pty.read; - readStream.on('data', (data) => { - buffer += data.toString(); - }); - })); + const pty = new Pty({ + command: '/bin/pwd', + dir: cwd, + onExit, + }); - test('respects env', () => - new Promise((done) => { - const oldFds = getOpenFds(); - const message = 'hello from env'; - let buffer = ''; + const readStream = pty.read; + readStream.on('data', (data) => { + buffer += data.toString(); + }); - const pty = new Pty({ - command: '/bin/sh', - args: ['-c', 'echo $ENV_VARIABLE && exit'], - envs: { - ENV_VARIABLE: message, - }, - onExit: (err, exitCode) => { - expect(err).toBeNull(); - expect(exitCode).toBe(0); - expect(buffer.trim()).toBe(message); - expect(getOpenFds()).toStrictEqual(oldFds); - done(); - }, - }); + await vi.waitFor(() => expect(onExit).toHaveBeenCalledTimes(1)); + expect(onExit).toHaveBeenCalledWith(null, 0); + expect(buffer.trim()).toBe(cwd); + expect(getOpenFds()).toStrictEqual(oldFds); + }); - const readStream = pty.read; - readStream.on('data', (data) => { - buffer += data.toString(); - }); - })); + test('respects env', async () => { + const oldFds = getOpenFds(); + const message = 'hello from env'; + let buffer = ''; + const onExit = vi.fn(); - test('resize after exit shouldn\'t throw', () => new Promise((done, reject) => { + const pty = new Pty({ + command: '/bin/sh', + args: ['-c', 'echo $ENV_VARIABLE && exit'], + envs: { + ENV_VARIABLE: message, + }, + onExit, + }); + + const readStream = pty.read; + readStream.on('data', (data) => { + buffer += data.toString(); + }); + + await vi.waitFor(() => expect(onExit).toHaveBeenCalledTimes(1)); + expect(onExit).toHaveBeenCalledWith(null, 0); + expect(buffer.trim()).toBe(message); + expect(getOpenFds()).toStrictEqual(oldFds); + }); + + test('resize after exit shouldn\'t throw', async () => { + const onExit = vi.fn(); const pty = new Pty({ command: '/bin/echo', args: ['hello'], - onExit: (err, exitCode) => { - try { - expect(err).toBeNull(); - expect(exitCode).toBe(0); - expect(() => { - pty.resize({ rows: 60, cols: 100 }); - }).not.toThrow(); - done(); - } catch (e) { - reject(e) - } - }, + onExit, }); - pty.read.on('data', () => { }); - })); + pty.read.on('data', () => {}); - test('resize after close shouldn\'t throw', () => new Promise((done, reject) => { + await vi.waitFor(() => expect(onExit).toHaveBeenCalledTimes(1)); + expect(onExit).toHaveBeenCalledWith(null, 0); + expect(() => { + pty.resize({ rows: 60, cols: 100 }); + }).not.toThrow(); + }); + + test('resize after close shouldn\'t throw', async () => { + const onExit = vi.fn(); const pty = new Pty({ command: '/bin/sh', - onExit: (err, exitCode) => { - try { - expect(err).toBeNull(); - expect(exitCode).toBe(0); - } catch (e) { - reject(e) - } - }, + onExit, }); - pty.read.on('data', () => { }); + pty.read.on('data', () => {}); pty.close(); expect(() => { pty.resize({ rows: 60, cols: 100 }); }).not.toThrow(); - done(); - })); + + await vi.waitFor(() => expect(onExit).toHaveBeenCalledTimes(1)); + expect(onExit).toHaveBeenCalledWith(null, 0); + }); test( 'ordering is correct', { repeats: 100 }, - () => - new Promise((done) => { - const oldFds = getOpenFds(); - let buffer = Buffer.from(''); - const n = 1024; - const pty = new Pty({ - command: '/bin/sh', - args: [ - '-c', - `seq 0 ${n}` - ], - onExit: (err, exitCode) => { - expect(err).toBeNull(); - expect(exitCode).toBe(0); - expect(buffer.toString().trim().split('\n').map(Number)).toStrictEqual( - Array.from({ length: n + 1 }, (_, i) => i), - ); - expect(getOpenFds()).toStrictEqual(oldFds); - done(); - }, - }); - - const readStream = pty.read; - readStream.on('data', (data) => { - buffer = Buffer.concat([buffer, data]); - }); - }), - ); + async () => { + const oldFds = getOpenFds(); + let buffer = Buffer.from(''); + const n = 1024; + const onExit = vi.fn(); - test('doesnt miss large output from fast commands', - () => - new Promise((done) => { - const payload = `hello`.repeat(4096); - let buffer = Buffer.from(''); - const pty = new Pty({ - command: '/bin/echo', - args: [ - '-n', - payload - ], - onExit: (err, exitCode) => { - expect(err).toBeNull(); - expect(exitCode).toBe(0); - expect(buffer.toString().length).toBe(payload.length); - done(); - }, - }); - - const readStream = pty.read; - readStream.on('data', (data) => { - buffer = Buffer.concat([buffer, data]); - }); - }) - ); + const pty = new Pty({ + command: '/bin/sh', + args: [ + '-c', + `seq 0 ${n}` + ], + onExit, + }); - testSkipOnDarwin( - 'does not leak files', - () => - new Promise((done) => { - const oldFds = getOpenFds(); - const promises = []; - for (let i = 0; i < 10; i++) { - promises.push( - new Promise((accept) => { - let buffer = Buffer.from(''); - const pty = new Pty({ - command: '/bin/sh', - args: ['-c', 'sleep 0.1 ; ls /proc/$$/fd'], - onExit: (err, exitCode) => { - expect(err).toBeNull(); - expect(exitCode).toBe(0); - expect( - buffer - .toString() - .trim() - .split(/\s+/) - .filter((fd) => { - // Some shells dup stdio to fd 255 for reasons. - return fd !== '255'; - }) - .toSorted(), - ).toStrictEqual(['0', '1', '2']); - accept(); - }, - }); - - const readStream = pty.read; - readStream.on('data', (data) => { - buffer = Buffer.concat([buffer, data]); - }); - }), - ); - } - Promise.allSettled(promises).then(() => { - expect(getOpenFds()).toStrictEqual(oldFds); - done(); - }); - }), - ); + const readStream = pty.read; + readStream.on('data', (data) => { + buffer = Buffer.concat([buffer, data]); + }); - test( - 'can run concurrent shells', - () => - new Promise((done) => { - const oldFds = getOpenFds(); - const donePromises: Array> = []; - const readyPromises: Array> = []; - const writeStreams: Array = []; - - // We have local echo enabled, so we'll read the message twice. - const expectedResult = 'hello cat\r\nhello cat\r\n'; - - for (let i = 0; i < 10; i++) { - donePromises.push( - new Promise((accept) => { - let buffer = Buffer.from(''); - const pty = new Pty({ - command: '/bin/cat', - onExit: (err, exitCode) => { - expect(err).toBeNull(); - expect(exitCode).toBe(0); - let result = buffer.toString(); - if (IS_DARWIN) { - // Darwin adds the visible EOT to the stream. - result = result.replace('^D\b\b', ''); - } - expect(result).toStrictEqual(expectedResult); - accept(); - }, - }); - - readyPromises.push( - new Promise((ready) => { - let readyMessageReceived = false; - const readStream = pty.read; - readStream.on('data', (data) => { - buffer = Buffer.concat([buffer, data]); - if (buffer.toString().includes('hello cat\r\n') && !readyMessageReceived) { - readyMessageReceived = true; - ready(); - } - }); - }), - ); - - writeStreams.push(pty.write); - pty.write.write('hello cat\n'); - }), - ); - } - Promise.allSettled(readyPromises).then(() => { - for (const writeStream of writeStreams) { - writeStream.end(EOT); - } - }); - Promise.allSettled(donePromises).then(() => { - expect(getOpenFds()).toStrictEqual(oldFds); - done(); - }); - }), + await vi.waitFor(() => expect(onExit).toHaveBeenCalledTimes(1)); + expect(onExit).toHaveBeenCalledWith(null, 0); + expect(buffer.toString().trim().split('\n').map(Number)).toStrictEqual( + Array.from({ length: n + 1 }, (_, i) => i), + ); + expect(getOpenFds()).toStrictEqual(oldFds); + } ); - test("doesn't break when executing non-existing binary", () => - new Promise((done) => { - const oldFds = getOpenFds(); - try { - new Pty({ - command: '/bin/this-does-not-exist', - onExit: () => { - expect(getOpenFds()).toStrictEqual(oldFds); - }, - }); - } catch (e: any) { - expect(e.message).toContain('No such file or directory'); - - done(); + test('doesnt miss large output from fast commands', async () => { + const payload = `hello`.repeat(4096); + let buffer = Buffer.from(''); + const onExit = vi.fn(); + + const pty = new Pty({ + command: '/bin/echo', + args: [ + '-n', + payload + ], + onExit, + }); + + const readStream = pty.read; + readStream.on('data', (data) => { + buffer = Buffer.concat([buffer, data]); + }); + + await vi.waitFor(() => expect(onExit).toHaveBeenCalledTimes(1)); + expect(onExit).toHaveBeenCalledWith(null, 0); + expect(buffer.toString().length).toBe(payload.length); + }); + + testSkipOnDarwin('does not leak files', async () => { + const oldFds = getOpenFds(); + const promises = []; + + for (let i = 0; i < 10; i++) { + const onExit = vi.fn(); + let buffer = Buffer.from(''); + + const pty = new Pty({ + command: '/bin/sh', + args: ['-c', 'sleep 0.1 ; ls /proc/$$/fd'], + onExit, + }); + + const readStream = pty.read; + readStream.on('data', (data) => { + buffer = Buffer.concat([buffer, data]); + }); + + promises.push( + vi.waitFor(() => expect(onExit).toHaveBeenCalledTimes(1)).then(() => { + expect(onExit).toHaveBeenCalledWith(null, 0); + expect( + buffer + .toString() + .trim() + .split(/\s+/) + .filter((fd) => { + // Some shells dup stdio to fd 255 for reasons. + return fd !== '255'; + }) + .toSorted(), + ).toStrictEqual(['0', '1', '2']); + }) + ); + } + + await Promise.all(promises); + expect(getOpenFds()).toStrictEqual(oldFds); + }); + + test('can run concurrent shells', async () => { + const oldFds = getOpenFds(); + const writeStreams: Array = []; + const buffers: Array = []; + const onExits: Array = []; + const expectedResult = 'hello cat\r\nhello cat\r\n'; + + // Create 10 concurrent shells + for (let i = 0; i < 10; i++) { + const onExit = vi.fn(); + onExits.push(onExit); + buffers[i] = Buffer.from(''); + + const pty = new Pty({ + command: '/bin/cat', + onExit, + }); + + const readStream = pty.read; + readStream.on('data', (data) => { + buffers[i] = Buffer.concat([buffers[i], data]); + }); + + writeStreams.push(pty.write); + pty.write.write('hello cat\n'); + } + + // Wait for initial output + await vi.waitFor(() => + buffers.every(buffer => buffer.toString().includes('hello cat\r\n')) + ); + + // Send EOT to all shells + for (const writeStream of writeStreams) { + writeStream.end(EOT); + } + + // Wait for all shells to exit + await Promise.all(onExits.map(onExit => + vi.waitFor(() => expect(onExit).toHaveBeenCalledTimes(1)) + )); + + // Verify results + for (let i = 0; i < 10; i++) { + expect(onExits[i]).toHaveBeenCalledWith(null, 0); + let result = buffers[i].toString(); + if (IS_DARWIN) { + result = result.replace('^D\b\b', ''); } - })); + expect(result).toStrictEqual(expectedResult); + } + + expect(getOpenFds()).toStrictEqual(oldFds); + }); + + test("doesn't break when executing non-existing binary", async () => { + const oldFds = getOpenFds(); + + await expect(async () => { + new Pty({ + command: '/bin/this-does-not-exist', + onExit: () => {}, + }); + }).rejects.toThrow('No such file or directory'); + + expect(getOpenFds()).toStrictEqual(oldFds); + }); }, ); @@ -489,52 +452,49 @@ describe('cgroup opts', () => { beforeEach(async () => { if (!IS_DARWIN) { // create a new cgroup with the right permissions - await exec("sudo cgcreate -g 'cpu:/test.slice'") - await exec("sudo chown -R $(id -u):$(id -g) /sys/fs/cgroup/cpu/test.slice") + await exec("sudo cgcreate -g 'cpu:/test.slice'"); + await exec("sudo chown -R $(id -u):$(id -g) /sys/fs/cgroup/cpu/test.slice"); } }); afterEach(async () => { if (!IS_DARWIN) { // remove the cgroup - await exec("sudo cgdelete cpu:/test.slice") + await exec("sudo cgdelete cpu:/test.slice"); } }); - testSkipOnDarwin('basic cgroup', () => new Promise((done) => { + testSkipOnDarwin('basic cgroup', async () => { const oldFds = getOpenFds(); let buffer = ''; + const onExit = vi.fn(); + const pty = new Pty({ command: '/bin/cat', args: ['/proc/self/cgroup'], cgroupPath: '/sys/fs/cgroup/cpu/test.slice', - onExit: (err, exitCode) => { - expect(err).toBeNull(); - expect(exitCode).toBe(0); - expect(buffer).toContain('/test.slice'); - expect(getOpenFds()).toStrictEqual(oldFds); - done(); - }, + onExit, }); const readStream = pty.read; readStream.on('data', (data) => { buffer = data.toString(); }); - }) - ); - testOnlyOnDarwin('cgroup is not supported on darwin', () => { + await vi.waitFor(() => expect(onExit).toHaveBeenCalledTimes(1)); + expect(onExit).toHaveBeenCalledWith(null, 0); + expect(buffer).toContain('/test.slice'); + expect(getOpenFds()).toStrictEqual(oldFds); + }); + + testOnlyOnDarwin('cgroup is not supported on darwin', async () => { expect(() => { new Pty({ command: '/bin/cat', args: ['/proc/self/cgroup'], cgroupPath: '/sys/fs/cgroup/cpu/test.slice', - onExit: (err, exitCode) => { - expect(err).toBeNull(); - expect(exitCode).toBe(0); - }, - }) + onExit: vi.fn(), + }); }).toThrowError(); }); }); From 036ecc0a93e2a332c3cb27f7859e754dfedaba4e Mon Sep 17 00:00:00 2001 From: Jacky Zhao Date: Thu, 24 Oct 2024 15:21:45 -0700 Subject: [PATCH 07/60] kill proc --- tests/index.test.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/index.test.ts b/tests/index.test.ts index 039822f..c4a6e81 100644 --- a/tests/index.test.ts +++ b/tests/index.test.ts @@ -277,6 +277,7 @@ describe( pty.resize({ rows: 60, cols: 100 }); }).not.toThrow(); + process.kill(pty.pid, 'SIGKILL'); await vi.waitFor(() => expect(onExit).toHaveBeenCalledTimes(1)); expect(onExit).toHaveBeenCalledWith(null, 0); }); From dd6356e0bc250870487ef5ec740fd0eeca12d464 Mon Sep 17 00:00:00 2001 From: Jacky Zhao Date: Thu, 24 Oct 2024 15:24:45 -0700 Subject: [PATCH 08/60] fix exit code --- tests/index.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/index.test.ts b/tests/index.test.ts index c4a6e81..6b17e42 100644 --- a/tests/index.test.ts +++ b/tests/index.test.ts @@ -279,7 +279,7 @@ describe( process.kill(pty.pid, 'SIGKILL'); await vi.waitFor(() => expect(onExit).toHaveBeenCalledTimes(1)); - expect(onExit).toHaveBeenCalledWith(null, 0); + expect(onExit).toHaveBeenCalledWith(null, -1); }); test( From f482437491527d089aae1e2ddbb904941e9258eb Mon Sep 17 00:00:00 2001 From: Jacky Zhao Date: Thu, 24 Oct 2024 15:32:44 -0700 Subject: [PATCH 09/60] small test changes --- tests/index.test.ts | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/tests/index.test.ts b/tests/index.test.ts index 6b17e42..717dc8b 100644 --- a/tests/index.test.ts +++ b/tests/index.test.ts @@ -1,7 +1,7 @@ import { Pty, getCloseOnExec, setCloseOnExec } from '../wrapper'; import { type Writable } from 'stream'; import { readdirSync, readlinkSync } from 'fs'; -import { describe, test, expect, beforeEach, afterEach, vi } from 'vitest'; +import { describe, test, expect, beforeEach, afterEach, vi, type Mock } from 'vitest'; import { exec as execAsync } from 'child_process'; import { promisify } from 'util'; const exec = promisify(execAsync); @@ -93,7 +93,6 @@ describe( const oldFds = getOpenFds(); const message = 'hello cat\n'; let buffer = ''; - const expectedResult = 'hello cat\r\nhello cat\r\n'; const onExit = vi.fn(); const pty = new Pty({ @@ -119,6 +118,8 @@ describe( // Darwin adds the visible EOT to the stream. result = result.replace('^D\b\b', ''); } + + const expectedResult = 'hello cat\r\nhello cat\r\n'; expect(result.trim()).toStrictEqual(expectedResult.trim()); expect(getOpenFds()).toStrictEqual(oldFds); }); @@ -126,7 +127,6 @@ describe( test('can be started in non-interactive fashion', async () => { const oldFds = getOpenFds(); let buffer = ''; - const expectedResult = '\r\n'; const onExit = vi.fn(); const pty = new Pty({ @@ -143,6 +143,7 @@ describe( await vi.waitFor(() => expect(onExit).toHaveBeenCalledTimes(1)); expect(onExit).toHaveBeenCalledWith(null, 0); let result = buffer.toString(); + const expectedResult = '\r\n'; expect(result.trim()).toStrictEqual(expectedResult.trim()); expect(getOpenFds()).toStrictEqual(oldFds); }); @@ -383,7 +384,7 @@ describe( const oldFds = getOpenFds(); const writeStreams: Array = []; const buffers: Array = []; - const onExits: Array = []; + const onExits: Array = []; const expectedResult = 'hello cat\r\nhello cat\r\n'; // Create 10 concurrent shells From 1f65122b325fe7b860c4b179bfe32a3c9893cb86 Mon Sep 17 00:00:00 2001 From: Jacky Zhao Date: Thu, 24 Oct 2024 15:35:01 -0700 Subject: [PATCH 10/60] 1000 repeats --- tests/index.test.ts | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/tests/index.test.ts b/tests/index.test.ts index 717dc8b..6dce387 100644 --- a/tests/index.test.ts +++ b/tests/index.test.ts @@ -47,7 +47,7 @@ function getOpenFds(): FdRecord { describe( 'PTY', - { repeats: 50 }, + { repeats: 1000 }, () => { test('spawns and exits', async () => { const oldFds = getOpenFds(); @@ -285,7 +285,6 @@ describe( test( 'ordering is correct', - { repeats: 100 }, async () => { const oldFds = getOpenFds(); let buffer = Buffer.from(''); From 58633130f6a44caf1c4a86323fa7ba0c14d42266 Mon Sep 17 00:00:00 2001 From: Jacky Zhao Date: Thu, 24 Oct 2024 15:44:42 -0700 Subject: [PATCH 11/60] no console --- wrapper.ts | 6 ------ 1 file changed, 6 deletions(-) diff --git a/wrapper.ts b/wrapper.ts index 711da62..87f84db 100644 --- a/wrapper.ts +++ b/wrapper.ts @@ -73,7 +73,6 @@ export class Pty { markReadFinished = resolve; }); const mockedExit = (error: NodeJS.ErrnoException | null, code: number) => { - console.log('mocked exit') markExited({ error, code }); }; @@ -99,20 +98,16 @@ export class Pty { this.#handledEndOfData = true; // must wait for fd close and exit result before calling real exit - console.log('handle end') await readFinished; const result = await exitResult; realExit(result.error, result.code); - console.log('done') }; this.read.on('end', () => { - console.log('end') markReadFinished(); }); this.read.on('close', () => { - console.log('close') handleClose(); }); @@ -132,7 +127,6 @@ export class Pty { // is nothing left to read and we can start tearing things down. If we hadn't received an // error so far, we are considered to be in good standing. this.read.off('error', handleError); - console.log('eio') this.#socket.emit('end'); return; } From 76daf08559c8c5ffead5e0fb9f1cb9d1e0f80bd0 Mon Sep 17 00:00:00 2001 From: Jacky Zhao Date: Thu, 24 Oct 2024 15:59:17 -0700 Subject: [PATCH 12/60] 100 repeats --- tests/index.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/index.test.ts b/tests/index.test.ts index 6dce387..c880b98 100644 --- a/tests/index.test.ts +++ b/tests/index.test.ts @@ -47,7 +47,7 @@ function getOpenFds(): FdRecord { describe( 'PTY', - { repeats: 1000 }, + { repeats: 100 }, () => { test('spawns and exits', async () => { const oldFds = getOpenFds(); From 326a9dac2a13aa306456fb9fc1704716b915b0f3 Mon Sep 17 00:00:00 2001 From: Jacky Zhao Date: Thu, 24 Oct 2024 16:02:32 -0700 Subject: [PATCH 13/60] readd log --- wrapper.ts | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/wrapper.ts b/wrapper.ts index 87f84db..711da62 100644 --- a/wrapper.ts +++ b/wrapper.ts @@ -73,6 +73,7 @@ export class Pty { markReadFinished = resolve; }); const mockedExit = (error: NodeJS.ErrnoException | null, code: number) => { + console.log('mocked exit') markExited({ error, code }); }; @@ -98,16 +99,20 @@ export class Pty { this.#handledEndOfData = true; // must wait for fd close and exit result before calling real exit + console.log('handle end') await readFinished; const result = await exitResult; realExit(result.error, result.code); + console.log('done') }; this.read.on('end', () => { + console.log('end') markReadFinished(); }); this.read.on('close', () => { + console.log('close') handleClose(); }); @@ -127,6 +132,7 @@ export class Pty { // is nothing left to read and we can start tearing things down. If we hadn't received an // error so far, we are considered to be in good standing. this.read.off('error', handleError); + console.log('eio') this.#socket.emit('end'); return; } From 51e30727b937256046470f59c12611a756e5e361 Mon Sep 17 00:00:00 2001 From: Jacky Zhao Date: Thu, 24 Oct 2024 16:29:17 -0700 Subject: [PATCH 14/60] modify canon settings --- index.d.ts | 32 +++--- index.js | 263 +++++++++++++++++++++----------------------- src/lib.rs | 57 +++++++++- tests/index.test.ts | 2 +- 4 files changed, 198 insertions(+), 156 deletions(-) diff --git a/index.d.ts b/index.d.ts index 5aeee45..544fd76 100644 --- a/index.d.ts +++ b/index.d.ts @@ -5,40 +5,40 @@ /** The options that can be passed to the constructor of Pty. */ export interface PtyOptions { - command: string; - args?: Array; - envs?: Record; - dir?: string; - size?: Size; - cgroupPath?: string; - interactive?: boolean; - onExit: (err: null | Error, exitCode: number) => void; + command: string + args?: Array + envs?: Record + dir?: string + size?: Size + cgroupPath?: string + interactive?: boolean + onExit: (err: null | Error, exitCode: number) => void } /** A size struct to pass to resize. */ export interface Size { - cols: number; - rows: number; + cols: number + rows: number } /** Resize the terminal. */ -export function ptyResize(fd: number, size: Size): void; +export function ptyResize(fd: number, size: Size): void /** * Set the close-on-exec flag on a file descriptor. This is `fcntl(fd, F_SETFD, FD_CLOEXEC)` under * the covers. */ -export function setCloseOnExec(fd: number, closeOnExec: boolean): void; +export function setCloseOnExec(fd: number, closeOnExec: boolean): void /** * Get the close-on-exec flag on a file descriptor. This is `fcntl(fd, F_GETFD) & FD_CLOEXEC == *_CLOEXEC` under the covers. */ -export function getCloseOnExec(fd: number): boolean; +export function getCloseOnExec(fd: number): boolean export class Pty { /** The pid of the forked process. */ - pid: number; - constructor(opts: PtyOptions); + pid: number + constructor(opts: PtyOptions) /** * Transfers ownership of the file descriptor for the PTY controller. This can only be called * once (it will error the second time). The caller is responsible for closing the file * descriptor. */ - takeFd(): c_int; + takeFd(): c_int } diff --git a/index.js b/index.js index 824d2fb..43de6ae 100644 --- a/index.js +++ b/index.js @@ -5,31 +5,26 @@ /* auto-generated by NAPI-RS */ const { existsSync, readFileSync } = require('fs') -const { join } = require('path'); -const { createRequire } = require('node:module'); -require = createRequire(__filename); +const { join } = require('path') -const { platform, arch } = process; +const { platform, arch } = process -let nativeBinding = null; -let localFileExisted = false; -let loadError = null; +let nativeBinding = null +let localFileExisted = false +let loadError = null function isMusl() { // For Node 10 if (!process.report || typeof process.report.getReport !== 'function') { try { - const lddPath = require('child_process') - .execSync('which ldd') - .toString() - .trim(); - return readFileSync(lddPath, 'utf8').includes('musl'); + const lddPath = require('child_process').execSync('which ldd').toString().trim() + return readFileSync(lddPath, 'utf8').includes('musl') } catch (e) { - return true; + return true } } else { - const { glibcVersionRuntime } = process.report.getReport().header; - return !glibcVersionRuntime; + const { glibcVersionRuntime } = process.report.getReport().header + return !glibcVersionRuntime } } @@ -37,295 +32,287 @@ switch (platform) { case 'android': switch (arch) { case 'arm64': - localFileExisted = existsSync( - join(__dirname, 'ruspty.android-arm64.node'), - ); + localFileExisted = existsSync(join(__dirname, 'ruspty.android-arm64.node')) try { if (localFileExisted) { - nativeBinding = require('./ruspty.android-arm64.node'); + nativeBinding = require('./ruspty.android-arm64.node') } else { - nativeBinding = require('@replit/ruspty-android-arm64'); + nativeBinding = require('@replit/ruspty-android-arm64') } } catch (e) { - loadError = e; + loadError = e } - break; + break case 'arm': - localFileExisted = existsSync( - join(__dirname, 'ruspty.android-arm-eabi.node'), - ); + localFileExisted = existsSync(join(__dirname, 'ruspty.android-arm-eabi.node')) try { if (localFileExisted) { - nativeBinding = require('./ruspty.android-arm-eabi.node'); + nativeBinding = require('./ruspty.android-arm-eabi.node') } else { - nativeBinding = require('@replit/ruspty-android-arm-eabi'); + nativeBinding = require('@replit/ruspty-android-arm-eabi') } } catch (e) { - loadError = e; + loadError = e } - break; + break default: - throw new Error(`Unsupported architecture on Android ${arch}`); + throw new Error(`Unsupported architecture on Android ${arch}`) } - break; + break case 'win32': switch (arch) { case 'x64': localFileExisted = existsSync( - join(__dirname, 'ruspty.win32-x64-msvc.node'), - ); + join(__dirname, 'ruspty.win32-x64-msvc.node') + ) try { if (localFileExisted) { - nativeBinding = require('./ruspty.win32-x64-msvc.node'); + nativeBinding = require('./ruspty.win32-x64-msvc.node') } else { - nativeBinding = require('@replit/ruspty-win32-x64-msvc'); + nativeBinding = require('@replit/ruspty-win32-x64-msvc') } } catch (e) { - loadError = e; + loadError = e } - break; + break case 'ia32': localFileExisted = existsSync( - join(__dirname, 'ruspty.win32-ia32-msvc.node'), - ); + join(__dirname, 'ruspty.win32-ia32-msvc.node') + ) try { if (localFileExisted) { - nativeBinding = require('./ruspty.win32-ia32-msvc.node'); + nativeBinding = require('./ruspty.win32-ia32-msvc.node') } else { - nativeBinding = require('@replit/ruspty-win32-ia32-msvc'); + nativeBinding = require('@replit/ruspty-win32-ia32-msvc') } } catch (e) { - loadError = e; + loadError = e } - break; + break case 'arm64': localFileExisted = existsSync( - join(__dirname, 'ruspty.win32-arm64-msvc.node'), - ); + join(__dirname, 'ruspty.win32-arm64-msvc.node') + ) try { if (localFileExisted) { - nativeBinding = require('./ruspty.win32-arm64-msvc.node'); + nativeBinding = require('./ruspty.win32-arm64-msvc.node') } else { - nativeBinding = require('@replit/ruspty-win32-arm64-msvc'); + nativeBinding = require('@replit/ruspty-win32-arm64-msvc') } } catch (e) { - loadError = e; + loadError = e } - break; + break default: - throw new Error(`Unsupported architecture on Windows: ${arch}`); + throw new Error(`Unsupported architecture on Windows: ${arch}`) } - break; + break case 'darwin': - localFileExisted = existsSync( - join(__dirname, 'ruspty.darwin-universal.node'), - ); + localFileExisted = existsSync(join(__dirname, 'ruspty.darwin-universal.node')) try { if (localFileExisted) { - nativeBinding = require('./ruspty.darwin-universal.node'); + nativeBinding = require('./ruspty.darwin-universal.node') } else { - nativeBinding = require('@replit/ruspty-darwin-universal'); + nativeBinding = require('@replit/ruspty-darwin-universal') } - break; + break } catch {} switch (arch) { case 'x64': - localFileExisted = existsSync( - join(__dirname, 'ruspty.darwin-x64.node'), - ); + localFileExisted = existsSync(join(__dirname, 'ruspty.darwin-x64.node')) try { if (localFileExisted) { - nativeBinding = require('./ruspty.darwin-x64.node'); + nativeBinding = require('./ruspty.darwin-x64.node') } else { - nativeBinding = require('@replit/ruspty-darwin-x64'); + nativeBinding = require('@replit/ruspty-darwin-x64') } } catch (e) { - loadError = e; + loadError = e } - break; + break case 'arm64': localFileExisted = existsSync( - join(__dirname, 'ruspty.darwin-arm64.node'), - ); + join(__dirname, 'ruspty.darwin-arm64.node') + ) try { if (localFileExisted) { - nativeBinding = require('./ruspty.darwin-arm64.node'); + nativeBinding = require('./ruspty.darwin-arm64.node') } else { - nativeBinding = require('@replit/ruspty-darwin-arm64'); + nativeBinding = require('@replit/ruspty-darwin-arm64') } } catch (e) { - loadError = e; + loadError = e } - break; + break default: - throw new Error(`Unsupported architecture on macOS: ${arch}`); + throw new Error(`Unsupported architecture on macOS: ${arch}`) } - break; + break case 'freebsd': if (arch !== 'x64') { - throw new Error(`Unsupported architecture on FreeBSD: ${arch}`); + throw new Error(`Unsupported architecture on FreeBSD: ${arch}`) } - localFileExisted = existsSync(join(__dirname, 'ruspty.freebsd-x64.node')); + localFileExisted = existsSync(join(__dirname, 'ruspty.freebsd-x64.node')) try { if (localFileExisted) { - nativeBinding = require('./ruspty.freebsd-x64.node'); + nativeBinding = require('./ruspty.freebsd-x64.node') } else { - nativeBinding = require('@replit/ruspty-freebsd-x64'); + nativeBinding = require('@replit/ruspty-freebsd-x64') } } catch (e) { - loadError = e; + loadError = e } - break; + break case 'linux': switch (arch) { case 'x64': if (isMusl()) { localFileExisted = existsSync( - join(__dirname, 'ruspty.linux-x64-musl.node'), - ); + join(__dirname, 'ruspty.linux-x64-musl.node') + ) try { if (localFileExisted) { - nativeBinding = require('./ruspty.linux-x64-musl.node'); + nativeBinding = require('./ruspty.linux-x64-musl.node') } else { - nativeBinding = require('@replit/ruspty-linux-x64-musl'); + nativeBinding = require('@replit/ruspty-linux-x64-musl') } } catch (e) { - loadError = e; + loadError = e } } else { localFileExisted = existsSync( - join(__dirname, 'ruspty.linux-x64-gnu.node'), - ); + join(__dirname, 'ruspty.linux-x64-gnu.node') + ) try { if (localFileExisted) { - nativeBinding = require('./ruspty.linux-x64-gnu.node'); + nativeBinding = require('./ruspty.linux-x64-gnu.node') } else { - nativeBinding = require('@replit/ruspty-linux-x64-gnu'); + nativeBinding = require('@replit/ruspty-linux-x64-gnu') } } catch (e) { - loadError = e; + loadError = e } } - break; + break case 'arm64': if (isMusl()) { localFileExisted = existsSync( - join(__dirname, 'ruspty.linux-arm64-musl.node'), - ); + join(__dirname, 'ruspty.linux-arm64-musl.node') + ) try { if (localFileExisted) { - nativeBinding = require('./ruspty.linux-arm64-musl.node'); + nativeBinding = require('./ruspty.linux-arm64-musl.node') } else { - nativeBinding = require('@replit/ruspty-linux-arm64-musl'); + nativeBinding = require('@replit/ruspty-linux-arm64-musl') } } catch (e) { - loadError = e; + loadError = e } } else { localFileExisted = existsSync( - join(__dirname, 'ruspty.linux-arm64-gnu.node'), - ); + join(__dirname, 'ruspty.linux-arm64-gnu.node') + ) try { if (localFileExisted) { - nativeBinding = require('./ruspty.linux-arm64-gnu.node'); + nativeBinding = require('./ruspty.linux-arm64-gnu.node') } else { - nativeBinding = require('@replit/ruspty-linux-arm64-gnu'); + nativeBinding = require('@replit/ruspty-linux-arm64-gnu') } } catch (e) { - loadError = e; + loadError = e } } - break; + break case 'arm': if (isMusl()) { localFileExisted = existsSync( - join(__dirname, 'ruspty.linux-arm-musleabihf.node'), - ); + join(__dirname, 'ruspty.linux-arm-musleabihf.node') + ) try { if (localFileExisted) { - nativeBinding = require('./ruspty.linux-arm-musleabihf.node'); + nativeBinding = require('./ruspty.linux-arm-musleabihf.node') } else { - nativeBinding = require('@replit/ruspty-linux-arm-musleabihf'); + nativeBinding = require('@replit/ruspty-linux-arm-musleabihf') } } catch (e) { - loadError = e; + loadError = e } } else { localFileExisted = existsSync( - join(__dirname, 'ruspty.linux-arm-gnueabihf.node'), - ); + join(__dirname, 'ruspty.linux-arm-gnueabihf.node') + ) try { if (localFileExisted) { - nativeBinding = require('./ruspty.linux-arm-gnueabihf.node'); + nativeBinding = require('./ruspty.linux-arm-gnueabihf.node') } else { - nativeBinding = require('@replit/ruspty-linux-arm-gnueabihf'); + nativeBinding = require('@replit/ruspty-linux-arm-gnueabihf') } } catch (e) { - loadError = e; + loadError = e } } - break; + break case 'riscv64': if (isMusl()) { localFileExisted = existsSync( - join(__dirname, 'ruspty.linux-riscv64-musl.node'), - ); + join(__dirname, 'ruspty.linux-riscv64-musl.node') + ) try { if (localFileExisted) { - nativeBinding = require('./ruspty.linux-riscv64-musl.node'); + nativeBinding = require('./ruspty.linux-riscv64-musl.node') } else { - nativeBinding = require('@replit/ruspty-linux-riscv64-musl'); + nativeBinding = require('@replit/ruspty-linux-riscv64-musl') } } catch (e) { - loadError = e; + loadError = e } } else { localFileExisted = existsSync( - join(__dirname, 'ruspty.linux-riscv64-gnu.node'), - ); + join(__dirname, 'ruspty.linux-riscv64-gnu.node') + ) try { if (localFileExisted) { - nativeBinding = require('./ruspty.linux-riscv64-gnu.node'); + nativeBinding = require('./ruspty.linux-riscv64-gnu.node') } else { - nativeBinding = require('@replit/ruspty-linux-riscv64-gnu'); + nativeBinding = require('@replit/ruspty-linux-riscv64-gnu') } } catch (e) { - loadError = e; + loadError = e } } - break; + break case 's390x': localFileExisted = existsSync( - join(__dirname, 'ruspty.linux-s390x-gnu.node'), - ); + join(__dirname, 'ruspty.linux-s390x-gnu.node') + ) try { if (localFileExisted) { - nativeBinding = require('./ruspty.linux-s390x-gnu.node'); + nativeBinding = require('./ruspty.linux-s390x-gnu.node') } else { - nativeBinding = require('@replit/ruspty-linux-s390x-gnu'); + nativeBinding = require('@replit/ruspty-linux-s390x-gnu') } } catch (e) { - loadError = e; + loadError = e } - break; + break default: - throw new Error(`Unsupported architecture on Linux: ${arch}`); + throw new Error(`Unsupported architecture on Linux: ${arch}`) } - break; + break default: - throw new Error(`Unsupported OS: ${platform}, architecture: ${arch}`); + throw new Error(`Unsupported OS: ${platform}, architecture: ${arch}`) } if (!nativeBinding) { if (loadError) { - throw loadError; + throw loadError } - throw new Error(`Failed to load native binding`); + throw new Error(`Failed to load native binding`) } -const { Pty, ptyResize, setCloseOnExec, getCloseOnExec } = nativeBinding; +const { Pty, ptyResize, setCloseOnExec, getCloseOnExec } = nativeBinding -module.exports.Pty = Pty; -module.exports.ptyResize = ptyResize; -module.exports.setCloseOnExec = setCloseOnExec; -module.exports.getCloseOnExec = getCloseOnExec; +module.exports.Pty = Pty +module.exports.ptyResize = ptyResize +module.exports.setCloseOnExec = setCloseOnExec +module.exports.getCloseOnExec = getCloseOnExec diff --git a/src/lib.rs b/src/lib.rs index beb2ab2..5b00cf5 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -203,7 +203,62 @@ impl Pty { // set input modes let user_fd = OwnedFd::from_raw_fd(raw_user_fd); if let Ok(mut termios) = termios::tcgetattr(&user_fd) { - termios.input_flags |= termios::InputFlags::IUTF8; + // Input flags + termios.input_flags = termios::InputFlags::ICRNL // Map CR to NL on input + | termios::InputFlags::IXON // Enable XON/XOFF flow control on output + | termios::InputFlags::IXANY // Allow any character to restart output + | termios::InputFlags::IMAXBEL // Ring bell when input queue is full + | termios::InputFlags::BRKINT // Send SIGINT on break + | termios::InputFlags::IUTF8; // UTF8 input handling + + // Output flags + termios.output_flags = termios::OutputFlags::OPOST // Post-process output + | termios::OutputFlags::ONLCR; // Map NL to CR-NL on output + + // Control flags + termios.control_flags = termios::ControlFlags::CREAD // Enable receiver + | termios::ControlFlags::CS8 // 8 bit chars + | termios::ControlFlags::HUPCL; // Hangup on last close + + // Local flags + termios.local_flags = termios::LocalFlags::ICANON // Enable canonical mode + | termios::LocalFlags::ISIG // Enable signals + | termios::LocalFlags::IEXTEN // Enable extended functions + | termios::LocalFlags::ECHO // Enable echo + | termios::LocalFlags::ECHOE // Echo erase char as BS-SP-BS + | termios::LocalFlags::ECHOK // Echo kill char + | termios::LocalFlags::ECHOKE // BS-SP-BS entire line on kill + | termios::LocalFlags::ECHOCTL; // Echo control chars as ^X + + // Control chars + termios.control_chars[termios::SpecialCharacterIndices::VEOF as usize] = 4; // ^D + termios.control_chars[termios::SpecialCharacterIndices::VEOL as usize] = 0xff; // disabled + termios.control_chars[termios::SpecialCharacterIndices::VEOL2 as usize] = 0xff; // disabled + termios.control_chars[termios::SpecialCharacterIndices::VERASE as usize] = 0x7f; // ^? + termios.control_chars[termios::SpecialCharacterIndices::VWERASE as usize] = 23; // ^W + termios.control_chars[termios::SpecialCharacterIndices::VKILL as usize] = 21; // ^U + termios.control_chars[termios::SpecialCharacterIndices::VREPRINT as usize] = 18; // ^R + termios.control_chars[termios::SpecialCharacterIndices::VINTR as usize] = 3; // ^C + termios.control_chars[termios::SpecialCharacterIndices::VQUIT as usize] = 0x1c; // ^\ + termios.control_chars[termios::SpecialCharacterIndices::VSUSP as usize] = 26; // ^Z + termios.control_chars[termios::SpecialCharacterIndices::VSTART as usize] = 17; // ^Q + termios.control_chars[termios::SpecialCharacterIndices::VSTOP as usize] = 19; // ^S + termios.control_chars[termios::SpecialCharacterIndices::VLNEXT as usize] = 22; // ^V + termios.control_chars[termios::SpecialCharacterIndices::VDISCARD as usize] = 15; // ^O + termios.control_chars[termios::SpecialCharacterIndices::VMIN as usize] = 1; + termios.control_chars[termios::SpecialCharacterIndices::VTIME as usize] = 0; + + #[cfg(target_os = "macos")] + { + termios.control_chars[termios::SpecialCharacterIndices::VDSUSP as usize] = 25; // ^Y + termios.control_chars[termios::SpecialCharacterIndices::VSTATUS as usize] = 20; + // ^T + } + + // Set input/output baud rate + termios::cfsetispeed(&mut termios, termios::BaudRate::B38400)?; + termios::cfsetospeed(&mut termios, termios::BaudRate::B38400)?; + termios::tcsetattr(&user_fd, SetArg::TCSANOW, &termios)?; } diff --git a/tests/index.test.ts b/tests/index.test.ts index c880b98..a3de1ed 100644 --- a/tests/index.test.ts +++ b/tests/index.test.ts @@ -47,7 +47,7 @@ function getOpenFds(): FdRecord { describe( 'PTY', - { repeats: 100 }, + // { repeats: 100 }, () => { test('spawns and exits', async () => { const oldFds = getOpenFds(); From 115cd7018ccc4884dc748cb2abdf597e57218210 Mon Sep 17 00:00:00 2001 From: Jacky Zhao Date: Thu, 24 Oct 2024 16:45:16 -0700 Subject: [PATCH 15/60] re-add repeats --- tests/index.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/index.test.ts b/tests/index.test.ts index a3de1ed..c880b98 100644 --- a/tests/index.test.ts +++ b/tests/index.test.ts @@ -47,7 +47,7 @@ function getOpenFds(): FdRecord { describe( 'PTY', - // { repeats: 100 }, + { repeats: 100 }, () => { test('spawns and exits', async () => { const oldFds = getOpenFds(); From e26aa2dad9f4b3762303099257f154005268bfed Mon Sep 17 00:00:00 2001 From: Jacky Zhao Date: Thu, 24 Oct 2024 16:59:02 -0700 Subject: [PATCH 16/60] undo lib changes, format after build --- index.d.ts | 32 +++---- index.js | 261 +++++++++++++++++++++++++++------------------------ package.json | 2 +- src/lib.rs | 57 +---------- wrapper.ts | 12 +-- 5 files changed, 160 insertions(+), 204 deletions(-) diff --git a/index.d.ts b/index.d.ts index 544fd76..5aeee45 100644 --- a/index.d.ts +++ b/index.d.ts @@ -5,40 +5,40 @@ /** The options that can be passed to the constructor of Pty. */ export interface PtyOptions { - command: string - args?: Array - envs?: Record - dir?: string - size?: Size - cgroupPath?: string - interactive?: boolean - onExit: (err: null | Error, exitCode: number) => void + command: string; + args?: Array; + envs?: Record; + dir?: string; + size?: Size; + cgroupPath?: string; + interactive?: boolean; + onExit: (err: null | Error, exitCode: number) => void; } /** A size struct to pass to resize. */ export interface Size { - cols: number - rows: number + cols: number; + rows: number; } /** Resize the terminal. */ -export function ptyResize(fd: number, size: Size): void +export function ptyResize(fd: number, size: Size): void; /** * Set the close-on-exec flag on a file descriptor. This is `fcntl(fd, F_SETFD, FD_CLOEXEC)` under * the covers. */ -export function setCloseOnExec(fd: number, closeOnExec: boolean): void +export function setCloseOnExec(fd: number, closeOnExec: boolean): void; /** * Get the close-on-exec flag on a file descriptor. This is `fcntl(fd, F_GETFD) & FD_CLOEXEC == *_CLOEXEC` under the covers. */ -export function getCloseOnExec(fd: number): boolean +export function getCloseOnExec(fd: number): boolean; export class Pty { /** The pid of the forked process. */ - pid: number - constructor(opts: PtyOptions) + pid: number; + constructor(opts: PtyOptions); /** * Transfers ownership of the file descriptor for the PTY controller. This can only be called * once (it will error the second time). The caller is responsible for closing the file * descriptor. */ - takeFd(): c_int + takeFd(): c_int; } diff --git a/index.js b/index.js index 43de6ae..7ac8050 100644 --- a/index.js +++ b/index.js @@ -5,26 +5,29 @@ /* auto-generated by NAPI-RS */ const { existsSync, readFileSync } = require('fs') -const { join } = require('path') +const { join } = require('path'); -const { platform, arch } = process +const { platform, arch } = process; -let nativeBinding = null -let localFileExisted = false -let loadError = null +let nativeBinding = null; +let localFileExisted = false; +let loadError = null; function isMusl() { // For Node 10 if (!process.report || typeof process.report.getReport !== 'function') { try { - const lddPath = require('child_process').execSync('which ldd').toString().trim() - return readFileSync(lddPath, 'utf8').includes('musl') + const lddPath = require('child_process') + .execSync('which ldd') + .toString() + .trim(); + return readFileSync(lddPath, 'utf8').includes('musl'); } catch (e) { - return true + return true; } } else { - const { glibcVersionRuntime } = process.report.getReport().header - return !glibcVersionRuntime + const { glibcVersionRuntime } = process.report.getReport().header; + return !glibcVersionRuntime; } } @@ -32,287 +35,295 @@ switch (platform) { case 'android': switch (arch) { case 'arm64': - localFileExisted = existsSync(join(__dirname, 'ruspty.android-arm64.node')) + localFileExisted = existsSync( + join(__dirname, 'ruspty.android-arm64.node'), + ); try { if (localFileExisted) { - nativeBinding = require('./ruspty.android-arm64.node') + nativeBinding = require('./ruspty.android-arm64.node'); } else { - nativeBinding = require('@replit/ruspty-android-arm64') + nativeBinding = require('@replit/ruspty-android-arm64'); } } catch (e) { - loadError = e + loadError = e; } - break + break; case 'arm': - localFileExisted = existsSync(join(__dirname, 'ruspty.android-arm-eabi.node')) + localFileExisted = existsSync( + join(__dirname, 'ruspty.android-arm-eabi.node'), + ); try { if (localFileExisted) { - nativeBinding = require('./ruspty.android-arm-eabi.node') + nativeBinding = require('./ruspty.android-arm-eabi.node'); } else { - nativeBinding = require('@replit/ruspty-android-arm-eabi') + nativeBinding = require('@replit/ruspty-android-arm-eabi'); } } catch (e) { - loadError = e + loadError = e; } - break + break; default: - throw new Error(`Unsupported architecture on Android ${arch}`) + throw new Error(`Unsupported architecture on Android ${arch}`); } - break + break; case 'win32': switch (arch) { case 'x64': localFileExisted = existsSync( - join(__dirname, 'ruspty.win32-x64-msvc.node') - ) + join(__dirname, 'ruspty.win32-x64-msvc.node'), + ); try { if (localFileExisted) { - nativeBinding = require('./ruspty.win32-x64-msvc.node') + nativeBinding = require('./ruspty.win32-x64-msvc.node'); } else { - nativeBinding = require('@replit/ruspty-win32-x64-msvc') + nativeBinding = require('@replit/ruspty-win32-x64-msvc'); } } catch (e) { - loadError = e + loadError = e; } - break + break; case 'ia32': localFileExisted = existsSync( - join(__dirname, 'ruspty.win32-ia32-msvc.node') - ) + join(__dirname, 'ruspty.win32-ia32-msvc.node'), + ); try { if (localFileExisted) { - nativeBinding = require('./ruspty.win32-ia32-msvc.node') + nativeBinding = require('./ruspty.win32-ia32-msvc.node'); } else { - nativeBinding = require('@replit/ruspty-win32-ia32-msvc') + nativeBinding = require('@replit/ruspty-win32-ia32-msvc'); } } catch (e) { - loadError = e + loadError = e; } - break + break; case 'arm64': localFileExisted = existsSync( - join(__dirname, 'ruspty.win32-arm64-msvc.node') - ) + join(__dirname, 'ruspty.win32-arm64-msvc.node'), + ); try { if (localFileExisted) { - nativeBinding = require('./ruspty.win32-arm64-msvc.node') + nativeBinding = require('./ruspty.win32-arm64-msvc.node'); } else { - nativeBinding = require('@replit/ruspty-win32-arm64-msvc') + nativeBinding = require('@replit/ruspty-win32-arm64-msvc'); } } catch (e) { - loadError = e + loadError = e; } - break + break; default: - throw new Error(`Unsupported architecture on Windows: ${arch}`) + throw new Error(`Unsupported architecture on Windows: ${arch}`); } - break + break; case 'darwin': - localFileExisted = existsSync(join(__dirname, 'ruspty.darwin-universal.node')) + localFileExisted = existsSync( + join(__dirname, 'ruspty.darwin-universal.node'), + ); try { if (localFileExisted) { - nativeBinding = require('./ruspty.darwin-universal.node') + nativeBinding = require('./ruspty.darwin-universal.node'); } else { - nativeBinding = require('@replit/ruspty-darwin-universal') + nativeBinding = require('@replit/ruspty-darwin-universal'); } - break + break; } catch {} switch (arch) { case 'x64': - localFileExisted = existsSync(join(__dirname, 'ruspty.darwin-x64.node')) + localFileExisted = existsSync( + join(__dirname, 'ruspty.darwin-x64.node'), + ); try { if (localFileExisted) { - nativeBinding = require('./ruspty.darwin-x64.node') + nativeBinding = require('./ruspty.darwin-x64.node'); } else { - nativeBinding = require('@replit/ruspty-darwin-x64') + nativeBinding = require('@replit/ruspty-darwin-x64'); } } catch (e) { - loadError = e + loadError = e; } - break + break; case 'arm64': localFileExisted = existsSync( - join(__dirname, 'ruspty.darwin-arm64.node') - ) + join(__dirname, 'ruspty.darwin-arm64.node'), + ); try { if (localFileExisted) { - nativeBinding = require('./ruspty.darwin-arm64.node') + nativeBinding = require('./ruspty.darwin-arm64.node'); } else { - nativeBinding = require('@replit/ruspty-darwin-arm64') + nativeBinding = require('@replit/ruspty-darwin-arm64'); } } catch (e) { - loadError = e + loadError = e; } - break + break; default: - throw new Error(`Unsupported architecture on macOS: ${arch}`) + throw new Error(`Unsupported architecture on macOS: ${arch}`); } - break + break; case 'freebsd': if (arch !== 'x64') { - throw new Error(`Unsupported architecture on FreeBSD: ${arch}`) + throw new Error(`Unsupported architecture on FreeBSD: ${arch}`); } - localFileExisted = existsSync(join(__dirname, 'ruspty.freebsd-x64.node')) + localFileExisted = existsSync(join(__dirname, 'ruspty.freebsd-x64.node')); try { if (localFileExisted) { - nativeBinding = require('./ruspty.freebsd-x64.node') + nativeBinding = require('./ruspty.freebsd-x64.node'); } else { - nativeBinding = require('@replit/ruspty-freebsd-x64') + nativeBinding = require('@replit/ruspty-freebsd-x64'); } } catch (e) { - loadError = e + loadError = e; } - break + break; case 'linux': switch (arch) { case 'x64': if (isMusl()) { localFileExisted = existsSync( - join(__dirname, 'ruspty.linux-x64-musl.node') - ) + join(__dirname, 'ruspty.linux-x64-musl.node'), + ); try { if (localFileExisted) { - nativeBinding = require('./ruspty.linux-x64-musl.node') + nativeBinding = require('./ruspty.linux-x64-musl.node'); } else { - nativeBinding = require('@replit/ruspty-linux-x64-musl') + nativeBinding = require('@replit/ruspty-linux-x64-musl'); } } catch (e) { - loadError = e + loadError = e; } } else { localFileExisted = existsSync( - join(__dirname, 'ruspty.linux-x64-gnu.node') - ) + join(__dirname, 'ruspty.linux-x64-gnu.node'), + ); try { if (localFileExisted) { - nativeBinding = require('./ruspty.linux-x64-gnu.node') + nativeBinding = require('./ruspty.linux-x64-gnu.node'); } else { - nativeBinding = require('@replit/ruspty-linux-x64-gnu') + nativeBinding = require('@replit/ruspty-linux-x64-gnu'); } } catch (e) { - loadError = e + loadError = e; } } - break + break; case 'arm64': if (isMusl()) { localFileExisted = existsSync( - join(__dirname, 'ruspty.linux-arm64-musl.node') - ) + join(__dirname, 'ruspty.linux-arm64-musl.node'), + ); try { if (localFileExisted) { - nativeBinding = require('./ruspty.linux-arm64-musl.node') + nativeBinding = require('./ruspty.linux-arm64-musl.node'); } else { - nativeBinding = require('@replit/ruspty-linux-arm64-musl') + nativeBinding = require('@replit/ruspty-linux-arm64-musl'); } } catch (e) { - loadError = e + loadError = e; } } else { localFileExisted = existsSync( - join(__dirname, 'ruspty.linux-arm64-gnu.node') - ) + join(__dirname, 'ruspty.linux-arm64-gnu.node'), + ); try { if (localFileExisted) { - nativeBinding = require('./ruspty.linux-arm64-gnu.node') + nativeBinding = require('./ruspty.linux-arm64-gnu.node'); } else { - nativeBinding = require('@replit/ruspty-linux-arm64-gnu') + nativeBinding = require('@replit/ruspty-linux-arm64-gnu'); } } catch (e) { - loadError = e + loadError = e; } } - break + break; case 'arm': if (isMusl()) { localFileExisted = existsSync( - join(__dirname, 'ruspty.linux-arm-musleabihf.node') - ) + join(__dirname, 'ruspty.linux-arm-musleabihf.node'), + ); try { if (localFileExisted) { - nativeBinding = require('./ruspty.linux-arm-musleabihf.node') + nativeBinding = require('./ruspty.linux-arm-musleabihf.node'); } else { - nativeBinding = require('@replit/ruspty-linux-arm-musleabihf') + nativeBinding = require('@replit/ruspty-linux-arm-musleabihf'); } } catch (e) { - loadError = e + loadError = e; } } else { localFileExisted = existsSync( - join(__dirname, 'ruspty.linux-arm-gnueabihf.node') - ) + join(__dirname, 'ruspty.linux-arm-gnueabihf.node'), + ); try { if (localFileExisted) { - nativeBinding = require('./ruspty.linux-arm-gnueabihf.node') + nativeBinding = require('./ruspty.linux-arm-gnueabihf.node'); } else { - nativeBinding = require('@replit/ruspty-linux-arm-gnueabihf') + nativeBinding = require('@replit/ruspty-linux-arm-gnueabihf'); } } catch (e) { - loadError = e + loadError = e; } } - break + break; case 'riscv64': if (isMusl()) { localFileExisted = existsSync( - join(__dirname, 'ruspty.linux-riscv64-musl.node') - ) + join(__dirname, 'ruspty.linux-riscv64-musl.node'), + ); try { if (localFileExisted) { - nativeBinding = require('./ruspty.linux-riscv64-musl.node') + nativeBinding = require('./ruspty.linux-riscv64-musl.node'); } else { - nativeBinding = require('@replit/ruspty-linux-riscv64-musl') + nativeBinding = require('@replit/ruspty-linux-riscv64-musl'); } } catch (e) { - loadError = e + loadError = e; } } else { localFileExisted = existsSync( - join(__dirname, 'ruspty.linux-riscv64-gnu.node') - ) + join(__dirname, 'ruspty.linux-riscv64-gnu.node'), + ); try { if (localFileExisted) { - nativeBinding = require('./ruspty.linux-riscv64-gnu.node') + nativeBinding = require('./ruspty.linux-riscv64-gnu.node'); } else { - nativeBinding = require('@replit/ruspty-linux-riscv64-gnu') + nativeBinding = require('@replit/ruspty-linux-riscv64-gnu'); } } catch (e) { - loadError = e + loadError = e; } } - break + break; case 's390x': localFileExisted = existsSync( - join(__dirname, 'ruspty.linux-s390x-gnu.node') - ) + join(__dirname, 'ruspty.linux-s390x-gnu.node'), + ); try { if (localFileExisted) { - nativeBinding = require('./ruspty.linux-s390x-gnu.node') + nativeBinding = require('./ruspty.linux-s390x-gnu.node'); } else { - nativeBinding = require('@replit/ruspty-linux-s390x-gnu') + nativeBinding = require('@replit/ruspty-linux-s390x-gnu'); } } catch (e) { - loadError = e + loadError = e; } - break + break; default: - throw new Error(`Unsupported architecture on Linux: ${arch}`) + throw new Error(`Unsupported architecture on Linux: ${arch}`); } - break + break; default: - throw new Error(`Unsupported OS: ${platform}, architecture: ${arch}`) + throw new Error(`Unsupported OS: ${platform}, architecture: ${arch}`); } if (!nativeBinding) { if (loadError) { - throw loadError + throw loadError; } - throw new Error(`Failed to load native binding`) + throw new Error(`Failed to load native binding`); } -const { Pty, ptyResize, setCloseOnExec, getCloseOnExec } = nativeBinding +const { Pty, ptyResize, setCloseOnExec, getCloseOnExec } = nativeBinding; -module.exports.Pty = Pty -module.exports.ptyResize = ptyResize -module.exports.setCloseOnExec = setCloseOnExec -module.exports.getCloseOnExec = getCloseOnExec +module.exports.Pty = Pty; +module.exports.ptyResize = ptyResize; +module.exports.setCloseOnExec = setCloseOnExec; +module.exports.getCloseOnExec = getCloseOnExec; diff --git a/package.json b/package.json index 2f1b96c..338344b 100644 --- a/package.json +++ b/package.json @@ -36,7 +36,7 @@ }, "scripts": { "artifacts": "napi artifacts", - "build": "napi build --platform --release && npm run build:wrapper", + "build": "napi build --platform --release && npm run build:wrapper && npm run format", "build:wrapper": "tsup", "prepublishOnly": "napi prepublish -t npm", "test": "vitest run", diff --git a/src/lib.rs b/src/lib.rs index 5b00cf5..beb2ab2 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -203,62 +203,7 @@ impl Pty { // set input modes let user_fd = OwnedFd::from_raw_fd(raw_user_fd); if let Ok(mut termios) = termios::tcgetattr(&user_fd) { - // Input flags - termios.input_flags = termios::InputFlags::ICRNL // Map CR to NL on input - | termios::InputFlags::IXON // Enable XON/XOFF flow control on output - | termios::InputFlags::IXANY // Allow any character to restart output - | termios::InputFlags::IMAXBEL // Ring bell when input queue is full - | termios::InputFlags::BRKINT // Send SIGINT on break - | termios::InputFlags::IUTF8; // UTF8 input handling - - // Output flags - termios.output_flags = termios::OutputFlags::OPOST // Post-process output - | termios::OutputFlags::ONLCR; // Map NL to CR-NL on output - - // Control flags - termios.control_flags = termios::ControlFlags::CREAD // Enable receiver - | termios::ControlFlags::CS8 // 8 bit chars - | termios::ControlFlags::HUPCL; // Hangup on last close - - // Local flags - termios.local_flags = termios::LocalFlags::ICANON // Enable canonical mode - | termios::LocalFlags::ISIG // Enable signals - | termios::LocalFlags::IEXTEN // Enable extended functions - | termios::LocalFlags::ECHO // Enable echo - | termios::LocalFlags::ECHOE // Echo erase char as BS-SP-BS - | termios::LocalFlags::ECHOK // Echo kill char - | termios::LocalFlags::ECHOKE // BS-SP-BS entire line on kill - | termios::LocalFlags::ECHOCTL; // Echo control chars as ^X - - // Control chars - termios.control_chars[termios::SpecialCharacterIndices::VEOF as usize] = 4; // ^D - termios.control_chars[termios::SpecialCharacterIndices::VEOL as usize] = 0xff; // disabled - termios.control_chars[termios::SpecialCharacterIndices::VEOL2 as usize] = 0xff; // disabled - termios.control_chars[termios::SpecialCharacterIndices::VERASE as usize] = 0x7f; // ^? - termios.control_chars[termios::SpecialCharacterIndices::VWERASE as usize] = 23; // ^W - termios.control_chars[termios::SpecialCharacterIndices::VKILL as usize] = 21; // ^U - termios.control_chars[termios::SpecialCharacterIndices::VREPRINT as usize] = 18; // ^R - termios.control_chars[termios::SpecialCharacterIndices::VINTR as usize] = 3; // ^C - termios.control_chars[termios::SpecialCharacterIndices::VQUIT as usize] = 0x1c; // ^\ - termios.control_chars[termios::SpecialCharacterIndices::VSUSP as usize] = 26; // ^Z - termios.control_chars[termios::SpecialCharacterIndices::VSTART as usize] = 17; // ^Q - termios.control_chars[termios::SpecialCharacterIndices::VSTOP as usize] = 19; // ^S - termios.control_chars[termios::SpecialCharacterIndices::VLNEXT as usize] = 22; // ^V - termios.control_chars[termios::SpecialCharacterIndices::VDISCARD as usize] = 15; // ^O - termios.control_chars[termios::SpecialCharacterIndices::VMIN as usize] = 1; - termios.control_chars[termios::SpecialCharacterIndices::VTIME as usize] = 0; - - #[cfg(target_os = "macos")] - { - termios.control_chars[termios::SpecialCharacterIndices::VDSUSP as usize] = 25; // ^Y - termios.control_chars[termios::SpecialCharacterIndices::VSTATUS as usize] = 20; - // ^T - } - - // Set input/output baud rate - termios::cfsetispeed(&mut termios, termios::BaudRate::B38400)?; - termios::cfsetospeed(&mut termios, termios::BaudRate::B38400)?; - + termios.input_flags |= termios::InputFlags::IUTF8; termios::tcsetattr(&user_fd, SetArg::TCSANOW, &termios)?; } diff --git a/wrapper.ts b/wrapper.ts index 711da62..0a9020c 100644 --- a/wrapper.ts +++ b/wrapper.ts @@ -73,7 +73,7 @@ export class Pty { markReadFinished = resolve; }); const mockedExit = (error: NodeJS.ErrnoException | null, code: number) => { - console.log('mocked exit') + console.log('mocked exit'); markExited({ error, code }); }; @@ -99,20 +99,20 @@ export class Pty { this.#handledEndOfData = true; // must wait for fd close and exit result before calling real exit - console.log('handle end') + console.log('handle end'); await readFinished; const result = await exitResult; realExit(result.error, result.code); - console.log('done') + console.log('done'); }; this.read.on('end', () => { - console.log('end') + console.log('end'); markReadFinished(); }); this.read.on('close', () => { - console.log('close') + console.log('close'); handleClose(); }); @@ -132,7 +132,7 @@ export class Pty { // is nothing left to read and we can start tearing things down. If we hadn't received an // error so far, we are considered to be in good standing. this.read.off('error', handleError); - console.log('eio') + console.log('eio'); this.#socket.emit('end'); return; } From 28d0f2abc6bc8ebd73a740c53ff60e802691d349 Mon Sep 17 00:00:00 2001 From: Jacky Zhao Date: Thu, 24 Oct 2024 17:14:24 -0700 Subject: [PATCH 17/60] different ordering test --- tests/index.test.ts | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/tests/index.test.ts b/tests/index.test.ts index c880b98..73178e0 100644 --- a/tests/index.test.ts +++ b/tests/index.test.ts @@ -47,7 +47,7 @@ function getOpenFds(): FdRecord { describe( 'PTY', - { repeats: 100 }, + { repeats: 500 }, () => { test('spawns and exits', async () => { const oldFds = getOpenFds(); @@ -283,7 +283,7 @@ describe( expect(onExit).toHaveBeenCalledWith(null, -1); }); - test( + test.only( 'ordering is correct', async () => { const oldFds = getOpenFds(); @@ -307,9 +307,12 @@ describe( await vi.waitFor(() => expect(onExit).toHaveBeenCalledTimes(1)); expect(onExit).toHaveBeenCalledWith(null, 0); - expect(buffer.toString().trim().split('\n').map(Number)).toStrictEqual( - Array.from({ length: n + 1 }, (_, i) => i), - ); + + const lines = buffer.toString().trim().split('\n'); + for (let i = 0; i < n + 1; i++) { + expect(Number(lines[i])).toBe(i); + } + expect(getOpenFds()).toStrictEqual(oldFds); } ); From 2c725bf11906dd2aa3b943f77831827ad98fcab6 Mon Sep 17 00:00:00 2001 From: Jacky Zhao Date: Thu, 24 Oct 2024 17:16:08 -0700 Subject: [PATCH 18/60] allow only --- package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/package.json b/package.json index 338344b..cf90207 100644 --- a/package.json +++ b/package.json @@ -39,7 +39,7 @@ "build": "napi build --platform --release && npm run build:wrapper && npm run format", "build:wrapper": "tsup", "prepublishOnly": "napi prepublish -t npm", - "test": "vitest run", + "test": "vitest run --allowOnly", "test:hang": "vitest run --reporter=hanging-process", "universal": "napi universal", "version": "napi version", From 91925baa263c569d15ac372933b042a6d351780c Mon Sep 17 00:00:00 2001 From: Jacky Zhao Date: Thu, 24 Oct 2024 17:22:15 -0700 Subject: [PATCH 19/60] better log --- tests/index.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/index.test.ts b/tests/index.test.ts index 73178e0..b90e58e 100644 --- a/tests/index.test.ts +++ b/tests/index.test.ts @@ -310,7 +310,7 @@ describe( const lines = buffer.toString().trim().split('\n'); for (let i = 0; i < n + 1; i++) { - expect(Number(lines[i])).toBe(i); + expect(Number(lines[i]), `expected line ${i} to contain ${i} but got ${lines[i]}`).toBe(i); } expect(getOpenFds()).toStrictEqual(oldFds); From c67fe7527919da27ce01cc2bc982d16ebfd15088 Mon Sep 17 00:00:00 2001 From: Jacky Zhao Date: Thu, 24 Oct 2024 17:25:39 -0700 Subject: [PATCH 20/60] line no check --- tests/index.test.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/index.test.ts b/tests/index.test.ts index b90e58e..08b4898 100644 --- a/tests/index.test.ts +++ b/tests/index.test.ts @@ -295,7 +295,7 @@ describe( command: '/bin/sh', args: [ '-c', - `seq 0 ${n}` + `seq 0 ${n} && exit` ], onExit, }); @@ -309,6 +309,7 @@ describe( expect(onExit).toHaveBeenCalledWith(null, 0); const lines = buffer.toString().trim().split('\n'); + expect(lines.length).toBe(n + 1); for (let i = 0; i < n + 1; i++) { expect(Number(lines[i]), `expected line ${i} to contain ${i} but got ${lines[i]}`).toBe(i); } From d5cfe4ca8fa8b10cccf03344bbf223aafc18a84c Mon Sep 17 00:00:00 2001 From: Jacky Zhao Date: Thu, 24 Oct 2024 17:31:26 -0700 Subject: [PATCH 21/60] more logging changes --- tests/index.test.ts | 1 + wrapper.ts | 14 ++++---------- 2 files changed, 5 insertions(+), 10 deletions(-) diff --git a/tests/index.test.ts b/tests/index.test.ts index 08b4898..94f03fa 100644 --- a/tests/index.test.ts +++ b/tests/index.test.ts @@ -308,6 +308,7 @@ describe( await vi.waitFor(() => expect(onExit).toHaveBeenCalledTimes(1)); expect(onExit).toHaveBeenCalledWith(null, 0); + console.log(buffer.byteLength) const lines = buffer.toString().trim().split('\n'); expect(lines.length).toBe(n + 1); for (let i = 0; i < n + 1; i++) { diff --git a/wrapper.ts b/wrapper.ts index 0a9020c..50c6bb7 100644 --- a/wrapper.ts +++ b/wrapper.ts @@ -47,7 +47,7 @@ export class Pty { #fd: number; #handledClose: boolean = false; - #handledEndOfData: boolean = false; + #fdClosed: boolean = false; #socket: ReadStream; #writable: Writable; @@ -73,7 +73,6 @@ export class Pty { markReadFinished = resolve; }); const mockedExit = (error: NodeJS.ErrnoException | null, code: number) => { - console.log('mocked exit'); markExited({ error, code }); }; @@ -92,27 +91,23 @@ export class Pty { // catch end events const handleClose = async () => { - if (this.#handledEndOfData) { + if (this.#fdClosed) { return; } - this.#handledEndOfData = true; + this.#fdClosed = true; // must wait for fd close and exit result before calling real exit - console.log('handle end'); await readFinished; const result = await exitResult; realExit(result.error, result.code); - console.log('done'); }; this.read.on('end', () => { - console.log('end'); markReadFinished(); }); this.read.on('close', () => { - console.log('close'); handleClose(); }); @@ -132,7 +127,6 @@ export class Pty { // is nothing left to read and we can start tearing things down. If we hadn't received an // error so far, we are considered to be in good standing. this.read.off('error', handleError); - console.log('eio'); this.#socket.emit('end'); return; } @@ -154,7 +148,7 @@ export class Pty { } resize(size: Size) { - if (this.#handledClose || this.#handledEndOfData) { + if (this.#handledClose || this.#fdClosed) { return; } From 16421d71a2f18a533d93a79a3ff654ca0ff89a83 Mon Sep 17 00:00:00 2001 From: Jacky Zhao Date: Thu, 24 Oct 2024 17:35:23 -0700 Subject: [PATCH 22/60] enable both --- tests/index.test.ts | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/tests/index.test.ts b/tests/index.test.ts index 94f03fa..a3a6129 100644 --- a/tests/index.test.ts +++ b/tests/index.test.ts @@ -308,7 +308,6 @@ describe( await vi.waitFor(() => expect(onExit).toHaveBeenCalledTimes(1)); expect(onExit).toHaveBeenCalledWith(null, 0); - console.log(buffer.byteLength) const lines = buffer.toString().trim().split('\n'); expect(lines.length).toBe(n + 1); for (let i = 0; i < n + 1; i++) { @@ -319,7 +318,7 @@ describe( } ); - test('doesnt miss large output from fast commands', async () => { + test.only('doesnt miss large output from fast commands', async () => { const payload = `hello`.repeat(4096); let buffer = Buffer.from(''); const onExit = vi.fn(); From 67e06117b620581539f9e021395116197f9af702 Mon Sep 17 00:00:00 2001 From: Jacky Zhao Date: Thu, 24 Oct 2024 17:39:59 -0700 Subject: [PATCH 23/60] remove some linux specific stuff --- src/lib.rs | 12 +----------- 1 file changed, 1 insertion(+), 11 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index beb2ab2..f97c55f 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -190,16 +190,6 @@ impl Pty { // and it's not safe to keep it open libc::close(raw_controller_fd); - // just to be safe, mark every single file descriptor as close-on-exec. - // needs to use the raw syscall to avoid dependencies on newer versions of glibc. - #[cfg(target_os = "linux")] - libc::syscall( - libc::SYS_close_range, - 3, - libc::c_uint::MAX, - libc::CLOSE_RANGE_CLOEXEC as c_int, - ); - // set input modes let user_fd = OwnedFd::from_raw_fd(raw_user_fd); if let Ok(mut termios) = termios::tcgetattr(&user_fd) { @@ -244,7 +234,7 @@ impl Pty { let wait_result = child.wait(); // try to wait for the controller fd to be fully read - poll_controller_fd_until_read(raw_controller_fd); + // poll_controller_fd_until_read(raw_controller_fd); // we don't drop the controller fd immediately // let pty.close() be responsible for closing it From 4c07825ea430f482996b205684d202879d1761d2 Mon Sep 17 00:00:00 2001 From: Jacky Zhao Date: Thu, 24 Oct 2024 17:44:28 -0700 Subject: [PATCH 24/60] try with timeout? --- src/lib.rs | 13 +++++++++++-- wrapper.ts | 4 +++- 2 files changed, 14 insertions(+), 3 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index f97c55f..b52b0ce 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -190,11 +190,20 @@ impl Pty { // and it's not safe to keep it open libc::close(raw_controller_fd); + // just to be safe, mark every single file descriptor as close-on-exec. + // needs to use the raw syscall to avoid dependencies on newer versions of glibc. + #[cfg(target_os = "linux")] + libc::syscall( + libc::SYS_close_range, + 3, + libc::c_uint::MAX, + libc::CLOSE_RANGE_CLOEXEC as c_int, + ); + // set input modes let user_fd = OwnedFd::from_raw_fd(raw_user_fd); if let Ok(mut termios) = termios::tcgetattr(&user_fd) { termios.input_flags |= termios::InputFlags::IUTF8; - termios::tcsetattr(&user_fd, SetArg::TCSANOW, &termios)?; } // reset signal handlers @@ -234,7 +243,7 @@ impl Pty { let wait_result = child.wait(); // try to wait for the controller fd to be fully read - // poll_controller_fd_until_read(raw_controller_fd); + poll_controller_fd_until_read(raw_controller_fd); // we don't drop the controller fd immediately // let pty.close() be responsible for closing it diff --git a/wrapper.ts b/wrapper.ts index 50c6bb7..53ba7d4 100644 --- a/wrapper.ts +++ b/wrapper.ts @@ -127,7 +127,9 @@ export class Pty { // is nothing left to read and we can start tearing things down. If we hadn't received an // error so far, we are considered to be in good standing. this.read.off('error', handleError); - this.#socket.emit('end'); + setTimeout(() => { + this.#socket.emit('end'); + }, 200); return; } } From 48850d265d866eca75dd8bcbad1c8e92c7987541 Mon Sep 17 00:00:00 2001 From: Jacky Zhao Date: Thu, 24 Oct 2024 17:44:58 -0700 Subject: [PATCH 25/60] re-add require --- index.js | 2 ++ 1 file changed, 2 insertions(+) diff --git a/index.js b/index.js index 7ac8050..824d2fb 100644 --- a/index.js +++ b/index.js @@ -6,6 +6,8 @@ const { existsSync, readFileSync } = require('fs') const { join } = require('path'); +const { createRequire } = require('node:module'); +require = createRequire(__filename); const { platform, arch } = process; From 28dc611c332d4cd7e84e9c58eef65e3e1dd08e37 Mon Sep 17 00:00:00 2001 From: Jacky Zhao Date: Thu, 24 Oct 2024 17:53:22 -0700 Subject: [PATCH 26/60] try sleep? --- tests/index.test.ts | 2 +- wrapper.ts | 4 +--- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/tests/index.test.ts b/tests/index.test.ts index a3a6129..eb8a729 100644 --- a/tests/index.test.ts +++ b/tests/index.test.ts @@ -295,7 +295,7 @@ describe( command: '/bin/sh', args: [ '-c', - `seq 0 ${n} && exit` + `seq 0 ${n} && sleep 0.1` ], onExit, }); diff --git a/wrapper.ts b/wrapper.ts index 53ba7d4..50c6bb7 100644 --- a/wrapper.ts +++ b/wrapper.ts @@ -127,9 +127,7 @@ export class Pty { // is nothing left to read and we can start tearing things down. If we hadn't received an // error so far, we are considered to be in good standing. this.read.off('error', handleError); - setTimeout(() => { - this.#socket.emit('end'); - }, 200); + this.#socket.emit('end'); return; } } From 2a8829b0d01dbb7328ebdafdf444c01d76faaa04 Mon Sep 17 00:00:00 2001 From: Jacky Zhao Date: Thu, 24 Oct 2024 18:15:13 -0700 Subject: [PATCH 27/60] check readable side --- tests/index.test.ts | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/tests/index.test.ts b/tests/index.test.ts index eb8a729..a735610 100644 --- a/tests/index.test.ts +++ b/tests/index.test.ts @@ -295,7 +295,7 @@ describe( command: '/bin/sh', args: [ '-c', - `seq 0 ${n} && sleep 0.1` + `seq 0 ${n}` ], onExit, }); @@ -305,8 +305,10 @@ describe( buffer = Buffer.concat([buffer, data]); }); + await vi.waitFor(() => expect(onExit).toHaveBeenCalledTimes(1)); expect(onExit).toHaveBeenCalledWith(null, 0); + expect(readStream.readableEnded).toBe(true); const lines = buffer.toString().trim().split('\n'); expect(lines.length).toBe(n + 1); @@ -318,7 +320,7 @@ describe( } ); - test.only('doesnt miss large output from fast commands', async () => { + test('doesnt miss large output from fast commands', async () => { const payload = `hello`.repeat(4096); let buffer = Buffer.from(''); const onExit = vi.fn(); From a370cd0fe5e36c3eca4ac04da4386f281328a92c Mon Sep 17 00:00:00 2001 From: Jacky Zhao Date: Thu, 24 Oct 2024 18:18:55 -0700 Subject: [PATCH 28/60] try suggestion --- wrapper.ts | 150 ++++++++++++----------------------------------------- 1 file changed, 33 insertions(+), 117 deletions(-) diff --git a/wrapper.ts b/wrapper.ts index 50c6bb7..6d5c235 100644 --- a/wrapper.ts +++ b/wrapper.ts @@ -1,12 +1,6 @@ import { type Readable, Writable } from 'node:stream'; import { ReadStream } from 'node:tty'; -import { - Pty as RawPty, - type Size, - setCloseOnExec as rawSetCloseOnExec, - getCloseOnExec as rawGetCloseOnExec, - ptyResize, -} from './index.js'; +import { Pty as RawPty, type Size } from './index.js'; import { type PtyOptions as RawOptions } from './index.js'; export type PtyOptions = RawOptions; @@ -16,41 +10,13 @@ type ExitResult = { code: number; }; -/** - * A very thin wrapper around PTYs and processes. - * - * @example - * const { Pty } = require('@replit/ruspty'); - * const fs = require('fs'); - * - * const pty = new Pty({ - * command: '/bin/sh', - * args: [], - * envs: ENV, - * dir: CWD, - * size: { rows: 24, cols: 80 }, - * onExit: (...result) => { - * // TODO: Handle process exit. - * }, - * }); - * - * const read = pty.read; - * const write = pty.write; - * - * read.on('data', (chunk) => { - * // TODO: Handle data. - * }); - * write.write('echo hello\n'); - */ export class Pty { #pty: RawPty; #fd: number; - - #handledClose: boolean = false; - #fdClosed: boolean = false; - #socket: ReadStream; #writable: Writable; + #pendingExit: ExitResult | null = null; + #ended = false; get read(): Readable { return this.#socket; @@ -63,25 +29,19 @@ export class Pty { constructor(options: PtyOptions) { const realExit = options.onExit; - let markExited: (value: ExitResult) => void; - let exitResult: Promise = new Promise((resolve) => { - markExited = resolve; - }); - - let markReadFinished: () => void; - let readFinished = new Promise((resolve) => { - markReadFinished = resolve; - }); - const mockedExit = (error: NodeJS.ErrnoException | null, code: number) => { - markExited({ error, code }); + // Create a proxy exit handler that coordinates with stream end + const handleExit = (error: NodeJS.ErrnoException | null, code: number) => { + const result = { error, code }; + if (this.#ended) { + // Stream already ended, safe to call exit immediately + realExit(error, code); + } else { + // Store exit result until stream ends + this.#pendingExit = result; + } }; - // when pty exits, we should wait until the fd actually ends (end OR error) - // before closing the pty - // we use a mocked exit function to capture the exit result - // and then call the real exit function after the fd is fully read - this.#pty = new RawPty({ ...options, onExit: mockedExit }); - // Transfer ownership of the FD to us. + this.#pty = new RawPty({ ...options, onExit: handleExit }); this.#fd = this.#pty.takeFd(); this.#socket = new ReadStream(this.#fd); @@ -89,50 +49,27 @@ export class Pty { write: this.#socket.write.bind(this.#socket), }); - // catch end events - const handleClose = async () => { - if (this.#fdClosed) { - return; - } - - this.#fdClosed = true; - - // must wait for fd close and exit result before calling real exit - await readFinished; - const result = await exitResult; - realExit(result.error, result.code); - }; - + // Handle stream end this.read.on('end', () => { - markReadFinished(); - }); - - this.read.on('close', () => { - handleClose(); + this.#ended = true; + if (this.#pendingExit) { + // Process already exited, now safe to call exit callback + const { error, code } = this.#pendingExit; + realExit(error, code); + } }); - // PTYs signal their done-ness with an EIO error. we therefore need to filter them out (as well as - // cleaning up other spurious errors) so that the user doesn't need to handle them and be in - // blissful peace. + // Filter expected PTY errors const handleError = (err: NodeJS.ErrnoException) => { if (err.code) { - const code = err.code; - if (code === 'EINTR' || code === 'EAGAIN') { - // these two are expected. EINTR happens when the kernel restarts a `read(2)`/`write(2)` - // syscall due to it being interrupted by another syscall, and EAGAIN happens when there - // is no more data to be read by the fd. + if (err.code === 'EINTR' || err.code === 'EAGAIN') { return; - } else if (code.indexOf('EIO') !== -1) { - // EIO only happens when the child dies. It is therefore our only true signal that there - // is nothing left to read and we can start tearing things down. If we hadn't received an - // error so far, we are considered to be in good standing. + } else if (err.code.indexOf('EIO') !== -1) { this.read.off('error', handleError); this.#socket.emit('end'); return; } } - - // if we haven't handled the error by now, we should throw it throw err; }; @@ -140,29 +77,20 @@ export class Pty { } close() { - this.#handledClose = true; - - // end instead of destroy so that the user can read the last bits of data - // and allow graceful close event to mark the fd as ended + // Use end() instead of destroy() to allow reading remaining data this.#socket.end(); } resize(size: Size) { - if (this.#handledClose || this.#fdClosed) { - return; - } - - try { - ptyResize(this.#fd, size); - } catch (e: unknown) { - if (e instanceof Error && 'code' in e && e.code === 'EBADF') { - // EBADF means the file descriptor is invalid. This can happen if the PTY has already - // exited but we don't know about it yet. In that case, we just ignore the error. - return; + if (!this.#ended) { + try { + ptyResize(this.#fd, size); + } catch (e: unknown) { + if (e instanceof Error && 'code' in e && e.code === 'EBADF') { + return; + } + throw e; } - - // otherwise, rethrow - throw e; } } @@ -170,15 +98,3 @@ export class Pty { return this.#pty.pid; } } - -/** - * Set the close-on-exec flag on a file descriptor. This is `fcntl(fd, F_SETFD, FD_CLOEXEC)` under - * the covers. - */ -export const setCloseOnExec = rawSetCloseOnExec; - -/** - * Get the close-on-exec flag on a file descriptor. This is `fcntl(fd, F_GETFD) & FD_CLOEXEC == - * FD_CLOEXEC` under the covers. - */ -export const getCloseOnExec = rawGetCloseOnExec; From 5587772cabb9b60e50c5ea23a122c7facf7f1d67 Mon Sep 17 00:00:00 2001 From: Jacky Zhao Date: Thu, 24 Oct 2024 18:21:17 -0700 Subject: [PATCH 29/60] oops forgor import --- wrapper.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/wrapper.ts b/wrapper.ts index 6d5c235..a367dfc 100644 --- a/wrapper.ts +++ b/wrapper.ts @@ -1,6 +1,6 @@ import { type Readable, Writable } from 'node:stream'; import { ReadStream } from 'node:tty'; -import { Pty as RawPty, type Size } from './index.js'; +import { Pty as RawPty, ptyResize, type Size } from './index.js'; import { type PtyOptions as RawOptions } from './index.js'; export type PtyOptions = RawOptions; From 8630ac2dccce68a63e37ca1cc27328e8c23bf1ac Mon Sep 17 00:00:00 2001 From: Jacky Zhao Date: Thu, 24 Oct 2024 18:27:49 -0700 Subject: [PATCH 30/60] test --- tests/index.test.ts | 5 +- wrapper.ts | 150 ++++++++++++++++++++++++++++++++++---------- 2 files changed, 120 insertions(+), 35 deletions(-) diff --git a/tests/index.test.ts b/tests/index.test.ts index a735610..2b2b4f0 100644 --- a/tests/index.test.ts +++ b/tests/index.test.ts @@ -302,13 +302,14 @@ describe( const readStream = pty.read; readStream.on('data', (data) => { + console.log('data', data.toString()); buffer = Buffer.concat([buffer, data]); }); - await vi.waitFor(() => expect(onExit).toHaveBeenCalledTimes(1)); expect(onExit).toHaveBeenCalledWith(null, 0); - expect(readStream.readableEnded).toBe(true); + console.log(readStream.readableEnded) + // expect(readStream.readableEnded).toBe(true); const lines = buffer.toString().trim().split('\n'); expect(lines.length).toBe(n + 1); diff --git a/wrapper.ts b/wrapper.ts index a367dfc..50c6bb7 100644 --- a/wrapper.ts +++ b/wrapper.ts @@ -1,6 +1,12 @@ import { type Readable, Writable } from 'node:stream'; import { ReadStream } from 'node:tty'; -import { Pty as RawPty, ptyResize, type Size } from './index.js'; +import { + Pty as RawPty, + type Size, + setCloseOnExec as rawSetCloseOnExec, + getCloseOnExec as rawGetCloseOnExec, + ptyResize, +} from './index.js'; import { type PtyOptions as RawOptions } from './index.js'; export type PtyOptions = RawOptions; @@ -10,13 +16,41 @@ type ExitResult = { code: number; }; +/** + * A very thin wrapper around PTYs and processes. + * + * @example + * const { Pty } = require('@replit/ruspty'); + * const fs = require('fs'); + * + * const pty = new Pty({ + * command: '/bin/sh', + * args: [], + * envs: ENV, + * dir: CWD, + * size: { rows: 24, cols: 80 }, + * onExit: (...result) => { + * // TODO: Handle process exit. + * }, + * }); + * + * const read = pty.read; + * const write = pty.write; + * + * read.on('data', (chunk) => { + * // TODO: Handle data. + * }); + * write.write('echo hello\n'); + */ export class Pty { #pty: RawPty; #fd: number; + + #handledClose: boolean = false; + #fdClosed: boolean = false; + #socket: ReadStream; #writable: Writable; - #pendingExit: ExitResult | null = null; - #ended = false; get read(): Readable { return this.#socket; @@ -29,19 +63,25 @@ export class Pty { constructor(options: PtyOptions) { const realExit = options.onExit; - // Create a proxy exit handler that coordinates with stream end - const handleExit = (error: NodeJS.ErrnoException | null, code: number) => { - const result = { error, code }; - if (this.#ended) { - // Stream already ended, safe to call exit immediately - realExit(error, code); - } else { - // Store exit result until stream ends - this.#pendingExit = result; - } + let markExited: (value: ExitResult) => void; + let exitResult: Promise = new Promise((resolve) => { + markExited = resolve; + }); + + let markReadFinished: () => void; + let readFinished = new Promise((resolve) => { + markReadFinished = resolve; + }); + const mockedExit = (error: NodeJS.ErrnoException | null, code: number) => { + markExited({ error, code }); }; - this.#pty = new RawPty({ ...options, onExit: handleExit }); + // when pty exits, we should wait until the fd actually ends (end OR error) + // before closing the pty + // we use a mocked exit function to capture the exit result + // and then call the real exit function after the fd is fully read + this.#pty = new RawPty({ ...options, onExit: mockedExit }); + // Transfer ownership of the FD to us. this.#fd = this.#pty.takeFd(); this.#socket = new ReadStream(this.#fd); @@ -49,27 +89,50 @@ export class Pty { write: this.#socket.write.bind(this.#socket), }); - // Handle stream end - this.read.on('end', () => { - this.#ended = true; - if (this.#pendingExit) { - // Process already exited, now safe to call exit callback - const { error, code } = this.#pendingExit; - realExit(error, code); + // catch end events + const handleClose = async () => { + if (this.#fdClosed) { + return; } + + this.#fdClosed = true; + + // must wait for fd close and exit result before calling real exit + await readFinished; + const result = await exitResult; + realExit(result.error, result.code); + }; + + this.read.on('end', () => { + markReadFinished(); }); - // Filter expected PTY errors + this.read.on('close', () => { + handleClose(); + }); + + // PTYs signal their done-ness with an EIO error. we therefore need to filter them out (as well as + // cleaning up other spurious errors) so that the user doesn't need to handle them and be in + // blissful peace. const handleError = (err: NodeJS.ErrnoException) => { if (err.code) { - if (err.code === 'EINTR' || err.code === 'EAGAIN') { + const code = err.code; + if (code === 'EINTR' || code === 'EAGAIN') { + // these two are expected. EINTR happens when the kernel restarts a `read(2)`/`write(2)` + // syscall due to it being interrupted by another syscall, and EAGAIN happens when there + // is no more data to be read by the fd. return; - } else if (err.code.indexOf('EIO') !== -1) { + } else if (code.indexOf('EIO') !== -1) { + // EIO only happens when the child dies. It is therefore our only true signal that there + // is nothing left to read and we can start tearing things down. If we hadn't received an + // error so far, we are considered to be in good standing. this.read.off('error', handleError); this.#socket.emit('end'); return; } } + + // if we haven't handled the error by now, we should throw it throw err; }; @@ -77,20 +140,29 @@ export class Pty { } close() { - // Use end() instead of destroy() to allow reading remaining data + this.#handledClose = true; + + // end instead of destroy so that the user can read the last bits of data + // and allow graceful close event to mark the fd as ended this.#socket.end(); } resize(size: Size) { - if (!this.#ended) { - try { - ptyResize(this.#fd, size); - } catch (e: unknown) { - if (e instanceof Error && 'code' in e && e.code === 'EBADF') { - return; - } - throw e; + if (this.#handledClose || this.#fdClosed) { + return; + } + + try { + ptyResize(this.#fd, size); + } catch (e: unknown) { + if (e instanceof Error && 'code' in e && e.code === 'EBADF') { + // EBADF means the file descriptor is invalid. This can happen if the PTY has already + // exited but we don't know about it yet. In that case, we just ignore the error. + return; } + + // otherwise, rethrow + throw e; } } @@ -98,3 +170,15 @@ export class Pty { return this.#pty.pid; } } + +/** + * Set the close-on-exec flag on a file descriptor. This is `fcntl(fd, F_SETFD, FD_CLOEXEC)` under + * the covers. + */ +export const setCloseOnExec = rawSetCloseOnExec; + +/** + * Get the close-on-exec flag on a file descriptor. This is `fcntl(fd, F_GETFD) & FD_CLOEXEC == + * FD_CLOEXEC` under the covers. + */ +export const getCloseOnExec = rawGetCloseOnExec; From a9ba65cef1c0b48e38ce89aa66df15fcad29d015 Mon Sep 17 00:00:00 2001 From: Jacky Zhao Date: Thu, 24 Oct 2024 18:33:01 -0700 Subject: [PATCH 31/60] try without eio --- wrapper.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/wrapper.ts b/wrapper.ts index 50c6bb7..775abb1 100644 --- a/wrapper.ts +++ b/wrapper.ts @@ -127,7 +127,8 @@ export class Pty { // is nothing left to read and we can start tearing things down. If we hadn't received an // error so far, we are considered to be in good standing. this.read.off('error', handleError); - this.#socket.emit('end'); + console.log('eio emit') + // this.#socket.end(); return; } } From 79c5d98a73c21ca1ff220610f48185e99538eec3 Mon Sep 17 00:00:00 2001 From: Jacky Zhao Date: Thu, 24 Oct 2024 18:41:24 -0700 Subject: [PATCH 32/60] only print out bytelength --- tests/index.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/index.test.ts b/tests/index.test.ts index 2b2b4f0..64ae282 100644 --- a/tests/index.test.ts +++ b/tests/index.test.ts @@ -302,7 +302,7 @@ describe( const readStream = pty.read; readStream.on('data', (data) => { - console.log('data', data.toString()); + console.log('data', data.byteLength); buffer = Buffer.concat([buffer, data]); }); From def3aef6d24188628cacf742b46b1bd36ffe8f10 Mon Sep 17 00:00:00 2001 From: Jacky Zhao Date: Fri, 25 Oct 2024 11:14:47 -0700 Subject: [PATCH 33/60] more logging --- wrapper.ts | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/wrapper.ts b/wrapper.ts index 775abb1..42b7e9b 100644 --- a/wrapper.ts +++ b/wrapper.ts @@ -73,6 +73,7 @@ export class Pty { markReadFinished = resolve; }); const mockedExit = (error: NodeJS.ErrnoException | null, code: number) => { + console.log('mocked exit') markExited({ error, code }); }; @@ -97,17 +98,21 @@ export class Pty { this.#fdClosed = true; + console.log('close') // must wait for fd close and exit result before calling real exit await readFinished; const result = await exitResult; + console.log('real exit') realExit(result.error, result.code); }; this.read.on('end', () => { + console.log('readable finished') markReadFinished(); }); this.read.on('close', () => { + console.log('user-side pty fd closed') handleClose(); }); From d46ce6bbb1683aa711a47de9e86e3239b255c607 Mon Sep 17 00:00:00 2001 From: Jacky Zhao Date: Fri, 25 Oct 2024 13:02:12 -0700 Subject: [PATCH 34/60] even more logs --- tests/index.test.ts | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/tests/index.test.ts b/tests/index.test.ts index 64ae282..ad98b35 100644 --- a/tests/index.test.ts +++ b/tests/index.test.ts @@ -1,7 +1,7 @@ import { Pty, getCloseOnExec, setCloseOnExec } from '../wrapper'; import { type Writable } from 'stream'; import { readdirSync, readlinkSync } from 'fs'; -import { describe, test, expect, beforeEach, afterEach, vi, type Mock } from 'vitest'; +import { describe, test, expect, beforeEach, afterEach, vi, type Mock, onTestFinished } from 'vitest'; import { exec as execAsync } from 'child_process'; import { promisify } from 'util'; const exec = promisify(execAsync); @@ -286,6 +286,9 @@ describe( test.only( 'ordering is correct', async () => { + console.log('-- start --') + onTestFinished(() => console.log('-- end --')); + const oldFds = getOpenFds(); let buffer = Buffer.from(''); const n = 1024; @@ -308,8 +311,6 @@ describe( await vi.waitFor(() => expect(onExit).toHaveBeenCalledTimes(1)); expect(onExit).toHaveBeenCalledWith(null, 0); - console.log(readStream.readableEnded) - // expect(readStream.readableEnded).toBe(true); const lines = buffer.toString().trim().split('\n'); expect(lines.length).toBe(n + 1); From 8af59d08841d96fe729b9cc94d31b5f1b0703580 Mon Sep 17 00:00:00 2001 From: Jacky Zhao Date: Fri, 25 Oct 2024 13:09:19 -0700 Subject: [PATCH 35/60] zam --- tests/index.test.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/index.test.ts b/tests/index.test.ts index ad98b35..7ae8c20 100644 --- a/tests/index.test.ts +++ b/tests/index.test.ts @@ -1,7 +1,7 @@ import { Pty, getCloseOnExec, setCloseOnExec } from '../wrapper'; import { type Writable } from 'stream'; import { readdirSync, readlinkSync } from 'fs'; -import { describe, test, expect, beforeEach, afterEach, vi, type Mock, onTestFinished } from 'vitest'; +import { describe, test, expect, beforeEach, afterEach, vi, type Mock } from 'vitest'; import { exec as execAsync } from 'child_process'; import { promisify } from 'util'; const exec = promisify(execAsync); @@ -287,7 +287,6 @@ describe( 'ordering is correct', async () => { console.log('-- start --') - onTestFinished(() => console.log('-- end --')); const oldFds = getOpenFds(); let buffer = Buffer.from(''); @@ -313,6 +312,7 @@ describe( expect(onExit).toHaveBeenCalledWith(null, 0); const lines = buffer.toString().trim().split('\n'); + console.log('got lines', lines.length, 'expected', n + 1); expect(lines.length).toBe(n + 1); for (let i = 0; i < n + 1; i++) { expect(Number(lines[i]), `expected line ${i} to contain ${i} but got ${lines[i]}`).toBe(i); From 5381170afc38faf5a7bebd16f21204a6b6fb0a51 Mon Sep 17 00:00:00 2001 From: Jacky Zhao Date: Fri, 25 Oct 2024 13:31:11 -0700 Subject: [PATCH 36/60] strace analysis --- .github/workflows/CI.yml | 15 +++++++++++++++ src/lib.rs | 2 +- tests/index.test.ts | 1 - 3 files changed, 16 insertions(+), 2 deletions(-) diff --git a/.github/workflows/CI.yml b/.github/workflows/CI.yml index 77c69e3..14bd0e6 100644 --- a/.github/workflows/CI.yml +++ b/.github/workflows/CI.yml @@ -66,6 +66,21 @@ jobs: shell: bash - name: Dump GLIBC symbols run: objdump -T *.node | grep GLIBC | sed 's/.*GLIBC_\([.0-9]*\).*/\1/g' | sort -Vu + + - name: strace + if: runner.os == 'Linux' # This ensures the step only runs on Linux + run: | + sudo apt-get update && sudo apt-get install -y strace + + # Run tests with strace and save full output + strace -f -e trace=desc -e signal=none -e fault=none -s 100 npm run test 2> strace.log + - name: Upload strace logs + if: runner.os == 'Linux' && always() # Only upload on Linux, but always do it if the step ran + uses: actions/upload-artifact@v4 + with: + name: strace-logs + path: strace.log + - name: Test bindings run: npm run test - name: Upload artifact diff --git a/src/lib.rs b/src/lib.rs index b52b0ce..152f3a8 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -247,7 +247,7 @@ impl Pty { // we don't drop the controller fd immediately // let pty.close() be responsible for closing it - drop(user_fd); + // drop(user_fd); match wait_result { Ok(status) => { diff --git a/tests/index.test.ts b/tests/index.test.ts index 7ae8c20..185e818 100644 --- a/tests/index.test.ts +++ b/tests/index.test.ts @@ -311,7 +311,6 @@ describe( await vi.waitFor(() => expect(onExit).toHaveBeenCalledTimes(1)); expect(onExit).toHaveBeenCalledWith(null, 0); - const lines = buffer.toString().trim().split('\n'); console.log('got lines', lines.length, 'expected', n + 1); expect(lines.length).toBe(n + 1); for (let i = 0; i < n + 1; i++) { From 63a8b27c881c99cfaf364f9d82fa55b64f8d193b Mon Sep 17 00:00:00 2001 From: Jacky Zhao Date: Fri, 25 Oct 2024 13:35:31 -0700 Subject: [PATCH 37/60] oops --- tests/index.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/index.test.ts b/tests/index.test.ts index 185e818..73b001e 100644 --- a/tests/index.test.ts +++ b/tests/index.test.ts @@ -311,7 +311,7 @@ describe( await vi.waitFor(() => expect(onExit).toHaveBeenCalledTimes(1)); expect(onExit).toHaveBeenCalledWith(null, 0); - console.log('got lines', lines.length, 'expected', n + 1); + const lines = buffer.toString().trim().split('\n'); expect(lines.length).toBe(n + 1); for (let i = 0; i < n + 1; i++) { expect(Number(lines[i]), `expected line ${i} to contain ${i} but got ${lines[i]}`).toBe(i); From ed54c5641c72217b69d1a1a64bbb521dc974170e Mon Sep 17 00:00:00 2001 From: Jacky Zhao Date: Fri, 25 Oct 2024 14:51:53 -0700 Subject: [PATCH 38/60] more tests --- .github/workflows/CI.yml | 15 --------------- src/lib.rs | 6 ++---- 2 files changed, 2 insertions(+), 19 deletions(-) diff --git a/.github/workflows/CI.yml b/.github/workflows/CI.yml index 14bd0e6..77c69e3 100644 --- a/.github/workflows/CI.yml +++ b/.github/workflows/CI.yml @@ -66,21 +66,6 @@ jobs: shell: bash - name: Dump GLIBC symbols run: objdump -T *.node | grep GLIBC | sed 's/.*GLIBC_\([.0-9]*\).*/\1/g' | sort -Vu - - - name: strace - if: runner.os == 'Linux' # This ensures the step only runs on Linux - run: | - sudo apt-get update && sudo apt-get install -y strace - - # Run tests with strace and save full output - strace -f -e trace=desc -e signal=none -e fault=none -s 100 npm run test 2> strace.log - - name: Upload strace logs - if: runner.os == 'Linux' && always() # Only upload on Linux, but always do it if the step ran - uses: actions/upload-artifact@v4 - with: - name: strace-logs - path: strace.log - - name: Test bindings run: npm run test - name: Upload artifact diff --git a/src/lib.rs b/src/lib.rs index 152f3a8..2c9791a 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -240,15 +240,13 @@ impl Pty { .create_threadsafe_function(0, |ctx| ctx.env.create_int32(ctx.value).map(|v| vec![v]))?; thread::spawn(move || { + drop(user_fd); + let wait_result = child.wait(); // try to wait for the controller fd to be fully read poll_controller_fd_until_read(raw_controller_fd); - // we don't drop the controller fd immediately - // let pty.close() be responsible for closing it - // drop(user_fd); - match wait_result { Ok(status) => { if status.success() { From 7c32410dddc2648bc393ed4ff95609f9cb0a8c7d Mon Sep 17 00:00:00 2001 From: Jacky Zhao Date: Fri, 25 Oct 2024 15:01:22 -0700 Subject: [PATCH 39/60] try with destroy soon --- wrapper.ts | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/wrapper.ts b/wrapper.ts index 42b7e9b..a6d8b88 100644 --- a/wrapper.ts +++ b/wrapper.ts @@ -73,7 +73,7 @@ export class Pty { markReadFinished = resolve; }); const mockedExit = (error: NodeJS.ErrnoException | null, code: number) => { - console.log('mocked exit') + console.log('mocked exit'); markExited({ error, code }); }; @@ -98,21 +98,21 @@ export class Pty { this.#fdClosed = true; - console.log('close') + console.log('close'); // must wait for fd close and exit result before calling real exit await readFinished; const result = await exitResult; - console.log('real exit') + console.log('real exit'); realExit(result.error, result.code); }; this.read.on('end', () => { - console.log('readable finished') + console.log('readable finished'); markReadFinished(); }); this.read.on('close', () => { - console.log('user-side pty fd closed') + console.log('user-side pty fd closed'); handleClose(); }); @@ -132,7 +132,8 @@ export class Pty { // is nothing left to read and we can start tearing things down. If we hadn't received an // error so far, we are considered to be in good standing. this.read.off('error', handleError); - console.log('eio emit') + console.log('eio emit'); + this.#socket.destroySoon(); // this.#socket.end(); return; } From 2ef14a5f85df35943c298cad6e784935a88c1823 Mon Sep 17 00:00:00 2001 From: Jacky Zhao Date: Fri, 25 Oct 2024 15:07:55 -0700 Subject: [PATCH 40/60] try with emit end --- wrapper.ts | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/wrapper.ts b/wrapper.ts index a6d8b88..75317b0 100644 --- a/wrapper.ts +++ b/wrapper.ts @@ -133,8 +133,9 @@ export class Pty { // error so far, we are considered to be in good standing. this.read.off('error', handleError); console.log('eio emit'); - this.#socket.destroySoon(); - // this.#socket.end(); + // emit 'end' to signal no more data + // this will trigger our 'end' handler which marks readFinished + this.read.emit('end'); return; } } From e908971c3259761225465b1b30c0fd66d9f03f35 Mon Sep 17 00:00:00 2001 From: Jacky Zhao Date: Fri, 25 Oct 2024 15:22:38 -0700 Subject: [PATCH 41/60] run all the tests now --- src/lib.rs | 1 + tests/index.test.ts | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/src/lib.rs b/src/lib.rs index 2c9791a..0209f79 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -204,6 +204,7 @@ impl Pty { let user_fd = OwnedFd::from_raw_fd(raw_user_fd); if let Ok(mut termios) = termios::tcgetattr(&user_fd) { termios.input_flags |= termios::InputFlags::IUTF8; + termios::tcsetattr(&user_fd, SetArg::TCSANOW, &termios)?; } // reset signal handlers diff --git a/tests/index.test.ts b/tests/index.test.ts index 73b001e..0809094 100644 --- a/tests/index.test.ts +++ b/tests/index.test.ts @@ -283,7 +283,7 @@ describe( expect(onExit).toHaveBeenCalledWith(null, -1); }); - test.only( + test( 'ordering is correct', async () => { console.log('-- start --') From 084818db3ef43d3d91c80c70311a6b001144cdc8 Mon Sep 17 00:00:00 2001 From: Jacky Zhao Date: Fri, 25 Oct 2024 15:34:42 -0700 Subject: [PATCH 42/60] put back only fuck --- tests/index.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/index.test.ts b/tests/index.test.ts index 0809094..73b001e 100644 --- a/tests/index.test.ts +++ b/tests/index.test.ts @@ -283,7 +283,7 @@ describe( expect(onExit).toHaveBeenCalledWith(null, -1); }); - test( + test.only( 'ordering is correct', async () => { console.log('-- start --') From fdbc2fb1499d48309a8dd87e740f619cdd63b42d Mon Sep 17 00:00:00 2001 From: Jacky Zhao Date: Fri, 25 Oct 2024 15:36:12 -0700 Subject: [PATCH 43/60] wtf --- src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/lib.rs b/src/lib.rs index 0209f79..501064b 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -204,7 +204,7 @@ impl Pty { let user_fd = OwnedFd::from_raw_fd(raw_user_fd); if let Ok(mut termios) = termios::tcgetattr(&user_fd) { termios.input_flags |= termios::InputFlags::IUTF8; - termios::tcsetattr(&user_fd, SetArg::TCSANOW, &termios)?; + // termios::tcsetattr(&user_fd, SetArg::TCSANOW, &termios)?; } // reset signal handlers From f5cfe8b97d0bfd035bbf13489adf8dc4a7c4b855 Mon Sep 17 00:00:00 2001 From: Jacky Zhao Date: Fri, 25 Oct 2024 15:55:09 -0700 Subject: [PATCH 44/60] print some more expectations --- src/lib.rs | 2 +- tests/index.test.ts | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/src/lib.rs b/src/lib.rs index 501064b..0209f79 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -204,7 +204,7 @@ impl Pty { let user_fd = OwnedFd::from_raw_fd(raw_user_fd); if let Ok(mut termios) = termios::tcgetattr(&user_fd) { termios.input_flags |= termios::InputFlags::IUTF8; - // termios::tcsetattr(&user_fd, SetArg::TCSANOW, &termios)?; + termios::tcsetattr(&user_fd, SetArg::TCSANOW, &termios)?; } // reset signal handlers diff --git a/tests/index.test.ts b/tests/index.test.ts index 73b001e..7ae8c20 100644 --- a/tests/index.test.ts +++ b/tests/index.test.ts @@ -312,6 +312,7 @@ describe( expect(onExit).toHaveBeenCalledWith(null, 0); const lines = buffer.toString().trim().split('\n'); + console.log('got lines', lines.length, 'expected', n + 1); expect(lines.length).toBe(n + 1); for (let i = 0; i < n + 1; i++) { expect(Number(lines[i]), `expected line ${i} to contain ${i} but got ${lines[i]}`).toBe(i); From 79567f6b815e286ce0c2a1d9b9d6a0a4d61ec3a4 Mon Sep 17 00:00:00 2001 From: Jacky Zhao Date: Fri, 25 Oct 2024 16:05:01 -0700 Subject: [PATCH 45/60] even more print debug --- src/lib.rs | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/src/lib.rs b/src/lib.rs index 0209f79..5a93582 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -77,17 +77,20 @@ fn poll_controller_fd_until_read(raw_fd: RawFd) { if let Err(err) = poll(&mut [poll_fd], PollTimeout::ZERO) { if err == Errno::EINTR || err == Errno::EAGAIN { // we were interrupted, so we should just try again + println!("poll interrupted"); continue; } // we should almost never hit this, but if we do, we should just break out of the loop. this // can happen if Node destroys the terminal before waiting for the child process to go away. + println!("poll error: {}", err); break; } // check if POLLIN is no longer set (i.e. there is no more data to read) if let Some(flags) = poll_fd.revents() { if !flags.contains(PollFlags::POLLIN) { + println!("poll complete"); break; } } @@ -98,6 +101,7 @@ fn poll_controller_fd_until_read(raw_fd: RawFd) { continue; } else { // we have exhausted our attempts, its joever + println!("poll timeout"); break; } } @@ -164,6 +168,7 @@ impl Pty { unsafe { // right before we spawn the child, we should do a bunch of setup // this is all run in the context of the child process + println!("pre_exec"); cmd.pre_exec(move || { // start a new session let err = libc::setsid(); @@ -220,6 +225,7 @@ impl Pty { } // actually spawn the child + println!("spawn"); let mut child = cmd.spawn()?; let pid = child.id(); @@ -243,10 +249,14 @@ impl Pty { thread::spawn(move || { drop(user_fd); + println!("start wait"); let wait_result = child.wait(); + println!("end wait"); // try to wait for the controller fd to be fully read + println!("start poll"); poll_controller_fd_until_read(raw_controller_fd); + println!("end poll"); match wait_result { Ok(status) => { From 8e5b434219f0073b7b36f4895cf9225ae55c9075 Mon Sep 17 00:00:00 2001 From: Jacky Zhao Date: Fri, 25 Oct 2024 16:11:47 -0700 Subject: [PATCH 46/60] goddamn --- tests/index.test.ts | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/tests/index.test.ts b/tests/index.test.ts index 7ae8c20..f5b9845 100644 --- a/tests/index.test.ts +++ b/tests/index.test.ts @@ -314,11 +314,12 @@ describe( const lines = buffer.toString().trim().split('\n'); console.log('got lines', lines.length, 'expected', n + 1); expect(lines.length).toBe(n + 1); - for (let i = 0; i < n + 1; i++) { - expect(Number(lines[i]), `expected line ${i} to contain ${i} but got ${lines[i]}`).toBe(i); - } - - expect(getOpenFds()).toStrictEqual(oldFds); + console.log('-- end --') + // for (let i = 0; i < n + 1; i++) { + // expect(Number(lines[i]), `expected line ${i} to contain ${i} but got ${lines[i]}`).toBe(i); + // } + // + // expect(getOpenFds()).toStrictEqual(oldFds); } ); From a915772241b408a5ee4137c252d4675dc9c4f95c Mon Sep 17 00:00:00 2001 From: Jacky Zhao Date: Fri, 25 Oct 2024 16:23:20 -0700 Subject: [PATCH 47/60] flush --- src/lib.rs | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/src/lib.rs b/src/lib.rs index 5a93582..25a30ad 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -1,5 +1,6 @@ use std::collections::HashMap; use std::fs::File; +use std::io; use std::io::Error; use std::io::ErrorKind; use std::io::Write; @@ -78,12 +79,14 @@ fn poll_controller_fd_until_read(raw_fd: RawFd) { if err == Errno::EINTR || err == Errno::EAGAIN { // we were interrupted, so we should just try again println!("poll interrupted"); + io::stdout().flush().unwrap(); continue; } // we should almost never hit this, but if we do, we should just break out of the loop. this // can happen if Node destroys the terminal before waiting for the child process to go away. println!("poll error: {}", err); + io::stdout().flush().unwrap(); break; } @@ -91,6 +94,7 @@ fn poll_controller_fd_until_read(raw_fd: RawFd) { if let Some(flags) = poll_fd.revents() { if !flags.contains(PollFlags::POLLIN) { println!("poll complete"); + io::stdout().flush().unwrap(); break; } } @@ -102,6 +106,7 @@ fn poll_controller_fd_until_read(raw_fd: RawFd) { } else { // we have exhausted our attempts, its joever println!("poll timeout"); + io::stdout().flush().unwrap(); break; } } @@ -169,6 +174,7 @@ impl Pty { // right before we spawn the child, we should do a bunch of setup // this is all run in the context of the child process println!("pre_exec"); + io::stdout().flush().unwrap(); cmd.pre_exec(move || { // start a new session let err = libc::setsid(); @@ -226,6 +232,7 @@ impl Pty { // actually spawn the child println!("spawn"); + io::stdout().flush().unwrap(); let mut child = cmd.spawn()?; let pid = child.id(); @@ -250,13 +257,17 @@ impl Pty { drop(user_fd); println!("start wait"); + io::stdout().flush().unwrap(); let wait_result = child.wait(); println!("end wait"); + io::stdout().flush().unwrap(); // try to wait for the controller fd to be fully read println!("start poll"); + io::stdout().flush().unwrap(); poll_controller_fd_until_read(raw_controller_fd); println!("end poll"); + io::stdout().flush().unwrap(); match wait_result { Ok(status) => { From 58f03292cd791ee3b747630b7d94029204dc8e73 Mon Sep 17 00:00:00 2001 From: Jacky Zhao Date: Fri, 25 Oct 2024 16:36:50 -0700 Subject: [PATCH 48/60] i am confusion --- src/lib.rs | 20 -------------------- 1 file changed, 20 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index 25a30ad..6a6df3b 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -78,23 +78,17 @@ fn poll_controller_fd_until_read(raw_fd: RawFd) { if let Err(err) = poll(&mut [poll_fd], PollTimeout::ZERO) { if err == Errno::EINTR || err == Errno::EAGAIN { // we were interrupted, so we should just try again - println!("poll interrupted"); - io::stdout().flush().unwrap(); continue; } // we should almost never hit this, but if we do, we should just break out of the loop. this // can happen if Node destroys the terminal before waiting for the child process to go away. - println!("poll error: {}", err); - io::stdout().flush().unwrap(); break; } // check if POLLIN is no longer set (i.e. there is no more data to read) if let Some(flags) = poll_fd.revents() { if !flags.contains(PollFlags::POLLIN) { - println!("poll complete"); - io::stdout().flush().unwrap(); break; } } @@ -105,8 +99,6 @@ fn poll_controller_fd_until_read(raw_fd: RawFd) { continue; } else { // we have exhausted our attempts, its joever - println!("poll timeout"); - io::stdout().flush().unwrap(); break; } } @@ -173,8 +165,6 @@ impl Pty { unsafe { // right before we spawn the child, we should do a bunch of setup // this is all run in the context of the child process - println!("pre_exec"); - io::stdout().flush().unwrap(); cmd.pre_exec(move || { // start a new session let err = libc::setsid(); @@ -231,8 +221,6 @@ impl Pty { } // actually spawn the child - println!("spawn"); - io::stdout().flush().unwrap(); let mut child = cmd.spawn()?; let pid = child.id(); @@ -256,18 +244,10 @@ impl Pty { thread::spawn(move || { drop(user_fd); - println!("start wait"); - io::stdout().flush().unwrap(); let wait_result = child.wait(); - println!("end wait"); - io::stdout().flush().unwrap(); // try to wait for the controller fd to be fully read - println!("start poll"); - io::stdout().flush().unwrap(); poll_controller_fd_until_read(raw_controller_fd); - println!("end poll"); - io::stdout().flush().unwrap(); match wait_result { Ok(status) => { From d1aa64886de7f5464525390d85fbe28310b36b7d Mon Sep 17 00:00:00 2001 From: Jacky Zhao Date: Fri, 25 Oct 2024 16:41:19 -0700 Subject: [PATCH 49/60] more --- tests/index.test.ts | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/tests/index.test.ts b/tests/index.test.ts index f5b9845..e8a6116 100644 --- a/tests/index.test.ts +++ b/tests/index.test.ts @@ -308,13 +308,16 @@ describe( buffer = Buffer.concat([buffer, data]); }); + readStream.on('error', (err) => console.log('error', err)); + await vi.waitFor(() => expect(onExit).toHaveBeenCalledTimes(1)); expect(onExit).toHaveBeenCalledWith(null, 0); const lines = buffer.toString().trim().split('\n'); console.log('got lines', lines.length, 'expected', n + 1); - expect(lines.length).toBe(n + 1); + console.log('readable ended', readStream.readableEnded); console.log('-- end --') + expect(lines.length).toBe(n + 1); // for (let i = 0; i < n + 1; i++) { // expect(Number(lines[i]), `expected line ${i} to contain ${i} but got ${lines[i]}`).toBe(i); // } From a1a99980a34dd6d31aa470f3f67e4e0de597ca90 Mon Sep 17 00:00:00 2001 From: Jacky Zhao Date: Fri, 25 Oct 2024 17:27:06 -0700 Subject: [PATCH 50/60] another check --- src/lib.rs | 23 +++++++++++++++++++++-- 1 file changed, 21 insertions(+), 2 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index 6a6df3b..976d176 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -242,13 +242,32 @@ impl Pty { .create_threadsafe_function(0, |ctx| ctx.env.create_int32(ctx.value).map(|v| vec![v]))?; thread::spawn(move || { - drop(user_fd); - let wait_result = child.wait(); // try to wait for the controller fd to be fully read poll_controller_fd_until_read(raw_controller_fd); + // check TIOCOUTQ to see if there is any data left to be read + let mut user_outq = 0; + let user_tiocoutq_res = unsafe { libc::ioctl(raw_user_fd, libc::TIOCOUTQ, &mut user_outq) }; + if user_tiocoutq_res == -1 { + eprintln!("ioctl TIOCOUTQ failed: {}", user_tiocoutq_res); + } + + let mut controller_outq = 0; + let controller_tiocoutq_res = + unsafe { libc::ioctl(raw_controller_fd, libc::TIOCOUTQ, &mut controller_outq) }; + if controller_tiocoutq_res == -1 { + eprintln!("ioctl TIOCOUTQ failed: {}", controller_tiocoutq_res); + } + + println!( + "child process exited, user_outq: {}, controller_outq: {}", + user_outq, controller_outq + ); + + drop(user_fd); + match wait_result { Ok(status) => { if status.success() { From 1365d3a23bcac4b6f3e9d8744070869926414d8f Mon Sep 17 00:00:00 2001 From: Jacky Zhao Date: Fri, 25 Oct 2024 17:33:45 -0700 Subject: [PATCH 51/60] also FIONREAD --- src/lib.rs | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index 976d176..3cd7423 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -261,9 +261,22 @@ impl Pty { eprintln!("ioctl TIOCOUTQ failed: {}", controller_tiocoutq_res); } + let mut user_inq = 0; + let user_tiocinq_res = unsafe { libc::ioctl(raw_user_fd, libc::FIONREAD, &mut user_inq) }; + if user_tiocinq_res == -1 { + eprintln!("ioctl TIOCINQ failed: {}", user_tiocinq_res); + } + + let mut controller_inq = 0; + let controller_tiocinq_res = + unsafe { libc::ioctl(raw_controller_fd, libc::FIONREAD, &mut controller_inq) }; + if controller_tiocinq_res == -1 { + eprintln!("ioctl TIOCINQ failed: {}", controller_tiocinq_res); + } + println!( - "child process exited, user_outq: {}, controller_outq: {}", - user_outq, controller_outq + "user_outq: {}, controller_outq: {}, user_inq: {}, controller_inq: {}", + user_outq, controller_outq, user_inq, controller_inq ); drop(user_fd); From 557045eeace5fec9c2ac415e1fd33d8cb70a21e2 Mon Sep 17 00:00:00 2001 From: Jacky Zhao Date: Mon, 28 Oct 2024 09:55:58 -0700 Subject: [PATCH 52/60] test poll for TIOCING and TIOCOUTQ --- src/lib.rs | 121 +++++++++++++++++++++++++++++------------------------ 1 file changed, 66 insertions(+), 55 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index 3cd7423..d05e49d 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -20,7 +20,7 @@ use napi::Status::GenericFailure; use napi::{self, Env}; use nix::errno::Errno; use nix::fcntl::{fcntl, FcntlArg, FdFlag, OFlag}; -use nix::poll::{poll, PollFd, PollFlags, PollTimeout}; +use nix::libc::{FIONREAD, TIOCOUTQ, ioctl}; use nix::pty::{openpty, Winsize}; use nix::sys::termios::{self, SetArg}; @@ -60,14 +60,11 @@ fn cast_to_napi_error(err: Errno) -> napi::Error { napi::Error::new(GenericFailure, err) } -// if the child process exits before the controller fd is fully read, we might accidentally -// end in a case where onExit is called but js hasn't had the chance to fully read the controller fd +// if the child process exits before the controller fd is fully read or the user fd is fully +// flushed, we might accidentally end in a case where onExit is called but js hasn't had +// the chance to fully read the controller fd // let's wait until the controller fd is fully read before we call onExit -fn poll_controller_fd_until_read(raw_fd: RawFd) { - // wait until fd is fully read (i.e. POLLIN no longer set) - let borrowed_fd = unsafe { BorrowedFd::borrow_raw(raw_fd) }; - let poll_fd = PollFd::new(borrowed_fd, PollFlags::POLLIN); - +fn poll_pty_fds_until_read(controller_fd: RawFd, user_fd: RawFd) { let mut backoff = ExponentialBackoffBuilder::default() .with_initial_interval(Duration::from_millis(1)) .with_max_interval(Duration::from_millis(100)) @@ -75,30 +72,44 @@ fn poll_controller_fd_until_read(raw_fd: RawFd) { .build(); loop { - if let Err(err) = poll(&mut [poll_fd], PollTimeout::ZERO) { - if err == Errno::EINTR || err == Errno::EAGAIN { - // we were interrupted, so we should just try again - continue; - } + // Check both input and output queues for both FDs + let mut controller_inq: i32 = 0; + let mut controller_outq: i32 = 0; + let mut user_inq: i32 = 0; + let mut user_outq: i32 = 0; - // we should almost never hit this, but if we do, we should just break out of the loop. this - // can happen if Node destroys the terminal before waiting for the child process to go away. - break; - } + // Safe because we're passing valid file descriptors and properly sized integers + unsafe { + // Check bytes waiting to be read (FIONREAD, equivalent to TIOCINQ on Linux) + if ioctl(controller_fd, FIONREAD, &mut controller_inq) < 0 + || ioctl(user_fd, FIONREAD, &mut user_inq) < 0 + { + // break if we can't read + println!("ioctl failed FIONREAD"); + break; + } - // check if POLLIN is no longer set (i.e. there is no more data to read) - if let Some(flags) = poll_fd.revents() { - if !flags.contains(PollFlags::POLLIN) { + // Check bytes waiting to be written (TIOCOUTQ) + if ioctl(controller_fd, TIOCOUTQ, &mut controller_outq) < 0 + || ioctl(user_fd, TIOCOUTQ, &mut user_outq) < 0 + { + // break if we can't read + println!("ioctl failed TIOCOUTQ"); break; } } - // wait for a bit before trying again + // If all queues are empty, we're done + if controller_inq == 0 && controller_outq == 0 && user_inq == 0 && user_outq == 0 { + break; + } + + // Apply backoff strategy if let Some(d) = backoff.next_backoff() { thread::sleep(d); continue; } else { - // we have exhausted our attempts, its joever + // We have exhausted our attempts break; } } @@ -245,39 +256,39 @@ impl Pty { let wait_result = child.wait(); // try to wait for the controller fd to be fully read - poll_controller_fd_until_read(raw_controller_fd); - - // check TIOCOUTQ to see if there is any data left to be read - let mut user_outq = 0; - let user_tiocoutq_res = unsafe { libc::ioctl(raw_user_fd, libc::TIOCOUTQ, &mut user_outq) }; - if user_tiocoutq_res == -1 { - eprintln!("ioctl TIOCOUTQ failed: {}", user_tiocoutq_res); - } - - let mut controller_outq = 0; - let controller_tiocoutq_res = - unsafe { libc::ioctl(raw_controller_fd, libc::TIOCOUTQ, &mut controller_outq) }; - if controller_tiocoutq_res == -1 { - eprintln!("ioctl TIOCOUTQ failed: {}", controller_tiocoutq_res); - } - - let mut user_inq = 0; - let user_tiocinq_res = unsafe { libc::ioctl(raw_user_fd, libc::FIONREAD, &mut user_inq) }; - if user_tiocinq_res == -1 { - eprintln!("ioctl TIOCINQ failed: {}", user_tiocinq_res); - } - - let mut controller_inq = 0; - let controller_tiocinq_res = - unsafe { libc::ioctl(raw_controller_fd, libc::FIONREAD, &mut controller_inq) }; - if controller_tiocinq_res == -1 { - eprintln!("ioctl TIOCINQ failed: {}", controller_tiocinq_res); - } - - println!( - "user_outq: {}, controller_outq: {}, user_inq: {}, controller_inq: {}", - user_outq, controller_outq, user_inq, controller_inq - ); + poll_pty_fds_until_read(raw_controller_fd, raw_user_fd); + + // // check TIOCOUTQ to see if there is any data left to be read + // let mut user_outq = 0; + // let user_tiocoutq_res = unsafe { libc::ioctl(raw_user_fd, libc::TIOCOUTQ, &mut user_outq) }; + // if user_tiocoutq_res == -1 { + // eprintln!("ioctl TIOCOUTQ failed: {}", user_tiocoutq_res); + // } + // + // let mut controller_outq = 0; + // let controller_tiocoutq_res = + // unsafe { libc::ioctl(raw_controller_fd, libc::TIOCOUTQ, &mut controller_outq) }; + // if controller_tiocoutq_res == -1 { + // eprintln!("ioctl TIOCOUTQ failed: {}", controller_tiocoutq_res); + // } + // + // let mut user_inq = 0; + // let user_tiocinq_res = unsafe { libc::ioctl(raw_user_fd, libc::FIONREAD, &mut user_inq) }; + // if user_tiocinq_res == -1 { + // eprintln!("ioctl TIOCINQ failed: {}", user_tiocinq_res); + // } + // + // let mut controller_inq = 0; + // let controller_tiocinq_res = + // unsafe { libc::ioctl(raw_controller_fd, libc::FIONREAD, &mut controller_inq) }; + // if controller_tiocinq_res == -1 { + // eprintln!("ioctl TIOCINQ failed: {}", controller_tiocinq_res); + // } + // + // println!( + // "user_outq: {}, controller_outq: {}, user_inq: {}, controller_inq: {}", + // user_outq, controller_outq, user_inq, controller_inq + // ); drop(user_fd); From c9768cc29eb2a5ace42f3dbe3920bf2a8d928feb Mon Sep 17 00:00:00 2001 From: Jacky Zhao Date: Mon, 28 Oct 2024 09:58:53 -0700 Subject: [PATCH 53/60] cleanup some more --- src/lib.rs | 49 +++++++------------------------------------------ 1 file changed, 7 insertions(+), 42 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index d05e49d..52ce892 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -72,44 +72,42 @@ fn poll_pty_fds_until_read(controller_fd: RawFd, user_fd: RawFd) { .build(); loop { - // Check both input and output queues for both FDs + // check both input and output queues for both FDs let mut controller_inq: i32 = 0; let mut controller_outq: i32 = 0; let mut user_inq: i32 = 0; let mut user_outq: i32 = 0; - // Safe because we're passing valid file descriptors and properly sized integers + // safe because we're passing valid file descriptors and properly sized integers unsafe { - // Check bytes waiting to be read (FIONREAD, equivalent to TIOCINQ on Linux) + // check bytes waiting to be read (FIONREAD, equivalent to TIOCINQ on Linux) if ioctl(controller_fd, FIONREAD, &mut controller_inq) < 0 || ioctl(user_fd, FIONREAD, &mut user_inq) < 0 { // break if we can't read - println!("ioctl failed FIONREAD"); break; } - // Check bytes waiting to be written (TIOCOUTQ) + // check bytes waiting to be written (TIOCOUTQ) if ioctl(controller_fd, TIOCOUTQ, &mut controller_outq) < 0 || ioctl(user_fd, TIOCOUTQ, &mut user_outq) < 0 { // break if we can't read - println!("ioctl failed TIOCOUTQ"); break; } } - // If all queues are empty, we're done + // if all queues are empty, we're done if controller_inq == 0 && controller_outq == 0 && user_inq == 0 && user_outq == 0 { break; } - // Apply backoff strategy + // apply backoff strategy if let Some(d) = backoff.next_backoff() { thread::sleep(d); continue; } else { - // We have exhausted our attempts + // we have exhausted our attempts break; } } @@ -257,39 +255,6 @@ impl Pty { // try to wait for the controller fd to be fully read poll_pty_fds_until_read(raw_controller_fd, raw_user_fd); - - // // check TIOCOUTQ to see if there is any data left to be read - // let mut user_outq = 0; - // let user_tiocoutq_res = unsafe { libc::ioctl(raw_user_fd, libc::TIOCOUTQ, &mut user_outq) }; - // if user_tiocoutq_res == -1 { - // eprintln!("ioctl TIOCOUTQ failed: {}", user_tiocoutq_res); - // } - // - // let mut controller_outq = 0; - // let controller_tiocoutq_res = - // unsafe { libc::ioctl(raw_controller_fd, libc::TIOCOUTQ, &mut controller_outq) }; - // if controller_tiocoutq_res == -1 { - // eprintln!("ioctl TIOCOUTQ failed: {}", controller_tiocoutq_res); - // } - // - // let mut user_inq = 0; - // let user_tiocinq_res = unsafe { libc::ioctl(raw_user_fd, libc::FIONREAD, &mut user_inq) }; - // if user_tiocinq_res == -1 { - // eprintln!("ioctl TIOCINQ failed: {}", user_tiocinq_res); - // } - // - // let mut controller_inq = 0; - // let controller_tiocinq_res = - // unsafe { libc::ioctl(raw_controller_fd, libc::FIONREAD, &mut controller_inq) }; - // if controller_tiocinq_res == -1 { - // eprintln!("ioctl TIOCINQ failed: {}", controller_tiocinq_res); - // } - // - // println!( - // "user_outq: {}, controller_outq: {}, user_inq: {}, controller_inq: {}", - // user_outq, controller_outq, user_inq, controller_inq - // ); - drop(user_fd); match wait_result { From 7366a5a9eaee416561692a13cf127c64066d0e7b Mon Sep 17 00:00:00 2001 From: Jacky Zhao Date: Mon, 28 Oct 2024 10:12:53 -0700 Subject: [PATCH 54/60] use -1 instead --- src/lib.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index 52ce892..9d4dd07 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -81,16 +81,16 @@ fn poll_pty_fds_until_read(controller_fd: RawFd, user_fd: RawFd) { // safe because we're passing valid file descriptors and properly sized integers unsafe { // check bytes waiting to be read (FIONREAD, equivalent to TIOCINQ on Linux) - if ioctl(controller_fd, FIONREAD, &mut controller_inq) < 0 - || ioctl(user_fd, FIONREAD, &mut user_inq) < 0 + if ioctl(controller_fd, FIONREAD, &mut controller_inq) == -1 + || ioctl(user_fd, FIONREAD, &mut user_inq) == -1 { // break if we can't read break; } // check bytes waiting to be written (TIOCOUTQ) - if ioctl(controller_fd, TIOCOUTQ, &mut controller_outq) < 0 - || ioctl(user_fd, TIOCOUTQ, &mut user_outq) < 0 + if ioctl(controller_fd, TIOCOUTQ, &mut controller_outq) == -1 + || ioctl(user_fd, TIOCOUTQ, &mut user_outq) == -1 { // break if we can't read break; From a1942945eb7938d78c4ae50cfe55af86b5d7185a Mon Sep 17 00:00:00 2001 From: Jacky Zhao Date: Mon, 28 Oct 2024 10:17:31 -0700 Subject: [PATCH 55/60] run all, remove debug --- package.json | 2 +- tests/index.test.ts | 20 ++++++-------------- wrapper.ts | 10 ++-------- 3 files changed, 9 insertions(+), 23 deletions(-) diff --git a/package.json b/package.json index cf90207..338344b 100644 --- a/package.json +++ b/package.json @@ -39,7 +39,7 @@ "build": "napi build --platform --release && npm run build:wrapper && npm run format", "build:wrapper": "tsup", "prepublishOnly": "napi prepublish -t npm", - "test": "vitest run --allowOnly", + "test": "vitest run", "test:hang": "vitest run --reporter=hanging-process", "universal": "napi universal", "version": "napi version", diff --git a/tests/index.test.ts b/tests/index.test.ts index e8a6116..6ad01be 100644 --- a/tests/index.test.ts +++ b/tests/index.test.ts @@ -283,11 +283,9 @@ describe( expect(onExit).toHaveBeenCalledWith(null, -1); }); - test.only( + test( 'ordering is correct', async () => { - console.log('-- start --') - const oldFds = getOpenFds(); let buffer = Buffer.from(''); const n = 1024; @@ -304,25 +302,19 @@ describe( const readStream = pty.read; readStream.on('data', (data) => { - console.log('data', data.byteLength); buffer = Buffer.concat([buffer, data]); }); - readStream.on('error', (err) => console.log('error', err)); - await vi.waitFor(() => expect(onExit).toHaveBeenCalledTimes(1)); expect(onExit).toHaveBeenCalledWith(null, 0); const lines = buffer.toString().trim().split('\n'); - console.log('got lines', lines.length, 'expected', n + 1); - console.log('readable ended', readStream.readableEnded); - console.log('-- end --') expect(lines.length).toBe(n + 1); - // for (let i = 0; i < n + 1; i++) { - // expect(Number(lines[i]), `expected line ${i} to contain ${i} but got ${lines[i]}`).toBe(i); - // } - // - // expect(getOpenFds()).toStrictEqual(oldFds); + for (let i = 0; i < n + 1; i++) { + expect(Number(lines[i]), `expected line ${i} to contain ${i} but got ${lines[i]}`).toBe(i); + } + + expect(getOpenFds()).toStrictEqual(oldFds); } ); diff --git a/wrapper.ts b/wrapper.ts index 75317b0..e784542 100644 --- a/wrapper.ts +++ b/wrapper.ts @@ -73,7 +73,6 @@ export class Pty { markReadFinished = resolve; }); const mockedExit = (error: NodeJS.ErrnoException | null, code: number) => { - console.log('mocked exit'); markExited({ error, code }); }; @@ -98,21 +97,17 @@ export class Pty { this.#fdClosed = true; - console.log('close'); // must wait for fd close and exit result before calling real exit await readFinished; const result = await exitResult; - console.log('real exit'); realExit(result.error, result.code); }; - this.read.on('end', () => { - console.log('readable finished'); + this.read.once('end', () => { markReadFinished(); }); - this.read.on('close', () => { - console.log('user-side pty fd closed'); + this.read.once('close', () => { handleClose(); }); @@ -132,7 +127,6 @@ export class Pty { // is nothing left to read and we can start tearing things down. If we hadn't received an // error so far, we are considered to be in good standing. this.read.off('error', handleError); - console.log('eio emit'); // emit 'end' to signal no more data // this will trigger our 'end' handler which marks readFinished this.read.emit('end'); From a0b56046a101b04aac3f3b25cb36e832f2132d8f Mon Sep 17 00:00:00 2001 From: Jacky Zhao Date: Mon, 28 Oct 2024 10:36:22 -0700 Subject: [PATCH 56/60] cleanup some more imports --- src/lib.rs | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index 9d4dd07..fd5f2f9 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -1,11 +1,9 @@ use std::collections::HashMap; use std::fs::File; -use std::io; use std::io::Error; use std::io::ErrorKind; -use std::io::Write; use std::os::fd::{AsRawFd, OwnedFd}; -use std::os::fd::{BorrowedFd, FromRawFd, IntoRawFd, RawFd}; +use std::os::fd::{FromRawFd, IntoRawFd, RawFd}; use std::os::unix::process::CommandExt; use std::process::{Command, Stdio}; use std::thread; From 7810ed9d0adc32e51c09c7952ea9fa67fddd19bf Mon Sep 17 00:00:00 2001 From: Jacky Zhao Date: Mon, 28 Oct 2024 10:37:51 -0700 Subject: [PATCH 57/60] re-add write trait --- src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/lib.rs b/src/lib.rs index fd5f2f9..ba73702 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -1,6 +1,6 @@ use std::collections::HashMap; use std::fs::File; -use std::io::Error; +use std::io::{Error, Write}; use std::io::ErrorKind; use std::os::fd::{AsRawFd, OwnedFd}; use std::os::fd::{FromRawFd, IntoRawFd, RawFd}; From 9c853e90a9b745963d9e6766ae6ac936edb4e2f2 Mon Sep 17 00:00:00 2001 From: Jacky Zhao Date: Mon, 28 Oct 2024 10:51:44 -0700 Subject: [PATCH 58/60] 3.4.0 --- npm/darwin-arm64/package.json | 2 +- npm/darwin-x64/package.json | 2 +- npm/linux-x64-gnu/package.json | 2 +- package-lock.json | 4 ++-- package.json | 2 +- 5 files changed, 6 insertions(+), 6 deletions(-) diff --git a/npm/darwin-arm64/package.json b/npm/darwin-arm64/package.json index 725d4b1..75de279 100644 --- a/npm/darwin-arm64/package.json +++ b/npm/darwin-arm64/package.json @@ -1,6 +1,6 @@ { "name": "@replit/ruspty-darwin-arm64", - "version": "3.3.0", + "version": "3.4.0", "os": [ "darwin" ], diff --git a/npm/darwin-x64/package.json b/npm/darwin-x64/package.json index 4cc36e7..886abe1 100644 --- a/npm/darwin-x64/package.json +++ b/npm/darwin-x64/package.json @@ -1,6 +1,6 @@ { "name": "@replit/ruspty-darwin-x64", - "version": "3.3.0", + "version": "3.4.0", "os": [ "darwin" ], diff --git a/npm/linux-x64-gnu/package.json b/npm/linux-x64-gnu/package.json index cf1560f..122c20d 100644 --- a/npm/linux-x64-gnu/package.json +++ b/npm/linux-x64-gnu/package.json @@ -1,6 +1,6 @@ { "name": "@replit/ruspty-linux-x64-gnu", - "version": "3.3.0", + "version": "3.4.0", "os": [ "linux" ], diff --git a/package-lock.json b/package-lock.json index a968647..6cea6de 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,12 +1,12 @@ { "name": "@replit/ruspty", - "version": "3.3.0", + "version": "3.4.0", "lockfileVersion": 3, "requires": true, "packages": { "": { "name": "@replit/ruspty", - "version": "3.3.0", + "version": "3.4.0", "license": "MIT", "devDependencies": { "@napi-rs/cli": "^2.18.2", diff --git a/package.json b/package.json index 338344b..124f422 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "@replit/ruspty", - "version": "3.3.0", + "version": "3.4.0", "main": "dist/wrapper.js", "types": "dist/wrapper.d.ts", "author": "Szymon Kaliski ", From b6c7de294c88cf39db495a7d75309329e27f1a01 Mon Sep 17 00:00:00 2001 From: Jacky Zhao Date: Mon, 28 Oct 2024 13:12:16 -0700 Subject: [PATCH 59/60] try with nix::sys:ioctl --- Cargo.toml | 2 +- src/lib.rs | 68 +++++++++++++++++++++++++++++++++++++++--------------- wrapper.ts | 13 ++++------- 3 files changed, 54 insertions(+), 29 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 36bb6f4..453940c 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -12,7 +12,7 @@ libc = "0.2.152" # Default enable napi4 feature, see https://nodejs.org/api/n-api.html#node-api-version-matrix napi = { version = "2.12.2", default-features = false, features = ["napi4"] } napi-derive = "2.12.2" -nix = { version = "0.29.0", features = ["fs", "term", "poll"] } +nix = { version = "0.29.0", features = ["fs", "term", "poll", "ioctl"] } [build-dependencies] napi-build = "2.0.1" diff --git a/src/lib.rs b/src/lib.rs index ba73702..e4b98c7 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -1,7 +1,7 @@ use std::collections::HashMap; use std::fs::File; -use std::io::{Error, Write}; use std::io::ErrorKind; +use std::io::{Error, Write}; use std::os::fd::{AsRawFd, OwnedFd}; use std::os::fd::{FromRawFd, IntoRawFd, RawFd}; use std::os::unix::process::CommandExt; @@ -11,16 +11,53 @@ use std::time::Duration; use backoff::backoff::Backoff; use backoff::ExponentialBackoffBuilder; -use libc::{self, c_int}; +use libc::{self, c_int, c_ulong}; use napi::bindgen_prelude::JsFunction; use napi::threadsafe_function::{ErrorStrategy, ThreadsafeFunction, ThreadsafeFunctionCallMode}; use napi::Status::GenericFailure; use napi::{self, Env}; use nix::errno::Errno; use nix::fcntl::{fcntl, FcntlArg, FdFlag, OFlag}; -use nix::libc::{FIONREAD, TIOCOUTQ, ioctl}; +use nix::libc::{FIONREAD, TIOCOUTQ, TIOCSCTTY, TIOCSWINSZ}; use nix::pty::{openpty, Winsize}; use nix::sys::termios::{self, SetArg}; +use nix::{ioctl_none, ioctl_read, ioctl_write_ptr}; + +fn ioctl_seq_from_ident(ident: c_ulong) -> u8 { + (ident & 0xff) as u8 +} + +// define the ioctls using nix::sys::ioctl macro +const TIOCSCTTY_IDENT: c_ulong = TIOCSCTTY as c_ulong; +ioctl_none!( + tiocsctty, + TIOCSCTTY_IDENT, + ioctl_seq_from_ident(TIOCSCTTY_IDENT) +); + +const TIOCSWINSZ_IDENT: c_ulong = TIOCSWINSZ; +ioctl_write_ptr!( + tiocswinsz, + TIOCSWINSZ_IDENT, + ioctl_seq_from_ident(TIOCSWINSZ_IDENT), + Winsize +); + +const TIOCOUTQ_IDENT: c_ulong = TIOCOUTQ; +ioctl_read!( + tiocoutq, + TIOCOUTQ_IDENT, + ioctl_seq_from_ident(TIOCOUTQ_IDENT), + c_int +); + +const TIOCINQ_IDENT: c_ulong = FIONREAD; +ioctl_read!( + tiocinq, + TIOCINQ_IDENT, + ioctl_seq_from_ident(TIOCINQ_IDENT), + c_int +); #[macro_use] extern crate napi_derive; @@ -78,19 +115,17 @@ fn poll_pty_fds_until_read(controller_fd: RawFd, user_fd: RawFd) { // safe because we're passing valid file descriptors and properly sized integers unsafe { - // check bytes waiting to be read (FIONREAD, equivalent to TIOCINQ on Linux) - if ioctl(controller_fd, FIONREAD, &mut controller_inq) == -1 - || ioctl(user_fd, FIONREAD, &mut user_inq) == -1 + // check bytes waiting to be read (TIOCINQ on Linux) + if tiocinq(controller_fd, &mut controller_inq).is_err() + || tiocinq(user_fd, &mut user_inq).is_err() { - // break if we can't read break; } // check bytes waiting to be written (TIOCOUTQ) - if ioctl(controller_fd, TIOCOUTQ, &mut controller_outq) == -1 - || ioctl(user_fd, TIOCOUTQ, &mut user_outq) == -1 + if tiocoutq(controller_fd, &mut controller_outq).is_err() + || tiocoutq(user_fd, &mut user_outq).is_err() { - // break if we can't read break; } } @@ -189,9 +224,8 @@ impl Pty { } // become the controlling tty for the program - let err = libc::ioctl(raw_user_fd, libc::TIOCSCTTY.into(), 0); - if err == -1 { - return Err(Error::new(ErrorKind::Other, "ioctl-TIOCSCTTY")); + if let Err(err) = tiocsctty(raw_user_fd) { + return Err(Error::new(ErrorKind::Other, format!("ioctl-TIOCSCTTY: {}", err))); } // we need to drop the controller fd, since we don't need it in the child @@ -315,12 +349,8 @@ fn pty_resize(fd: i32, size: Size) -> Result<(), napi::Error> { ws_ypixel: 0, }; - let res = unsafe { libc::ioctl(fd, libc::TIOCSWINSZ, &window_size as *const _) }; - if res == -1 { - return Err(napi::Error::new( - napi::Status::GenericFailure, - format!("ioctl TIOCSWINSZ failed: {}", Error::last_os_error()), - )); + unsafe { + tiocswinsz(fd, &window_size).map_err(cast_to_napi_error)?; } Ok(()) diff --git a/wrapper.ts b/wrapper.ts index e784542..6babfc5 100644 --- a/wrapper.ts +++ b/wrapper.ts @@ -63,12 +63,12 @@ export class Pty { constructor(options: PtyOptions) { const realExit = options.onExit; - let markExited: (value: ExitResult) => void; + let markExited!: (value: ExitResult) => void; let exitResult: Promise = new Promise((resolve) => { markExited = resolve; }); - let markReadFinished: () => void; + let markReadFinished!: () => void; let readFinished = new Promise((resolve) => { markReadFinished = resolve; }); @@ -103,13 +103,8 @@ export class Pty { realExit(result.error, result.code); }; - this.read.once('end', () => { - markReadFinished(); - }); - - this.read.once('close', () => { - handleClose(); - }); + this.read.once('end', markReadFinished); + this.read.once('close', handleClose); // PTYs signal their done-ness with an EIO error. we therefore need to filter them out (as well as // cleaning up other spurious errors) so that the user doesn't need to handle them and be in From 9f5a4e9211c8a3ffbc6b3b4904ef6613b33c8dcc Mon Sep 17 00:00:00 2001 From: Jacky Zhao Date: Mon, 28 Oct 2024 13:31:49 -0700 Subject: [PATCH 60/60] back to raw ioctl we go --- Cargo.toml | 2 +- src/lib.rs | 65 ++++++++++++++---------------------------------------- 2 files changed, 18 insertions(+), 49 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 453940c..36bb6f4 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -12,7 +12,7 @@ libc = "0.2.152" # Default enable napi4 feature, see https://nodejs.org/api/n-api.html#node-api-version-matrix napi = { version = "2.12.2", default-features = false, features = ["napi4"] } napi-derive = "2.12.2" -nix = { version = "0.29.0", features = ["fs", "term", "poll", "ioctl"] } +nix = { version = "0.29.0", features = ["fs", "term", "poll"] } [build-dependencies] napi-build = "2.0.1" diff --git a/src/lib.rs b/src/lib.rs index e4b98c7..59ca4ff 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -11,53 +11,15 @@ use std::time::Duration; use backoff::backoff::Backoff; use backoff::ExponentialBackoffBuilder; -use libc::{self, c_int, c_ulong}; use napi::bindgen_prelude::JsFunction; use napi::threadsafe_function::{ErrorStrategy, ThreadsafeFunction, ThreadsafeFunctionCallMode}; use napi::Status::GenericFailure; use napi::{self, Env}; use nix::errno::Errno; use nix::fcntl::{fcntl, FcntlArg, FdFlag, OFlag}; -use nix::libc::{FIONREAD, TIOCOUTQ, TIOCSCTTY, TIOCSWINSZ}; +use nix::libc::{self, c_int, ioctl, FIONREAD, TIOCOUTQ, TIOCSCTTY, TIOCSWINSZ}; use nix::pty::{openpty, Winsize}; use nix::sys::termios::{self, SetArg}; -use nix::{ioctl_none, ioctl_read, ioctl_write_ptr}; - -fn ioctl_seq_from_ident(ident: c_ulong) -> u8 { - (ident & 0xff) as u8 -} - -// define the ioctls using nix::sys::ioctl macro -const TIOCSCTTY_IDENT: c_ulong = TIOCSCTTY as c_ulong; -ioctl_none!( - tiocsctty, - TIOCSCTTY_IDENT, - ioctl_seq_from_ident(TIOCSCTTY_IDENT) -); - -const TIOCSWINSZ_IDENT: c_ulong = TIOCSWINSZ; -ioctl_write_ptr!( - tiocswinsz, - TIOCSWINSZ_IDENT, - ioctl_seq_from_ident(TIOCSWINSZ_IDENT), - Winsize -); - -const TIOCOUTQ_IDENT: c_ulong = TIOCOUTQ; -ioctl_read!( - tiocoutq, - TIOCOUTQ_IDENT, - ioctl_seq_from_ident(TIOCOUTQ_IDENT), - c_int -); - -const TIOCINQ_IDENT: c_ulong = FIONREAD; -ioctl_read!( - tiocinq, - TIOCINQ_IDENT, - ioctl_seq_from_ident(TIOCINQ_IDENT), - c_int -); #[macro_use] extern crate napi_derive; @@ -115,17 +77,19 @@ fn poll_pty_fds_until_read(controller_fd: RawFd, user_fd: RawFd) { // safe because we're passing valid file descriptors and properly sized integers unsafe { - // check bytes waiting to be read (TIOCINQ on Linux) - if tiocinq(controller_fd, &mut controller_inq).is_err() - || tiocinq(user_fd, &mut user_inq).is_err() + // check bytes waiting to be read (FIONREAD, equivalent to TIOCINQ on Linux) + if ioctl(controller_fd, FIONREAD, &mut controller_inq) == -1 + || ioctl(user_fd, FIONREAD, &mut user_inq) == -1 { + // break if we can't read break; } // check bytes waiting to be written (TIOCOUTQ) - if tiocoutq(controller_fd, &mut controller_outq).is_err() - || tiocoutq(user_fd, &mut user_outq).is_err() + if ioctl(controller_fd, TIOCOUTQ, &mut controller_outq) == -1 + || ioctl(user_fd, TIOCOUTQ, &mut user_outq) == -1 { + // break if we can't read break; } } @@ -224,8 +188,9 @@ impl Pty { } // become the controlling tty for the program - if let Err(err) = tiocsctty(raw_user_fd) { - return Err(Error::new(ErrorKind::Other, format!("ioctl-TIOCSCTTY: {}", err))); + let err = libc::ioctl(raw_user_fd, TIOCSCTTY.into(), 0); + if err == -1 { + return Err(Error::new(ErrorKind::Other, "ioctl-TIOCSCTTY")); } // we need to drop the controller fd, since we don't need it in the child @@ -349,8 +314,12 @@ fn pty_resize(fd: i32, size: Size) -> Result<(), napi::Error> { ws_ypixel: 0, }; - unsafe { - tiocswinsz(fd, &window_size).map_err(cast_to_napi_error)?; + let res = unsafe { libc::ioctl(fd, TIOCSWINSZ, &window_size as *const _) }; + if res == -1 { + return Err(napi::Error::new( + napi::Status::GenericFailure, + format!("ioctl TIOCSWINSZ failed: {}", Error::last_os_error()), + )); } Ok(())