3.2.2 end tty stream cleanly on error (#46)

- don't `.destroy`, `.end` instead
- add previously failing test `resize after close shouldn\'t throw`
- check the fd is still valid before we try to resize

tests/index.test.ts

@@ -47,6 +47,7 @@ function getOpenFds(): FdRecord {
 
 describe(
   'PTY',
+  { repeats: 50 },
   () => {
     test('spawns and exits', () =>
       new Promise<void>((done) => {
@@ -273,6 +274,26 @@ describe(
       });
     }));
 
+    test('resize after close shouldn\'t throw', () => new Promise<void>((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<void>((done) => {
           const oldFds = getOpenFds();
@@ -348,11 +369,11 @@ describe(
             done();
           });
         }),
-      { repeats: 4 },
     );
 
     test(
       'can run concurrent shells',
+      { repeats: 4 },
       () =>
         new Promise<void>((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', () => {

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,15 +120,28 @@ export class Pty {
   }
 
   close() {
-    this.#socket.destroy();
+    // 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.#fdEnded) {
       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() {