From 31e3e6f1f8a2d63a606e154440e46c664eba9053 Mon Sep 17 00:00:00 2001
From: Denys Otrishko <shishugi@gmail.com>
Date: Fri, 6 Jul 2018 02:19:15 +0300
Subject: [PATCH] stream: fix readable behavior for highWaterMark === 0

Avoid trying to emit 'readable' due to the fact that
state.length is always >= state.highWaterMark if highWaterMark is 0.
Therefore upon .read(0) call (through .on('readable')) stream assumed
that it has enough data to emit 'readable' even though
state.length === 0 instead of issuing _read(). Which led to the TTY
not recognizing that someone is waiting for the input.

Fixes: https://github.com/nodejs/node/issues/20503
Refs: https://github.com/nodejs/node/pull/18372

PR-URL: https://github.com/nodejs/node/pull/21690
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
---
 lib/_stream_readable.js                       |  6 +++-
 test/parallel/test-readable-single-end.js     | 16 ++++++++++
 test/parallel/test-stream-readable-hwm-0.js   | 30 +++++++++++++++++++
 .../pseudo-tty/test-readable-tty-keepalive.js | 29 ++++++++++++++++++
 .../test-readable-tty-keepalive.out           |  0
 5 files changed, 80 insertions(+), 1 deletion(-)
 create mode 100644 test/parallel/test-readable-single-end.js
 create mode 100644 test/parallel/test-stream-readable-hwm-0.js
 create mode 100644 test/pseudo-tty/test-readable-tty-keepalive.js
 create mode 100644 test/pseudo-tty/test-readable-tty-keepalive.out

diff --git a/lib/_stream_readable.js b/lib/_stream_readable.js
index 0b5cac6b8376e3..029faaa6142d96 100644
--- a/lib/_stream_readable.js
+++ b/lib/_stream_readable.js
@@ -383,7 +383,10 @@ Readable.prototype.read = function(n) {
   // the 'readable' event and move on.
   if (n === 0 &&
       state.needReadable &&
-      (state.length >= state.highWaterMark || state.ended)) {
+      ((state.highWaterMark !== 0 ?
+        state.length >= state.highWaterMark :
+        state.length > 0) ||
+       state.ended)) {
     debug('read: emitReadable', state.length, state.ended);
     if (state.length === 0 && state.ended)
       endReadable(this);
@@ -808,6 +811,7 @@ Readable.prototype.on = function(ev, fn) {
     if (!state.endEmitted && !state.readableListening) {
       state.readableListening = state.needReadable = true;
       state.emittedReadable = false;
+      debug('on readable', state.length, state.reading);
       if (state.length) {
         emitReadable(this);
       } else if (!state.reading) {
diff --git a/test/parallel/test-readable-single-end.js b/test/parallel/test-readable-single-end.js
new file mode 100644
index 00000000000000..0969d49aa48e98
--- /dev/null
+++ b/test/parallel/test-readable-single-end.js
@@ -0,0 +1,16 @@
+'use strict';
+
+const common = require('../common');
+const { Readable } = require('stream');
+
+// This test ensures that there will not be an additional empty 'readable'
+// event when stream has ended (only 1 event signalling about end)
+
+const r = new Readable({
+  read: () => {},
+});
+
+r.push(null);
+
+r.on('readable', common.mustCall());
+r.on('end', common.mustCall());
diff --git a/test/parallel/test-stream-readable-hwm-0.js b/test/parallel/test-stream-readable-hwm-0.js
new file mode 100644
index 00000000000000..ecbc197d48208f
--- /dev/null
+++ b/test/parallel/test-stream-readable-hwm-0.js
@@ -0,0 +1,30 @@
+'use strict';
+
+const common = require('../common');
+
+// This test ensures that Readable stream will call _read() for streams
+// with highWaterMark === 0 upon .read(0) instead of just trying to
+// emit 'readable' event.
+
+const assert = require('assert');
+const { Readable } = require('stream');
+
+const r = new Readable({
+  // must be called only once upon setting 'readable' listener
+  read: common.mustCall(),
+  highWaterMark: 0,
+});
+
+let pushedNull = false;
+// this will trigger read(0) but must only be called after push(null)
+// because the we haven't pushed any data
+r.on('readable', common.mustCall(() => {
+  assert.strictEqual(r.read(), null);
+  assert.strictEqual(pushedNull, true);
+}));
+r.on('end', common.mustCall());
+process.nextTick(() => {
+  assert.strictEqual(r.read(), null);
+  pushedNull = true;
+  r.push(null);
+});
diff --git a/test/pseudo-tty/test-readable-tty-keepalive.js b/test/pseudo-tty/test-readable-tty-keepalive.js
new file mode 100644
index 00000000000000..4cf0b9fb2963f9
--- /dev/null
+++ b/test/pseudo-tty/test-readable-tty-keepalive.js
@@ -0,0 +1,29 @@
+'use strict';
+
+const common = require('../common');
+const assert = require('assert');
+
+// This test ensures that Node.js will not ignore tty 'readable' subscribers
+// when it's the only tty subscriber and the only thing keeping event loop alive
+// https://github.com/nodejs/node/issues/20503
+
+process.stdin.setEncoding('utf8');
+
+const expectedInput = ['foo', 'bar', null];
+
+process.stdin.on('readable', common.mustCall(function() {
+  const data = process.stdin.read();
+  assert.strictEqual(data, expectedInput.shift());
+}, 3)); // first 2 data, then end
+
+process.stdin.on('end', common.mustCall());
+
+setTimeout(() => {
+  process.stdin.push('foo');
+  process.nextTick(() => {
+    process.stdin.push('bar');
+    process.nextTick(() => {
+      process.stdin.push(null);
+    });
+  });
+}, 1);
diff --git a/test/pseudo-tty/test-readable-tty-keepalive.out b/test/pseudo-tty/test-readable-tty-keepalive.out
new file mode 100644
index 00000000000000..e69de29bb2d1d6