child_process: emit .send() errors in next tick

phillipj · phillipj · commit 3c38e877bbf3 · 2016-03-09T22:44:36.000+01:00
This changes the behaviour of error events emitted when process.send()
fails, from synchronously to asynchronously.
diff --git a/lib/internal/child_process.js b/lib/internal/child_process.js
@@ -524,7 +524,7 @@ function setupChannel(target, channel) {
     if (typeof callback === 'function') {
       process.nextTick(callback, ex);
     } else {
-      this.emit('error', ex);  // FIXME(bnoordhuis) Defer to next tick.
+      process.nextTick(() => this.emit('error', ex));
     }
     return false;
   };
@@ -635,7 +635,7 @@ function setupChannel(target, channel) {
         if (typeof callback === 'function') {
           process.nextTick(callback, ex);
         } else {
-          this.emit('error', ex);  // FIXME(bnoordhuis) Defer to next tick.
+          process.nextTick(() => this.emit('error', ex));
         }
       }
     }
diff --git a/test/parallel/test-child-process-send-disconnected-err.js b/test/parallel/test-child-process-send-disconnected-err.js
@@ -0,0 +1,58 @@
+'use strict';
+
+require('../common');
+
+const assert = require('assert');
+const fork = require('child_process').fork;
+const Pipe = process.binding('pipe_wrap').Pipe;
+
+const NODE_CHANNEL_FD = 3;  // equals file descriptor id of IPC created
+                            // by parent process and passed down to
+                            // child proc with process.env.NODE_CHANNEL_FD
+
+if (process.argv[2] !== 'child') {
+  const child = fork(__filename, ['child'], {
+    execArgv: ['--expose-internals']
+  });
+
+  child.on('exit', (exitCode) => process.exit(exitCode));
+
+  return;
+}
+
+let errorsEmitted = 0;
+
+const errorSpy = (err) => {
+  if (err && err.message === 'channel closed') {
+    return errorsEmitted++;
+  }
+
+  throw new Error('This callback should have been called with an error!');
+};
+
+const setupChannel = require('internal/child_process').setupChannel;
+
+const channel = new Pipe(true);
+channel.open(NODE_CHANNEL_FD);
+channel.unref();
+setupChannel(process, channel);
+
+// disconnecting the "target" will trigger errors on .send()
+process.connected = false;
+
+process.send('A message to send..', errorSpy);
+assert.equal(errorsEmitted, 0,
+  'Waits until next tick before invoking .send() callback');
+
+process.nextTick(() => {
+  assert.equal(errorsEmitted, 1);
+
+  process.on('error', errorSpy);
+  process.send('A message to send..');
+  assert.equal(errorsEmitted, 1,
+    'Waits until next tick before emitting error');
+
+  process.nextTick(() => {
+    assert.equal(errorsEmitted, 2);
+  });
+});
diff --git a/test/parallel/test-child-process-send-err.js b/test/parallel/test-child-process-send-err.js
@@ -0,0 +1,59 @@
+'use strict';
+
+require('../common');
+
+const assert = require('assert');
+const fork = require('child_process').fork;
+const Pipe = process.binding('pipe_wrap').Pipe;
+
+const UV_UNKNOWN_ERR = -4094;
+const NODE_CHANNEL_FD = 3;  // equals file descriptor id of IPC created
+                            // by parent process and passed down to
+                            // child proc with process.env.NODE_CHANNEL_FD
+
+if (process.argv[2] !== 'child') {
+  const child = fork(__filename, ['child'], {
+    execArgv: ['--expose-internals']
+  });
+
+  child.on('exit', (exitCode) => process.exit(exitCode));
+  return;
+}
+
+let errorsEmitted = 0;
+
+const errorSpy = (err) => {
+  // UV_UNKNOWN error triggered by .writeUtf8String() below
+  if (err && err.code === 'UNKNOWN') {
+    return errorsEmitted++;
+  }
+
+  throw new Error('This callback should have been called with an error!');
+};
+
+const setupChannel = require('internal/child_process').setupChannel;
+
+const channel = new Pipe(true);
+channel.open(NODE_CHANNEL_FD);
+channel.unref();
+setupChannel(process, channel);
+
+// fake UV_UNKNOWN error
+channel.writeUtf8String = () => UV_UNKNOWN_ERR;
+
+process._send('A message to send..', undefined, false, errorSpy);
+assert.equal(errorsEmitted, 0,
+  'Waits until next tick before invoking ._send() callback');
+
+process.nextTick(() => {
+  assert.equal(errorsEmitted, 1);
+
+  process.on('error', errorSpy);
+  process.send('A message to send..');
+  assert.equal(errorsEmitted, 1,
+    'Waits until next tick before emitting error');
+
+  process.nextTick(() => {
+    assert.equal(errorsEmitted, 2);
+  });
+});

Original file line number	Diff line number	Diff line change
`@@ -524,7 +524,7 @@ function setupChannel(target, channel) {`
`524`	`524`	`if (typeof callback === 'function') {`
`525`	`525`	`process.nextTick(callback, ex);`
`526`	`526`	`} else {`
`527`		`- this.emit('error', ex); // FIXME(bnoordhuis) Defer to next tick.`
	`527`	`+ process.nextTick(() => this.emit('error', ex));`
`528`	`528`	`}`
`529`	`529`	`return false;`
`530`	`530`	`};`
`@@ -635,7 +635,7 @@ function setupChannel(target, channel) {`
`635`	`635`	`if (typeof callback === 'function') {`
`636`	`636`	`process.nextTick(callback, ex);`
`637`	`637`	`} else {`
`638`		`- this.emit('error', ex); // FIXME(bnoordhuis) Defer to next tick.`
	`638`	`+ process.nextTick(() => this.emit('error', ex));`
`639`	`639`	`}`
`640`	`640`	`}`
`641`	`641`	`}`