From e559842188f541b884abff2ffad4d2d3e1b841a6 Mon Sep 17 00:00:00 2001
From: Robert Nagy <ronagy@icloud.com>
Date: Sun, 5 Jan 2020 18:41:31 +0100
Subject: [PATCH] stream: make readable & writable computed

This makes readable and writable automatically computed based
on the stream state.

Effectivly deprecating/discouraging manual management of this.

Makes the properties  more consistent and easier to reason about.

Fixes: https://github.com/nodejs/node/issues/29377

PR-URL: https://github.com/nodejs/node/pull/31197
Refs: https://github.com/nodejs/node/issues/29377
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
---
 doc/api/stream.md                             |  6 ++-
 lib/_http_incoming.js                         |  2 -
 lib/_stream_duplex.js                         |  2 +
 lib/_stream_readable.js                       | 19 +++++--
 lib/_stream_writable.js                       | 19 +++++--
 test/parallel/test-http2-socket-proxy.js      | 12 +++--
 .../parallel/test-stream-readable-readable.js | 45 +++++++++++++++++
 .../parallel/test-stream-writable-writable.js | 49 +++++++++++++++++++
 8 files changed, 139 insertions(+), 15 deletions(-)
 create mode 100644 test/parallel/test-stream-readable-readable.js
 create mode 100644 test/parallel/test-stream-writable-writable.js

diff --git a/doc/api/stream.md b/doc/api/stream.md
index 82cf99b4332000..cf3ecd461c13ed 100644
--- a/doc/api/stream.md
+++ b/doc/api/stream.md
@@ -495,7 +495,8 @@ added: v11.4.0
 
 * {boolean}
 
-Is `true` if it is safe to call [`writable.write()`][stream-write].
+Is `true` if it is safe to call [`writable.write()`][stream-write], which means
+the stream has not been destroyed, errored or ended.
 
 ##### `writable.writableEnded`
 <!-- YAML
@@ -1134,7 +1135,8 @@ added: v11.4.0
 
 * {boolean}
 
-Is `true` if it is safe to call [`readable.read()`][stream-read].
+Is `true` if it is safe to call [`readable.read()`][stream-read], which means
+the stream has not been destroyed or emitted `'error'` or `'end'`.
 
 ##### `readable.readableEncoding`
 <!-- YAML
