stream: Properly handle large reads from push-streams

Problem 1: If stream.push() triggers a 'readable' event, and the user
calls `read(n)` with some n > the highWaterMark, then the push() will
return false (indicating that they should not push any more), but no
future 'readable' event is coming (because we're above the
highWaterMark).

Solution: return true from push() when needReadable is set.

Problem 2: A read(n) for n != 0, after the stream had encountered an
EOF, would not trigger the 'end' event if the EOF was pushed in
synchronously by the _read() function.

Solution: Check for ended in stream.read() and schedule an end event if
the length now equals 0.

Fix #4585

lib/_stream_readable.js

@@ -108,8 +108,13 @@ Readable.prototype.push = function(chunk) {
   // if it's past the high water mark, we can push in some more.
   // Also, if it's still within the lowWaterMark, we can stand some
   // more bytes.  This is to work around cases where hwm=0 and
-  // lwm=0, such as the repl.
-  return rs.length < rs.highWaterMark || rs.length <= rs.lowWaterMark;
+  // lwm=0, such as the repl.  Also, if the push() triggered a
+  // readable event, and the user called read(largeNumber) such that
+  // needReadable was set, then we ought to push more, so that another
+  // 'readable' event will be triggered.
+  return rs.needReadable ||
+         rs.length < rs.highWaterMark ||
+         rs.length <= rs.lowWaterMark;
 };
 
 // backwards compatibility.
@@ -228,6 +233,12 @@ Readable.prototype.read = function(n) {
   if (state.length === 0 && !state.ended)
     state.needReadable = true;
 
+  // If we happened to read() exactly the remaining amount in the
+  // buffer, and the EOF has been seen at this point, then make sure
+  // that we emit 'end' on the very next tick.
+  if (state.ended && !state.endEmitted && state.length === 0)
+    endReadable(this);
+
   return ret;
 };
 

test/simple/test-stream2-large-read-stall.js

@@ -0,0 +1,82 @@
+// Copyright Joyent, Inc. and other Node contributors.
+//
+// Permission is hereby granted, free of charge, to any person obtaining a
+// copy of this software and associated documentation files (the
+// "Software"), to deal in the Software without restriction, including
+// without limitation the rights to use, copy, modify, merge, publish,
+// distribute, sublicense, and/or sell copies of the Software, and to permit
+// persons to whom the Software is furnished to do so, subject to the
+// following conditions:
+//
+// The above copyright notice and this permission notice shall be included
+// in all copies or substantial portions of the Software.
+//
+// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS
+// OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
+// MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN
+// NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM,
+// DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR
+// OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE
+// USE OR OTHER DEALINGS IN THE SOFTWARE.
+
+var common = require('../common.js');
+var assert = require('assert');
+
+// If everything aligns so that you do a read(n) of exactly the
+// remaining buffer, then make sure that 'end' still emits.
+
+var READSIZE = 100;
+var PUSHSIZE = 20;
+var PUSHCOUNT = 1000;
+var HWM = 50;
+
+var Readable = require('stream').Readable;
+var r = new Readable({
+  highWaterMark: HWM
+});
+var rs = r._readableState;
+
+r._read = push;
+
+r.on('readable', function() {
+  console.error('>> readable');
+  do {
+    console.error('  > read(%d)', READSIZE);
+    var ret = r.read(READSIZE);
+    console.error('  < %j (%d remain)', ret && ret.length, rs.length);
+  } while (ret && ret.length === READSIZE);
+
+  console.error('<< after read()',
+                ret && ret.length,
+                rs.needReadable,
+                rs.length);
+});
+
+var endEmitted = false;
+r.on('end', function() {
+  endEmitted = true;
+  console.error('end');
+});
+
+var pushes = 0;
+function push() {
+  if (pushes > PUSHCOUNT)
+    return;
+
+  if (pushes++ === PUSHCOUNT) {
+    console.error('   push(EOF)');
+    return r.push(null);
+  }
+
+  console.error('   push #%d', pushes);
+  if (r.push(new Buffer(PUSHSIZE)))
+    setTimeout(push);
+}
+
+// start the flow
+var ret = r.read(0);
+
+process.on('exit', function() {
+  assert.equal(pushes, PUSHCOUNT + 1);
+  assert(endEmitted);
+});