net: make readable/writable start as true

`net.Socket` is slightly breaking stream invariants by
having readable/writable going from `false` to `true`.
Streams assume that readable/writable starts out `true`
and then goes to `false` through `push(null)`/`end()`
after which it never goes back to `true`, e.g. once a
stream is `writable == false` it is assumed it will
never become `true`.

This PR changes 2 things:

Unless explicitly set to `false` through options:

- starts as `readable`/`writable` `true` by default.
- uses `push(null)`/`end()` to set `readable`/`writable`
  to `false`. Note that this would cause the socket to
  emit the `'end'`/`'finish'` events, which it did not
  do previously.

In the case it is explicitly set to `false` through
options` it is assumed to never become `true`.

PR-URL: nodejs#32272
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: James M Snell <[email protected]>

Author: ronag

lib/_stream_readable.js

@@ -23,7 +23,6 @@
 
 const {
   ArrayIsArray,
-  Boolean,
   NumberIsInteger,
   NumberIsNaN,
   ObjectDefineProperties,
@@ -1061,9 +1060,12 @@ ObjectDefineProperties(Readable.prototype, {
   readable: {
     get() {
       const r = this._readableState;
-      if (!r) return false;
-      if (r.readable !== undefined) return r.readable && !r.endEmitted;
-      return Boolean(!r.destroyed && !r.errorEmitted && !r.endEmitted);
+      // r.readable === false means that this is part of a Duplex stream
+      // where the readable side was disabled upon construction.
+      // Compat. The user might manually disable readable side through
+      // deprecated setter.
+      return !!r && r.readable !== false && !r.destroyed && !r.errorEmitted &&
+        !r.endEmitted;
     },
     set(val) {
       // Backwards compat.

lib/_stream_writable.js

@@ -27,7 +27,6 @@
 
 const {
   Array,
-  Boolean,
   FunctionPrototype,
   ObjectDefineProperty,
   ObjectDefineProperties,
@@ -744,9 +743,12 @@ ObjectDefineProperties(Writable.prototype, {
   writable: {
     get() {
       const w = this._writableState;
-      if (!w) return false;
-      if (w.writable !== undefined) return w.writable && !w.ended;
-      return Boolean(!w.destroyed && !w.errored && !w.ending);
+      // w.writable === false means that this is part of a Duplex stream
+      // where the writable side was disabled upon construction.
+      // Compat. The user might manually disable writable side through
+      // deprecated setter.
+      return !!w && w.writable !== false && !w.destroyed && !w.errored &&
+        !w.ending;
     },
     set(val) {
       // Backwards compatible.

lib/_tls_wrap.js

@@ -494,9 +494,7 @@ function TLSSocket(socket, opts) {
     handle: this._wrapHandle(wrap),
     allowHalfOpen: socket ? socket.allowHalfOpen : tlsOptions.allowHalfOpen,
     pauseOnCreate: tlsOptions.pauseOnConnect,
-    // The readable flag is only needed if pauseOnCreate will be handled.
-    readable: tlsOptions.pauseOnConnect,
-    writable: false
+    manualStart: true
   });
 
   // Proxy for API compatibility
@@ -506,11 +504,6 @@ function TLSSocket(socket, opts) {
 
   this._init(socket, wrap);
 
-  // Make sure to setup all required properties like: `connecting` before
-  // starting the flow of the data
-  this.readable = true;
-  this.writable = true;
-
   if (enableTrace && this._handle)
     this._handle.enableTrace();
 

lib/net.js

@@ -285,8 +285,6 @@ function Socket(options) {
   else
     options = { ...options };
 
-  options.readable = options.readable || false;
-  options.writable = options.writable || false;
   const { allowHalfOpen } = options;
 
   // Prevent the "no-half-open enforcer" from being inherited from `Duplex`.
@@ -655,8 +653,6 @@ Socket.prototype._destroy = function(exception, cb) {
 
   this.connecting = false;
 
-  this.readable = this.writable = false;
-
   for (let s = this; s !== null; s = s._parent) {
     clearTimeout(s[kTimeout]);
   }
@@ -1120,9 +1116,13 @@ function afterConnect(status, handle, req, readable, writable) {
   self._sockname = null;
 
   if (status === 0) {
-    self.readable = readable;
-    if (!self._writableState.ended)
-      self.writable = writable;
+    if (self.readable && !readable) {
+      self.push(null);
+      self.read();
+    }
+    if (self.writable && !writable) {
+      self.end();
+    }
     self._unrefTimer();
 
     self.emit('connect');

test/parallel/test-net-socket-constructor.js

@@ -27,12 +27,12 @@ function test(sock, readable, writable) {
 }
 
 if (cluster.isMaster) {
-  test(undefined, false, false);
+  test(undefined, true, true);
 
   const server = net.createServer(common.mustCall((socket) => {
     socket.unref();
     test(socket, true, true);
-    test({ handle: socket._handle }, false, false);
+    test({ handle: socket._handle }, true, true);
     test({ handle: socket._handle, readable: true, writable: true },
          true, true);
     server.close();
@@ -45,7 +45,7 @@ if (cluster.isMaster) {
       socket.end();
     }));
 
-    test(socket, false, true);
+    test(socket, true, true);
   }));
 
   cluster.setupMaster({
@@ -58,8 +58,8 @@ if (cluster.isMaster) {
     assert.strictEqual(signal, null);
   }));
 } else {
-  test(4, false, false);
-  test({ fd: 5 }, false, false);
+  test(4, true, true);
+  test({ fd: 5 }, true, true);
   test({ fd: 6, readable: true, writable: true }, true, true);
   process.disconnect();
 }

test/parallel/test-net-socket-setnodelay.js

@@ -13,28 +13,32 @@ const genSetNoDelay = (desiredArg) => (enable) => {
 // setNoDelay should default to true
 let socket = new net.Socket({
   handle: {
-    setNoDelay: common.mustCall(genSetNoDelay(true))
+    setNoDelay: common.mustCall(genSetNoDelay(true)),
+    readStart() {}
   }
 });
 socket.setNoDelay();
 
 socket = new net.Socket({
   handle: {
-    setNoDelay: common.mustCall(genSetNoDelay(true), 1)
+    setNoDelay: common.mustCall(genSetNoDelay(true), 1),
+    readStart() {}
   }
 });
 truthyValues.forEach((testVal) => socket.setNoDelay(testVal));
 
 socket = new net.Socket({
   handle: {
-    setNoDelay: common.mustNotCall()
+    setNoDelay: common.mustNotCall(),
+    readStart() {}
   }
 });
 falseyValues.forEach((testVal) => socket.setNoDelay(testVal));
 
 socket = new net.Socket({
   handle: {
-    setNoDelay: common.mustCall(() => {}, 3)
+    setNoDelay: common.mustCall(() => {}, 3),
+    readStart() {}
   }
 });
 truthyValues.concat(falseyValues).concat(truthyValues)
@@ -44,7 +48,8 @@ truthyValues.concat(falseyValues).concat(truthyValues)
 // In the case below, if it is called an exception will be thrown
 socket = new net.Socket({
   handle: {
-    setNoDelay: null
+    setNoDelay: null,
+    readStart() {}
   }
 });
 const returned = socket.setNoDelay(true);