Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

Commit 2456a54

Browse filesBrowse files
RantanenBethGriggs
authored andcommitted
lib: ensure readable stream flows to end
If a readable stream was set up with `highWaterMark 0`, the while-loop in `maybeReadMore_` function would never execute. The while loop now has an extra or-condition for the case where the stream is flowing and there are no items. The or-condition is adapted from the emit-condition of the `addChunk` function. The `addChunk` also contains a check for `state.sync`. However that part of the check was omitted here because the `maybeReadMore_` is executed using `process.nextTick`. `state.sync` is set and then unset within the `read()` function so it should never be in effect in `maybeReadMore_`. Fixes: #24915 PR-URL: #24918 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
1 parent 9bd4267 commit 2456a54
Copy full SHA for 2456a54

File tree

Expand file treeCollapse file tree

3 files changed

+157
-4
lines changed
Open diff view settings
Filter options
Expand file treeCollapse file tree

3 files changed

+157
-4
lines changed
Open diff view settings
Collapse file

‎lib/_stream_readable.js‎

Copy file name to clipboardExpand all lines: lib/_stream_readable.js
+26-4Lines changed: 26 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -567,16 +567,38 @@ function maybeReadMore(stream, state) {
567567
}
568568

569569
function maybeReadMore_(stream, state) {
570-
var len = state.length;
570+
// Attempt to read more data if we should.
571+
//
572+
// The conditions for reading more data are (one of):
573+
// - Not enough data buffered (state.length < state.highWaterMark). The loop
574+
// is responsible for filling the buffer with enough data if such data
575+
// is available. If highWaterMark is 0 and we are not in the flowing mode
576+
// we should _not_ attempt to buffer any extra data. We'll get more data
577+
// when the stream consumer calls read() instead.
578+
// - No data in the buffer, and the stream is in flowing mode. In this mode
579+
// the loop below is responsible for ensuring read() is called. Failing to
580+
// call read here would abort the flow and there's no other mechanism for
581+
// continuing the flow if the stream consumer has just subscribed to the
582+
// 'data' event.
583+
//
584+
// In addition to the above conditions to keep reading data, the following
585+
// conditions prevent the data from being read:
586+
// - The stream has ended (state.ended).
587+
// - There is already a pending 'read' operation (state.reading). This is a
588+
// case where the the stream has called the implementation defined _read()
589+
// method, but they are processing the call asynchronously and have _not_
590+
// called push() with new data. In this case we skip performing more
591+
// read()s. The execution ends in this method again after the _read() ends
592+
// up calling push() with more data.
571593
while (!state.reading && !state.ended &&
572-
state.length < state.highWaterMark) {
594+
(state.length < state.highWaterMark ||
595+
(state.flowing && state.length === 0))) {
596+
const len = state.length;
573597
debug('maybeReadMore read 0');
574598
stream.read(0);
575599
if (len === state.length)
576600
// didn't get any data, stop spinning.
577601
break;
578-
else
579-
len = state.length;
580602
}
581603
state.readingMore = false;
582604
}
Collapse file
+27Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,27 @@
1+
'use strict';
2+
3+
const common = require('../common');
4+
5+
// This test ensures that Readable stream will continue to call _read
6+
// for streams with highWaterMark === 0 once the stream returns data
7+
// by calling push() asynchronously.
8+
9+
const { Readable } = require('stream');
10+
11+
let count = 5;
12+
13+
const r = new Readable({
14+
// Called 6 times: First 5 return data, last one signals end of stream.
15+
read: common.mustCall(() => {
16+
process.nextTick(common.mustCall(() => {
17+
if (count--)
18+
r.push('a');
19+
else
20+
r.push(null);
21+
}));
22+
}, 6),
23+
highWaterMark: 0,
24+
});
25+
26+
r.on('end', common.mustCall());
27+
r.on('data', common.mustCall(5));
Collapse file
+104Lines changed: 104 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,104 @@
1+
'use strict';
2+
3+
const common = require('../common');
4+
5+
// Ensure that subscribing the 'data' event will not make the stream flow.
6+
// The 'data' event will require calling read() by hand.
7+
//
8+
// The test is written for the (somewhat rare) highWaterMark: 0 streams to
9+
// specifically catch any regressions that might occur with these streams.
10+
11+
const assert = require('assert');
12+
const { Readable } = require('stream');
13+
14+
const streamData = [ 'a', null ];
15+
16+
// Track the calls so we can assert their order later.
17+
const calls = [];
18+
const r = new Readable({
19+
read: common.mustCall(() => {
20+
calls.push('_read:' + streamData[0]);
21+
process.nextTick(() => {
22+
calls.push('push:' + streamData[0]);
23+
r.push(streamData.shift());
24+
});
25+
}, streamData.length),
26+
highWaterMark: 0,
27+
28+
// Object mode is used here just for testing convenience. It really
29+
// shouldn't affect the order of events. Just the data and its format.
30+
objectMode: true,
31+
});
32+
33+
assert.strictEqual(r.readableFlowing, null);
34+
r.on('readable', common.mustCall(() => {
35+
calls.push('readable');
36+
}, 2));
37+
assert.strictEqual(r.readableFlowing, false);
38+
r.on('data', common.mustCall((data) => {
39+
calls.push('data:' + data);
40+
}, 1));
41+
r.on('end', common.mustCall(() => {
42+
calls.push('end');
43+
}));
44+
assert.strictEqual(r.readableFlowing, false);
45+
46+
// The stream emits the events asynchronously but that's not guaranteed to
47+
// happen on the next tick (especially since the _read implementation above
48+
// uses process.nextTick).
49+
//
50+
// We use setImmediate here to give the stream enough time to emit all the
51+
// events it's about to emit.
52+
setImmediate(() => {
53+
54+
// Only the _read, push, readable calls have happened. No data must be
55+
// emitted yet.
56+
assert.deepStrictEqual(calls, ['_read:a', 'push:a', 'readable']);
57+
58+
// Calling 'r.read()' should trigger the data event.
59+
assert.strictEqual(r.read(), 'a');
60+
assert.deepStrictEqual(
61+
calls,
62+
['_read:a', 'push:a', 'readable', 'data:a']);
63+
64+
// The next 'read()' will return null because hwm: 0 does not buffer any
65+
// data and the _read implementation above does the push() asynchronously.
66+
//
67+
// Note: This 'null' signals "no data available". It isn't the end-of-stream
68+
// null value as the stream doesn't know yet that it is about to reach the
69+
// end.
70+
//
71+
// Using setImmediate again to give the stream enough time to emit all the
72+
// events it wants to emit.
73+
assert.strictEqual(r.read(), null);
74+
setImmediate(() => {
75+
76+
// There's a new 'readable' event after the data has been pushed.
77+
// The 'end' event will be emitted only after a 'read()'.
78+
//
79+
// This is somewhat special for the case where the '_read' implementation
80+
// calls 'push' asynchronously. If 'push' was synchronous, the 'end' event
81+
// would be emitted here _before_ we call read().
82+
assert.deepStrictEqual(
83+
calls,
84+
['_read:a', 'push:a', 'readable', 'data:a',
85+
'_read:null', 'push:null', 'readable']);
86+
87+
assert.strictEqual(r.read(), null);
88+
89+
// While it isn't really specified whether the 'end' event should happen
90+
// synchronously with read() or not, we'll assert the current behavior
91+
// ('end' event happening on the next tick after read()) so any changes
92+
// to it are noted and acknowledged in the future.
93+
assert.deepStrictEqual(
94+
calls,
95+
['_read:a', 'push:a', 'readable', 'data:a',
96+
'_read:null', 'push:null', 'readable']);
97+
process.nextTick(() => {
98+
assert.deepStrictEqual(
99+
calls,
100+
['_read:a', 'push:a', 'readable', 'data:a',
101+
'_read:null', 'push:null', 'readable', 'end']);
102+
});
103+
});
104+
});

0 commit comments

Comments
0 (0)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.