From f173e52b9d659b33a5d591f5327d656ca9f41306 Mon Sep 17 00:00:00 2001 From: Jacky Zhao Date: Fri, 20 Sep 2024 13:14:40 -0700 Subject: [PATCH 1/4] dont try to resize if we invalidated the fd --- tests/index.test.ts | 19 +++++++++++++++++++ wrapper.ts | 10 +++++++--- 2 files changed, 26 insertions(+), 3 deletions(-) diff --git a/tests/index.test.ts b/tests/index.test.ts index ae2bda0..c2b484d 100644 --- a/tests/index.test.ts +++ b/tests/index.test.ts @@ -254,6 +254,25 @@ describe( }); })); + test('resize after exit shouldn\'t throw', () => new Promise((done, reject) => { + 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) + } + }, + }); + })); + test( 'ordering is correct', () => diff --git a/wrapper.ts b/wrapper.ts index 97c018c..e4796ea 100644 --- a/wrapper.ts +++ b/wrapper.ts @@ -45,6 +45,7 @@ type ExitResult = { export class Pty { #pty: RawPty; #fd: number; + #fdEnded: boolean = false; #socket: ReadStream; read: Readable; @@ -78,13 +79,12 @@ export class Pty { this.write = userFacingWrite; // catch end events - let handleCloseCalled = false; const handleClose = () => { - if (handleCloseCalled) { + if (this.#fdEnded) { return; } - handleCloseCalled = true; + this.#fdEnded = true; exitResult.then((result) => realExit(result.error, result.code)); userFacingRead.end(); }; @@ -122,6 +122,10 @@ export class Pty { } resize(size: Size) { + if (this.#fdEnded) { + return; + } + ptyResize(this.#fd, size); } From efe4ef2c1d28abdb6385a9f3ecd3c8cbaec4bb5d Mon Sep 17 00:00:00 2001 From: Jacky Zhao Date: Fri, 4 Oct 2024 11:07:53 -0700 Subject: [PATCH 2/4] end instead of destroy, fcntl check --- src/lib.rs | 8 ++++++++ tests/index.test.ts | 27 +++++++++++++++++++++++---- wrapper.ts | 12 +++++++----- 3 files changed, 38 insertions(+), 9 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index beb2ab2..9dfff45 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -303,6 +303,14 @@ impl Pty { #[napi] #[allow(dead_code)] fn pty_resize(fd: i32, size: Size) -> Result<(), napi::Error> { + // check the fd is still valid before we try to resize + if unsafe { libc::fcntl(fd, libc::F_GETFD) } == -1 { + return Err(napi::Error::new( + napi::Status::GenericFailure, + "fd not valid for resize", + )); + } + let window_size = Winsize { ws_col: size.cols, ws_row: size.rows, diff --git a/tests/index.test.ts b/tests/index.test.ts index c2b484d..50496f6 100644 --- a/tests/index.test.ts +++ b/tests/index.test.ts @@ -47,6 +47,7 @@ function getOpenFds(): FdRecord { describe( 'PTY', + { repeats: 50 }, () => { test('spawns and exits', () => new Promise((done) => { @@ -273,6 +274,26 @@ describe( }); })); + test('resize after close shouldn\'t throw', () => new Promise((done, reject) => { + const pty = new Pty({ + command: '/bin/sh', + onExit: (err, exitCode) => { + try { + expect(err).toBeNull(); + expect(exitCode).toBe(0); + } catch (e) { + reject(e) + } + }, + }); + + pty.close(); + expect(() => { + pty.resize({ rows: 60, cols: 100 }); + }).not.toThrow(); + done(); + })); + test( 'ordering is correct', () => @@ -302,11 +323,11 @@ describe( buffer = Buffer.concat([buffer, data]); }); }), - { repeats: 1 }, ); testSkipOnDarwin( 'does not leak files', + { repeats: 4 }, () => new Promise((done) => { const oldFds = getOpenFds(); @@ -348,11 +369,11 @@ describe( done(); }); }), - { repeats: 4 }, ); test( 'can run concurrent shells', + { repeats: 4 }, () => new Promise((done) => { const oldFds = getOpenFds(); @@ -414,7 +435,6 @@ describe( done(); }); }), - { repeats: 4 }, ); test("doesn't break when executing non-existing binary", () => @@ -434,7 +454,6 @@ describe( } })); }, - { repeats: 50 }, ); describe('cgroup opts', () => { diff --git a/wrapper.ts b/wrapper.ts index e4796ea..c63853c 100644 --- a/wrapper.ts +++ b/wrapper.ts @@ -85,12 +85,14 @@ export class Pty { } this.#fdEnded = true; - exitResult.then((result) => realExit(result.error, result.code)); + exitResult.then((result) => { + realExit(result.error, result.code) + }); userFacingRead.end(); }; this.#socket.on('close', handleClose); - // PTYs signal their donness with an EIO error. we therefore need to filter them out (as well as + // 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) => { @@ -103,11 +105,11 @@ export class Pty { return; } if (code.indexOf('EIO') !== -1) { - // EIO only happens when the child dies . It is therefore our only true signal that there + // 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.#socket.off('error', handleError); - this.#socket.destroy(); + this.#socket.end(); return; } } @@ -118,7 +120,7 @@ export class Pty { } close() { - this.#socket.destroy(); + this.#socket.end(); } resize(size: Size) { From 5ecd2f761c501b29ad4e526270651aa7180a97f7 Mon Sep 17 00:00:00 2001 From: Jacky Zhao Date: Fri, 4 Oct 2024 12:12:57 -0700 Subject: [PATCH 3/4] check error in wrapper instead of in rust --- src/lib.rs | 8 -------- wrapper.ts | 13 ++++++++++++- 2 files changed, 12 insertions(+), 9 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index 9dfff45..beb2ab2 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -303,14 +303,6 @@ impl Pty { #[napi] #[allow(dead_code)] fn pty_resize(fd: i32, size: Size) -> Result<(), napi::Error> { - // check the fd is still valid before we try to resize - if unsafe { libc::fcntl(fd, libc::F_GETFD) } == -1 { - return Err(napi::Error::new( - napi::Status::GenericFailure, - "fd not valid for resize", - )); - } - let window_size = Winsize { ws_col: size.cols, ws_row: size.rows, diff --git a/wrapper.ts b/wrapper.ts index c63853c..9b6d5cb 100644 --- a/wrapper.ts +++ b/wrapper.ts @@ -128,7 +128,18 @@ export class Pty { return; } - ptyResize(this.#fd, size); + 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; + } } get pid() { From f827952034793ab41a68d4e75cffe4bb37975c98 Mon Sep 17 00:00:00 2001 From: Jacky Zhao Date: Fri, 4 Oct 2024 13:38:54 -0700 Subject: [PATCH 4/4] add comment --- wrapper.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/wrapper.ts b/wrapper.ts index 9b6d5cb..fe3d5f9 100644 --- a/wrapper.ts +++ b/wrapper.ts @@ -120,6 +120,8 @@ export class Pty { } close() { + // 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(); } @@ -136,7 +138,7 @@ export class Pty { // exited but we don't know about it yet. In that case, we just ignore the error. return; } - + // otherwise, rethrow throw e; }