stream: change in stdout/stderr events from child process (missing `finish`)

[travisperson]

• Version: v9.9.0
• Platform: 4.15.13-1-ARCH deps: update openssl to 1.0.1j #1 SMP PREEMPT Sun Mar 25 11:27:57 UTC 2018 x86_64 GNU/Linux
• Subsystem: stream

I believe commit 9c0c0e68ac introduced a breaking change to what events are emitted from child process streams stdout/stderr.

const { spawn } = require('child_process')

const ps = spawn('ls', ['-al'])

ps.stdout.on('finish', function() {
  console.log('Called `finish` on stdout')
});

ps.stdout.on('end', function() {
  console.log('Called `end` on stdout')
});

ps.stderr.on('finish', function() {
  console.log('Called `finish` on stderr')
});

ps.stderr.on('end', function() {
  console.log('Called `end` on stderr')
});

In node v9.9.0

Called `end` on stderr
Called `end` on stdout

In node v9.8.0

Called `end` on stderr
Called `finish` on stderr
Called `end` on stdout
Called `finish` on stdout

Per the documentation, finish should only be associated with a writable stream, so the current behavior in 9.9.0 seems to be accurate. Given that, I'm not sure if the previous behavior was a bug or not.

[lpinca]

Analysis seems correct. The following patch is untested but should fix the issue:

diff --git a/lib/net.js b/lib/net.js
index 5b460befa4..324f99804a 100644
--- a/lib/net.js
+++ b/lib/net.js
@@ -241,11 +241,11 @@ function Socket(options) {
   this[kTimeout] = null;
 
   if (typeof options === 'number')
-    options = { fd: options }; // Legacy interface.
+    options = { allowHalfOpen: false, fd: options }; // Legacy interface.
   else if (options === undefined)
-    options = {};
+    options = { allowHalfOpen: false };
   else
-    options = util._extend({}, options);
+    options = util._extend({ allowHalfOpen: false }, options);
 
   // For backwards compat do not emit close on destroy.
   options.emitClose = false;
@@ -300,9 +300,6 @@ function Socket(options) {
   // handle strings directly
   this._writableState.decodeStrings = false;
 
-  // default to *not* allowing half open sockets
-  this.allowHalfOpen = options.allowHalfOpen || false;
-
   // if we have a handle, then start the flow of data into the
   // buffer.  if not, then this will happen when we connect
   if (this._handle && options.readable !== false) {

I will test it properly and open a PR.

[lpinca]

On second thought I don't think it's 9c0c0e6 that caused the regression as this https://github.com/nodejs/node/blob/v9.9.0/lib/net.js#L613-L616 should call Socket.prototype.end() regardless of the "no-half-open" enforcer.

This needs a bisect.

[lpinca]

Ok, yes 9c0c0e6 is the first bad commit.
The issue happens because the socket is not writable so Socket.prototype.end() is not called.

The no-half-open enforcer was calling Socket.prototype.end() even if the socket was not writable.

I think the changed behavior is actually a bug fix as calling writable.end() on a technically readable only stream doesn't make sense but since people were relying on it we can apply the patch suggested above on v9.x after reverting 152c931 and 5b12d3a and tag #18974 as semver-major.

Makes sense?

[travisperson]

Yes, I think you are correct in this behavior change is a bug fix. The change does better reflect documentation on streams as finish should only be associated with a writable stream. Pushing #18974 to a semver-major seems appropriate.

Thank you for divining into this so quickly!