diff --git a/lib/_http_incoming.js b/lib/_http_incoming.js
index 79403455a6cca6..f309e32e36dc7b 100644
--- a/lib/_http_incoming.js
+++ b/lib/_http_incoming.js
@@ -63,8 +63,6 @@ function IncomingMessage(socket) {
   this.trailers = {};
   this.rawTrailers = [];
 
-  this.readable = true;
-
   this.aborted = false;
 
   this.upgrade = null;
diff --git a/lib/_stream_duplex.js b/lib/_stream_duplex.js
index 3f56b95b702e16..b832c973a1ee18 100644
--- a/lib/_stream_duplex.js
+++ b/lib/_stream_duplex.js
@@ -28,6 +28,7 @@
 
 const {
   ObjectDefineProperties,
+  ObjectGetOwnPropertyDescriptor,
   ObjectKeys,
   ObjectSetPrototypeOf,
 } = primordials;
@@ -71,6 +72,7 @@ function Duplex(options) {
 }
 
 ObjectDefineProperties(Duplex.prototype, {
+  writable: ObjectGetOwnPropertyDescriptor(Writable.prototype, 'writable'),
 
   destroyed: {
     get() {
diff --git a/lib/_stream_readable.js b/lib/_stream_readable.js
index 6fa3540057be33..4c64fd1b4fb346 100644
--- a/lib/_stream_readable.js
+++ b/lib/_stream_readable.js
@@ -23,6 +23,7 @@
 
 const {
   ArrayIsArray,
+  Boolean,
   NumberIsInteger,
   NumberIsNaN,
   ObjectDefineProperties,
@@ -181,9 +182,6 @@ function Readable(options) {
 
   this._readableState = new ReadableState(options, this, isDuplex);
 
-  // legacy
-  this.readable = true;
-
   if (options) {
     if (typeof options.read === 'function')
       this._read = options.read;
@@ -1057,6 +1055,20 @@ Readable.prototype[SymbolAsyncIterator] = function() {
 // because otherwise some prototype manipulation in
 // userland will fail
 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);
+    },
+    set(val) {
+      // Backwards compat.
+      if (this._readableState) {
+        this._readableState.readable = !!val;
+      }
+    }
+  },
 
   readableHighWaterMark: {
     enumerable: false,
@@ -1198,7 +1210,6 @@ function endReadableNT(state, stream) {
   // Check that we didn't get one last unshift.
   if (!state.errorEmitted && !state.endEmitted && state.length === 0) {
     state.endEmitted = true;
-    stream.readable = false;
     stream.emit('end');
 
     if (state.autoDestroy) {
diff --git a/lib/_stream_writable.js b/lib/_stream_writable.js
index 17ce4a9d3472a8..854da3ef058e94 100644
--- a/lib/_stream_writable.js
+++ b/lib/_stream_writable.js
@@ -27,6 +27,7 @@
 
 const {
   Array,
+  Boolean,
   FunctionPrototype,
   ObjectDefineProperty,
   ObjectSetPrototypeOf,
@@ -233,9 +234,6 @@ function Writable(options) {
 
   this._writableState = new WritableState(options, this, isDuplex);
 
-  // legacy.
-  this.writable = true;
-
   if (options) {
     if (typeof options.write === 'function')
       this._write = options.write;
@@ -776,6 +774,21 @@ function onFinished(stream, state, cb) {
   stream.prependListener('error', onerror);
 }
 
+ObjectDefineProperty(Writable.prototype, 'writable', {
+  get() {
+    const w = this._writableState;
+    if (!w) return false;
+    if (w.writable !== undefined) return w.writable;
+    return Boolean(!w.destroyed && !w.errored && !w.ending);
+  },
+  set(val) {
+    // Backwards compat.
+    if (this._writableState) {
+      this._writableState.writable = !!val;
+    }
+  }
+});
+
 ObjectDefineProperty(Writable.prototype, 'destroyed', {
   // Making it explicit this property is not enumerable
   // because otherwise some prototype manipulation in
diff --git a/test/parallel/test-http2-socket-proxy.js b/test/parallel/test-http2-socket-proxy.js
index 745006831759d1..17704e2fd9e3b7 100644
--- a/test/parallel/test-http2-socket-proxy.js
+++ b/test/parallel/test-http2-socket-proxy.js
@@ -85,10 +85,14 @@ server.on('stream', common.mustCall(function(stream, headers) {
 
   stream.respond();
 
-  socket.writable = 0;
-  socket.readable = 0;
-  assert.strictEqual(socket.writable, 0);
-  assert.strictEqual(socket.readable, 0);
+  socket.writable = true;
+  socket.readable = true;
+  assert.strictEqual(socket.writable, true);
+  assert.strictEqual(socket.readable, true);
+  socket.writable = false;
+  socket.readable = false;
+  assert.strictEqual(socket.writable, false);
+  assert.strictEqual(socket.readable, false);
 
   stream.end();
 
diff --git a/test/parallel/test-stream-readable-readable.js b/test/parallel/test-stream-readable-readable.js
new file mode 100644
index 00000000000000..6e1a7fb32e3080
--- /dev/null
+++ b/test/parallel/test-stream-readable-readable.js
@@ -0,0 +1,45 @@
+'use strict';
+const common = require('../common');
+const assert = require('assert');
+
+const { Readable } = require('stream');
+
+{
+  const r = new Readable({
+    read() {}
+  });
+  assert.strictEqual(r.readable, true);
+  r.destroy();
+  assert.strictEqual(r.readable, false);
+}
+
+{
+  const mustNotCall = common.mustNotCall();
+  const r = new Readable({
+    read() {}
+  });
+  assert.strictEqual(r.readable, true);
+  r.on('end', mustNotCall);
+  r.resume();
+  r.push(null);
+  assert.strictEqual(r.readable, true);
+  r.off('end', mustNotCall);
+  r.on('end', common.mustCall(() => {
+    assert.strictEqual(r.readable, false);
+  }));
+}
+
+{
+  const r = new Readable({
+    read: common.mustCall(() => {
+      process.nextTick(() => {
+        r.destroy(new Error());
+        assert.strictEqual(r.readable, false);
+      });
+    })
+  });
+  r.resume();
+  r.on('error', common.mustCall(() => {
+    assert.strictEqual(r.readable, false);
+  }));
+}
diff --git a/test/parallel/test-stream-writable-writable.js b/test/parallel/test-stream-writable-writable.js
new file mode 100644
index 00000000000000..98c1eab7719a22
--- /dev/null
+++ b/test/parallel/test-stream-writable-writable.js
@@ -0,0 +1,49 @@
+'use strict';
+const common = require('../common');
+const assert = require('assert');
+
+const { Writable } = require('stream');
+
+{
+  const w = new Writable({
+    write() {}
+  });
+  assert.strictEqual(w.writable, true);
+  w.destroy();
+  assert.strictEqual(w.writable, false);
+}
+
+{
+  const w = new Writable({
+    write: common.mustCall((chunk, encoding, callback) => {
+      callback(new Error());
+    })
+  });
+  assert.strictEqual(w.writable, true);
+  w.write('asd');
+  assert.strictEqual(w.writable, false);
+  w.on('error', common.mustCall());
+  w.destroy();
+}
+
+{
+  const w = new Writable({
+    write: common.mustCall((chunk, encoding, callback) => {
+      process.nextTick(() => {
+        callback(new Error());
+        assert.strictEqual(w.writable, false);
+      });
+    })
+  });
+  w.write('asd');
+  w.on('error', common.mustCall());
+}
+
+{
+  const w = new Writable({
+    write: common.mustNotCall()
+  });
+  assert.strictEqual(w.writable, true);
+  w.end();
+  assert.strictEqual(w.writable, false);
+}