unreliable proc.on('close') event

[justinmk]

This is a tracking issue for a potential issue noticed in #414 (comment)

Problem

proc.on('close') does not always trigger on nvim.quit(). Based on the failures demonstrated in #418, it appears that sometimes the child (nvim) does not close the stderr pipe:

    expect(received).toStrictEqual(expected) // deep equality

    - Expected  - 1
    + Received  + 1

      Object {
        "errors": 0,
    -   "stderrClosed": true,
    +   "stderrClosed": false,
        "stdoutClosed": true,
      }

      172 |     // TODO: 'close' event sometimes does not emit. #414
      173 |     proc.on('exit', () => {
    > 174 |       expect(r).toStrictEqual({
          |                 ^
      175 |         stdoutClosed: true,
      176 |         stderrClosed: true,
      177 |         errors: 0,

      at ChildProcess.toStrictEqual (src/attach/attach.test.ts:174:17)

Solution

• document that proc.on('close') is unreliable.
• document proc.on('exit') as a workaround.
• fix the issue(?) in Nvim itself. I thought it might be related to this: https://github.com/neovim/neovim/blob/c4762b309714897615607f135aab9d7bcc763c4f/src/nvim/channel.c#L168-L171

  // Don't close on exit, in case late error messages
  if (!exiting) {
    fclose(stderr);
  }

  but, forcing fclose(stderr) does not pass the tests in this PR. Nor does adding fclose(stderr) anywhere else AFAICT.

[justinmk]

patch to demo the issue, from #418

diff --git a/packages/neovim/src/attach/attach.test.ts b/packages/neovim/src/attach/attach.test.ts
index 70d16597..4f0c6942 100644
--- a/packages/neovim/src/attach/attach.test.ts
+++ b/packages/neovim/src/attach/attach.test.ts
@@ -154,14 +154,38 @@ describe('Nvim API', () => {
     expect(newLines).toEqual(['line1', 'line2']);
   });
 
-  it('emits "disconnect" after quit', done => {
+  it.only('emits "disconnect" after quit', done => {
     const disconnectMock = jest.fn();
     nvim.on('disconnect', disconnectMock);
 
     nvim.quit();
 
+    const r = {
+      stdoutClosed: false,
+      stderrClosed: false,
+      errors: 0,
+    };
+    proc.stdout.on('close', () => {
+      r.stdoutClosed = true;
+    });
+    proc.stderr.on('close', () => {
+      r.stderrClosed = true;
+    });
+    proc.on('error', () => {
+      r.errors = r.errors + 1;
+    });
+
     // TODO: 'close' event sometimes does not emit. #414
     proc.on('exit', () => {
+      expect(r).toStrictEqual({
+        stdoutClosed: true,
+        stderrClosed: true,
+        errors: 0,
+      });
+      done();
+    });
+
+    proc.on('close', () => {
       expect(disconnectMock).toHaveBeenCalledTimes(1);
       done();
     });

[Sadaf-A]

Greetings @justinmk ! is this still up for contribution? can I take this up?

[justinmk]

any issue that is still open is up for contribution.