src: nix stdin _readableState.reading manipulation

[chrisdickinson]

this opts for stream.push('') which has the same effect but uses a public API.

(see #445)

R=@indutny

[sonewman]

LGTM

[bnoordhuis]

@chrisdickinson Maybe R= someone? (Not me, I've taken a solemn vow to never touch anything involving streams.)

[sonewman]

It does set state.reading = false lib/_stream_readable.js#L139

The only thing I was wondering is that it looks like before it sets that property it would create a 0 length buffer (#L98-L104) and because it's paused in both cases it would add it to the _readableState.buffer array.

But this would then get handled in the relevant way by .read(0). It is quite a long journey through the code though.

But once the interactions are changed from manually manipulating state to triggering API, the underlying implementation detail could be changed in the readable-stream to make this action do less work.

I guess it depends if we want calling .push('') to be the trigger for a readable to be in a state of not reading. It is pretty complex to say the least.

[chrisdickinson]

I guess it depends if we want calling .push('') to be the trigger for a readable to be in a state of not reading. It is pretty complex to say the least.

It's easier to think of reading as the state of the readable during the space in time between .read(<anything>) and the first .push(<anything>): it is a flag to say "don't try to trigger _read, we've already done so and haven't seen any results from it yet!" .push('') then becomes a natural outcome of how the system works.

But once the interactions are changed from manually manipulating state to triggering API, the underlying implementation detail could be changed in the readable-stream to make this action do less work.

Yep! In particular we could look into the work necessary to not push zero-length buffers onto the queue. 👋

[sonewman]

@chrisdickinson Yeah, that does make sense for streams of text and buffers, would we do the same in objectMode?

#L139 seems to signify that we have just pushed some data and that data is being processed to be emitted if in flowing mode.

In every event, a stream could even be in a state where it is actually awaiting data, but is not paused or actively reading as well potentially.

What I stated before:

It does set state.reading = false lib/_stream_readable.js#L139

was actually wrong anyway #L160 is where reading = false would happen because:

state.objectMode || chunk && chunk.length > 0

would be false (#127) because of the zero length. (That will teach me to be reading code at 2am!:weary:)

@bnoordhuis IMO I think this PR is safe to merge (assuming all tests pass - I have not checked out the branch) although it would be good to get a extra pair of eyes on it (the more the better I always say) even if just for educational purposes.

I can see why you might have vowed to avoid touching streams 😄 I guess this is why we need to make them simpler, more friendly and approachable.

[vkurchatkin]

-1. While doing stdin._readableState.reading = false is clearly wrong at least it's obvious what happens (not much) and what was author's intention. stdin.push(''); is much worse:

• unclear what it does (can be solve with a comment);
• still relies on internals in non-obvious way; will be hard to catch this if push changes;
• still a hack - push is not supposed to be called from outside.

[sonewman]

@vkurchatkin I agree that it is not very intuitive, however the sooner we can remove implementation detail from code which is using it, we can develop the implementation and improve the API which is utilised.

Also .push() is part of the API, there is no rule that this should be called from inside of _read().

[vkurchatkin]

Quote from docs:

This function should be called by Readable implementors, NOT by consumers of Readable streams

[chrisdickinson]

That file is implementing the stream, not consuming it.

It's a lot easier to reason about streams when the only way of affecting their private state is through their public API – which .push is a part of (the litmus test being "is code outside of _stream_readable.js expected to call this?")

On Jan 19, 2015, at 8:29 AM, Vladimir Kurchatkin notifications@github.com wrote:

Quote from docs:

This function should be called by Readable implementors, NOT by consumers of Readable streams

—
Reply to this email directly or view it on GitHub.

[indutny]

Just curious. Will this create an empty buffer every time it does this?

[chrisdickinson]

Yes.

[indutny]

One comment, otherwise LGTM if tests are passing.

[sonewman]

@indutny I believe it will if the stdin in question is not in objectMode or the encoding is not the same as the state.encoding:

Readable.prototype.push = function(chunk, encoding) {
  var state = this._readableState;

  if (!state.objectMode && typeof chunk === 'string') {
    encoding = encoding || state.defaultEncoding;
    if (encoding !== state.encoding) {
      chunk = new Buffer(chunk, encoding);
      encoding = '';
    }
  }

  return readableAddChunk(this, state, chunk, encoding, false);
};

v1.x/lib/_stream_readable.js#L95-L107

However once it enters into readableAddChunk it will do nothing except return needMoreData(state); since the chunk.length is not > 0.

[sonewman]

There is an interesting part in the documentation I stumbled across, which states:

If you find that you have to use stream.push(''), please consider another approach, because it almost certainly indicates that something is horribly wrong.

https://github.com/iojs/io.js/blob/v1.x/doc/api/stream.markdown#streampush 😈

[chrisdickinson]

@sonewman Hah, yeah. I saw that too – though I imagine it's still better to use a documented (if harshly worded) part of the public API than to reach in and manipulate private state.

[sonewman]

@chrisdickinson I think this could be considered a valid use case, since the side effects are what we are trying to attain.

It puts the stream in a waiting mode, which is not reading but it's also not paused or ended.

It would be nice if we didn't have to make the zero length Buffer, perhaps we could change the conditional, I could be wrong, but I can't see any adverse side effects from doing:

if (!state.objectMode && chunk && typeof chunk === 'string')

v1.x/lib/_stream_readable.js#L98

[chrisdickinson]

Merged (finally!) in 8cee8f5. Thanks @Qard, @brendanashworth, for pinging me about this. Mea culpa on letting it sit so long :|

[silverwind]

Now that #5384 has landed, we might be able to undo the revert (#3490) of this.