stream: adding new 'data' handler doesn't resume stream after removing 'readable' handler

[tvald]

• Version: 11.2.0
• Platform: Windows 10 x64
• Subsystem: Streams

Description
After a readable handler has been added and removed from a stream, the stream no longer begins / resumes flowing when a data handler is added to the stream. This is a breaking change from 10.x and appears inconsistent with the (convoluted) intended behavior of streams. (More on this below.)

Related issues: #22209, #24366, #24281

Example

const stream = fs.createReadStream('foo.txt'); // some file that exists

const onData = () => { console.log('DATA'); };
const onReadable = () => {
  console.log('READABLE');
  stream.off('readable', onReadable);
  stream.on('data', onData);
  //stream.resume(); // <-- this causes data to be emitted
};

stream.on('readable', onReadable);

Expected Output:
The readable handler is called, and then the data handler is called.

Actual behavior:
The readable handler is called, but not the data handler.

Further Discussion
It is desirable for a stream to resume flowing when no readable handler is attached and a data handler is added. For example, one might generate a stream and check that it successfully opens (via the readable handler) before returning the stream to be consumed elsewhere (via the data handler). This worked at least up through 10.x.

With a certain reading, the current gap in behavior could be held as consistent with the "Three States" / "under the hood" explanation of stream modes, although that in itself may contradict the "Two Reading Modes" abstraction.

From streams documentation:
Two Reading Modes

All Readable streams begin in paused mode but can be switched to flowing mode in one of the following ways: (1) Adding a data event handler...

Adding a readable event handler automatically makes the stream to stop flowing, and the data to be consumed via readable.read(). If the readable event handler is removed, then the stream will start flowing again if there is a data event handler.

Three States

Calling readable.pause(), readable.unpipe(), or receiving backpressure will cause the readable.readableFlowing to be set as false, temporarily halting the flowing of events but not halting the generation of data. While in this state, attaching a listener for the 'data' event will not switch readable.readableFlowing to true.

Without having looked at code, I interpret the "Three States" description to indicate that there is no way to return from the readableFlowing == false state to the readableFlowing == null state. Hence, adding a readable handler destroys the auto-start behavior of the data handler.

This is sufficiently counter-intuitive that a slew of issues have been filed in the past few months, such as #24366 and #24281. In fact, a patch was even accepted in #22209 that ensures the auto-start behavior of a data handler IFF it was added before the readable handler.

[Trott]

@nodejs/streams

[lpinca]

For the example in the description you can use stream.removeListener() instead of stream.off() and it will work as expected. However if the 'data' listener if added on next tick (or later) the issue persists. Here is a test case:

'use strict';

const { Readable } = require('stream');

const readable = new Readable({
  read() {}
});

function read() {}

readable.setEncoding('utf8');
readable.on('readable', read);
readable.removeListener('readable', read);

process.nextTick(function() {
  readable.on('data', function(chunk) {
    console.log(chunk);
  });

  readable.push('hello');
});

The expected behavior is to have 'hello' printed on the console but nothing is actually printed because flowing is set to false

node/lib/_stream_readable.js

Line 830 in 7032e59

state.flowing = false;

when the 'readable' listener is added, and updateReadableListening()

node/lib/_stream_readable.js

Lines 877 to 890 in 7032e59

	function updateReadableListening(self) {
	const state = self._readableState;
	state.readableListening = self.listenerCount('readable') > 0;
	if (state.resumeScheduled && !state.paused) {
	// Flowing needs to be set to true now, otherwise
	// the upcoming resume will not flow.
	state.flowing = true;
	// Crude way to check if we should resume
	} else if (self.listenerCount('data') > 0) {
	self.resume();
	}
	}

does not find any registered 'data' listener.

I'm not sure if this is actually intended behavior but it is the reason why https://github.com/TooTallNate/node-https-proxy-agent no longer work in Node.js >= 10.0.0 and that lib has ~6 millions weekly downloads (I think we should add it to CITGM).

cc: @nodejs/streams @mcollina

[ronag]

For the example in the description you can use stream.removeListener() instead of stream.off() and it will work as expected.

I believe this part of the problem was resolved in 1665a93