Node: Cleanup `_writableState` and `_readableState` access across codebase

[vkurchatkin]

This is a meta-issue to keep to track of all usages of _writableState and _readableState outside of streams source. These properties are considered private and should not be used unless absolutely necessary. Usage of them can indicate a few things:

• the code can be rewritten using existing documented API to achieve the same result;
• streams lack some consumer functionality and new public API should be introduced;
• streams lack some implementor functionality and new protected API should be introduced;
• documentation needs to be added for some parts of private state for implementors;
• it is an optimization that is and always be possible only in core.

The list of all _writableState and _readableState usages:

src/node.js

• [ ] [L564](https://github.com/iojs/io.js/blob/8440cacb100ae83c2b2c02e82a87c73a66380c21/src/node.js#L564) stdin._readableState.reading = false. Added in commit: bb56dcc4505f1b8fe7fe by @isaacs; (#454)

• [ ] [L573](https://github.com/iojs/io.js/blob/8440cacb100ae83c2b2c02e82a87c73a66380c21/src/node.js#L573) stdin._readableState.reading = false. Added in commit: bb56dcc4505f1b8fe7fe by @isaacs; (#454)

  lib/_debug_agent.js

• [x] L87 this._readableState.objectMode = true (#270);

  lib/_http_server.js

• [ ] [L348](https://github.com/iojs/io.js/blob/8440cacb100ae83c2b2c02e82a87c73a66380c21/lib/_http_server.js#L348) socket._readableState.flowing = null (TODO(isaacs): Need a way to reset a stream to fresh state IE, not flowing, and not explicitly paused.). Added in 967b5dbb453f8110 by @isaacs;

• [ ] [L408](https://github.com/iojs/io.js/blob/8440cacb100ae83c2b2c02e82a87c73a66380c21/lib/_http_server.js#L408) var needPause = socket._writableState.needDrain. Added in 085dd30e93da6736 by @isaacs ;

• [ ] [L445](https://github.com/iojs/io.js/blob/8440cacb100ae83c2b2c02e82a87c73a66380c21/lib/_http_server.js#L445) req._readableState.resumeScheduled. Not sure where this originated, but in 2efe4ab7616669448f873b @indutny added oldMode check here;

  lib/_tls_legacy.js

• [ ] L421 this._writableState.finished;

• [ ] L508 self._readableState.length > 0;

  lib/_tls_wrap.js

• [ ] L311 self._writableState.errorEmitted (#465);

• [ ] L313 self._writableState.errorEmitted = true (#465);

• [ ] L350 socket._readableState.length;

  lib/child_process.js

• [x] L1061 stream._readableState.flowing (#511);

  lib/crypto.js

• [ ] whole LazyTransform thing. Is it really necessary? Maybe it should go to stream? Maybe it should be public? Maybe transforms should be lazy by default?;

• [x] L56 this._writableState.decodeStrings = false;
• [x] L57 this._writableState.defaultEncoding = 'binary';

• [x] L90 var encoding = this._readableState.encoding || 'buffer' (#610);

  lib/fs.js

• [ ] L1624 allocNewPool(this._readableState.highWaterMark);

  lib/net.js

• [ ] L162 this._writableState.decodeStrings = false (#465);

• [ ] L174 this._readableState.flowing = false (#465);
• [ ] L196 this._readableState.ended (#465);
• [ ] L226 self._readableState.ended;
• [ ] L242 this._readableState.ended = true (comment: ended should already be true, since this is called _after_ the EOF errno and onread has eof'ed) (#465);
• [ ] L243 this._readableState.endEmitted;
• [ ] L362 this._writableState.length;
• [ ] L392 this._readableState.endEmitted;
• [ ] L405 socket._writableState.length;
• [ ] L415 if (this._writableState.finished);
• [ ] L429 self._writableState.errorEmitted (#465);
• [ ] L433 self._writableState.errorEmitted = true (#465);
• [ ] L535 self._readableState.length === 0;
• [ ] L715 state.getBuffer();
• [ ] L842 this._readableState.reading = false;
• [ ] L843 this._readableState.ended = false;
• [ ] L844 this._readableState.endEmitted = false;
• [ ] L845 this._writableState.ended = false;
• [ ] L846 this._writableState.ending = false;
• [ ] L847 this._writableState.finished = false;

• [ ] L848 this._writableState.errorEmitted = false (#465);

  lib/zlib.js

• [ ] [L423](https://github.com/iojs/io.js/blob/8440cacb100ae83c2b2c02e82a87c73a66380c21/lib/zlib.js#L423) ws.ended;

• [ ] [L426](https://github.com/iojs/io.js/blob/8440cacb100ae83c2b2c02e82a87c73a66380c21/lib/zlib.js#L426) ws.ending;
• [ ] [L429](https://github.com/iojs/io.js/blob/8440cacb100ae83c2b2c02e82a87c73a66380c21/lib/zlib.js#L429) ws.needDrain;
• [ ] [L460](https://github.com/iojs/io.js/blob/8440cacb100ae83c2b2c02e82a87c73a66380c21/lib/zlib.js#L460) ws.ending || ws.ended;
• [ ] [L461](https://github.com/iojs/io.js/blob/8440cacb100ae83c2b2c02e82a87c73a66380c21/lib/zlib.js#L461) ws.length;
• [ ] [L479](https://github.com/iojs/io.js/blob/8440cacb100ae83c2b2c02e82a87c73a66380c21/lib/zlib.js#L479) ws.length.

List of used properties:

_readableState

• reading;
• objectMode;
• flowing (boolean?) is used to determine which mode readable stream is in; can be true, false or null; `null is the initial state which means that is implicitly paused;
• resumeScheduled;
• length;
• encoding;
• highWaterMark;
• ended;

• endEmitted;

  _writableState

• needDrain;

• ended;
• ending;
• finished;
• errorEmitted;
• decodeStrings;
• defaultEncoding;
• length;
• getBuffer();

/cc @chrisdickinson

[chrisdickinson]

This is great! Thanks for compiling a comprehensive list. The most egregious overreaches, I think, are any of the places we reach in and directly modify state. Informational access is less bad.

I'd like to encourage folks to pick these off one at a time. For changes that might require a new internal API, it'd be great to briefly discuss plans here first before cutting a PR.

On Jan 15, 2015, at 8:18 AM, Vladimir Kurchatkin [email protected] wrote:

This is a meta-issue to keep to track of all usages of _writableState and _readableState outside of streams source. These properties are considered private and should not be used unless absolutely necessary. Usage of them can indicate a few things:

the code can be rewritten using existing documented API to achieve the same result;
streams lack some consumer functionality and new public API should be introduced;
streams lack some implementor functionality and new protected API should be introduced;
documentation needs to be added for some parts of private state for implementors;
it is an optimization that is and always be possible only in core.
The list of all _writableState and _readableState usages:

src/node.js

L564 stdin._readableState.reading = false;
L573 stdin._readableState.reading = false;
lib/_debug_agent.js

L87 this._readableState.objectMode = true (#270);
lib/_http_server.js

L348 socket._readableState.flowing = null (TODO(isaacs): Need a way to reset a stream to fresh state IE, not flowing, and not explicitly paused.);
L408 var needPause = socket._writableState.needDrain;
L445 req._readableState.resumeScheduled;
lib/_tls_legacy.js

L421 his._writableState.finished;
L508 self._readableState.length > 0;
lib/_tls_wrap.js

L311 self._writableState.errorEmitted;
L313 self._writableState.errorEmitted = true;
L350 socket._readableState.length;
lib/child_process.js

L1061 stream._readableState.flowing;
lib/crypto.js

whole LazyTransform thing. Is it really necessary? Maybe it should go to stream? Maybe it should be public? Maybe transforms should be lazy by default?;
L56 this._writableState.decodeStrings = false;
L57 this._writableState.defaultEncoding = 'binary';
L90 var encoding = this._readableState.encoding || 'buffer';
lib/fs.js

L1624 allocNewPool(this._readableState.highWaterMark);
lib/net.js

L162 this._writableState.decodeStrings = false;
L174 this._readableState.flowing = false;
L196 this._readableState.ended;
L226 self._readableState.ended;
L242 this._readableState.ended = true (comment: ended should already be true, since this is called after the EOF errno and onread has eof'ed);
L243 this._readableState.endEmitted;
L362 this._writableState.length;
L392 this._readableState.endEmitted;
L405 socket._writableState.length;
L415 if (this._writableState.finished);
L429 self._writableState.errorEmitted;
L433 self._writableState.errorEmitted = true;
L535 self._readableState.length === 0;
L715 state.getBuffer();
L842 this._readableState.reading = false;
L843 this._readableState.ended = false;
L844 this._readableState.endEmitted = false;
L845 this._writableState.ended = false;
L846 this._writableState.ending = false;
L847 this._writableState.finished = false;
L848 this._writableState.errorEmitted = false;
/cc @chrisdickinson

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

[chrisdickinson]

Of note: I'm not opposed to _all_ non-streams access to _readableState, just accesses that are directly inspecting a property _or_ manipulating state. APIs private to io.js itself are okay to hang off of ReadableState, but I'd like to make sure that there's a clear API there, and not just random attribute access/manipulation.

[vkurchatkin]

We should also probably inspect userland modules to find out what people are doing with internals.

[chrisdickinson]

As a heads up, I edited your issue comment to link #454 for the node.js _readableState use (sorry sorry). In the future, I can ping you instead if you'd like to keep the list maintained!

[vkurchatkin]

@chrisdickinson fine by me. But think it's a good idea to reference this issue so we can see the timeline of all relevant stuff here

[chrisdickinson]

We might add lib/zlib.js:

• L423: ws.ended
• L426: ws.ending
• L429: ws.needDrain
• L460: ws.ending || ws.ended
• L461: ws.length
• L479: ws.length

[chrisdickinson]

Cross posting this from iojs/readable-stream#109:

To amend the intent of this issue a bit – the goal is not to absolutely remove all use of _{{writ,read}able,transform}State, but to remove all places where:

• subclasses directly manipulate private state, or...
• state is being preserved in the base classes for the exclusive use of a subclass.

Ultimately, the places where the subclasses access those state objects should be catalogued and we should look into the best way to deal with that access – whether that's promotion into a public or protected "reflection" API for subclasses.

[Trott]

This issue hasn't seen any activity in almost a year. Is it still active and useful? Or not so much?

[indutny]

I think it is very informative, and perhaps needs some work to be done.

[Fishrock123]

Marking as stalled, is this still viable after the problems we encountered last time?

[Fishrock123]

All the problems here are significantly difficult, but it would be great if we could get some people interested in this. Marked with labels.

[Zahidul-Islam]

@Fishrock123 @Trott Is this issue still need help?

[Trott]

@Fishrock123 @Trott Is this issue still need help?

I think the answer is "yes", but I'll let @nodejs/streams folks speak to that. They might also want to consider if they want to keep the "good first contribution" on this or not. Also might also want to consider adding "mentor available" label if any of them are willing to help others work through it.

[addaleax]

@Zahidul-Islam The thing is, for some of these items (and the list is not up to date, by the way), the answer is “this is okay and doesn’t need fixing”. It might just require talking about whether that is the case or not.

Also might also want to consider adding "mentor available" label if any of them are willing to help others work through it.

:+1: I am.

[Zahidul-Islam]

@addaleax I am more than happy to help. I just need some guideline (Where should I start?)

[addaleax]

@Zahidul-Islam Basically, the idea here would be that we take a look at the usage of _readableState and _writableState in lib/, and see if they are okay or they can be replaced with public streams APIs (because if it’s not possible to do that, that might indicate a deficiency in the streams API, and if it is possible, maybe we should do it for readability?). It’s admittedly all a bit vague, but I guess there’s no way around it…

Where should I start?

greping through lib for those two identifiers sounds like a good idea :)

[Zahidul-Islam]

@addaleax thank you. I will start with _readableState and _writableState in lib/ folder.

[mcollina]

We discussed this in the collaborator summit in Berlin 2017, and we decided to remove the 'good-first-contribution', and we think the first task is to update the list. As an example, crypto.js is now good to go.

We plan to add non-prefixed properties for the things that are needed, with sensible names. We should probably set up individual issues for every task, so it can be crunched in a distributed fashion.

[calvinmetcalf]

ok some meta issues that are coming up as I'm going through fixing these

1. should we use getters or should we use methods? this will likely have to do with the performance impact.
2. should these be documented? I'm not 100% sure we want all of these to be publicly known about, some of them are still fairly internal even if certain parts of node wants to use them. Thoughts?

cc @mscdex @Trott @jasnell

[mscdex]

I'm also concerned about adding new (especially non-underscored) properties directly to the prototype/instances of streams. There could be property name collision issues in userland. This is why I actually prefer having minimal reserved properties like we have currently.

[calvinmetcalf]

@mscdex so the main issue is just that there is information you can only get out of a stream by looking at it's readable/writable state, so if we want to remove all usage of readable/writable state then we need to add more properties or methods, they certainly can be underscored.

[mscdex]

@calvinmetcalf I understand, but I mean you could just set up a single non-underscored property for each type of state and then either have mixed underscored and non-underscored in those state objects or use Symbols to better hide whichever "private" properties and only expose non-underscored properties.

[calvinmetcalf]

we can definitely use symbols, but we would totally need to benchmark that,
it could effect that.

On Mon, May 8, 2017 at 12:48 PM Brian White [e-mail]
wrote:

@calvinmetcalf https://github.com/calvinmetcalf I understand, but I
mean you could just set up a single non-underscored property for each type
of state and then either have mixed underscored and non-underscored in
those state objects or use Symbols to better hide whichever "private"
properties and only expose non-underscored properties.

—
You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub
https://github.com/nodejs/node/issues/445#issuecomment-299922970, or mute
the thread
https://github.com/notifications/unsubscribe-auth/ABE4nwUX5k1ClrQ4_0A5Qm8rFlcwT1akks5r30dpgaJpZM4DS5L6
.

[calvinmetcalf]

though symbols would be hard to use with readable stream, as it is flowing, readBuffer and writeBuffer are mentioned in the docs so those we should probably have be documented non-underscored properties for them, {read/write}length on the other hand are pretty obscure and if we were to document them we'd probably want to put it in some sort of advanced streams mechanics section, highwater mark (along with object mode) are part of user configured options so it would make sense to maybe have an options property, except the readable and writable halves could have different options, so we might need to have different objects for each, but that might really have some performance issues.

[mscdex]

I'm still -1 on adding a bunch of new properties directly to streams. I would still rather see having a non-prefixed property (e.g. .readableState) and add to that instead as I am very concerned about naming clashes with userland. I know I have run into 3rd-party stream modules before that have methods like readBuffer(), writeBuffer(), readLength(), writeLength(), etc. and this would break those unnecessarily.

[mcollina]

I'm still -1 on adding a bunch of new properties directly to streams. I would still rather see having a non-prefixed property (e.g. .readableState) and add to that instead as I am very concerned about naming clashes with userland. I know I have run into 3rd-party stream modules before that have methods like readBuffer(), writeBuffer(), readLength(), writeLength(), etc. and this would break those unnecessarily.

@mscdex One of the current weak spots performance-wise of streams is the creation of _readableState and _writableState. Those are slow. I do not think it is feasible to allocate another full object for this. Also, we do not have a benchmark for stream creation.

[mcollina]

I think we are not in agreement on how to tackle this issue, so I'm restarting the conversation here.
Also, tagging @nodejs/ctc.

In a lot of places, core uses streams internal state, as the public API does not provide full discoverability on what a stream is doing.

In order to solve this, there are three routes:

a. leave _writableState and _readableState as is, and just document them. A variation is to remap them without the _, but keep them in the same object. This would cause to expose a lot of other properties we do not need.

b. expose the part of the state as properties/getters on the stream object themselves, with the disadvantage that the streams would get _some_ more properties.

c. allocate a new object for writableState and readableState and use getters there to link them to the _writableState and _readableState. This would force the creation of 2 more objects for each Duplex, and the allocation of a stream is already _slow_.

Note that we cannot remove or change the same properties of _writableState and _readableState  because they are currently used by the ecosystem.

[mcollina]

I would like to talk about this in the next CTC cc @nodejs/ctc. We need direction, as stated above ^.

[mcollina]

@nodejs/ctc The three approaches to solve this are described in https://github.com/nodejs/node/issues/445#issuecomment-310028610.

I would recommend to review https://github.com/nodejs/node/pull/12855 and https://github.com/nodejs/node/pull/12857.

[Fishrock123]

Option a seems unfortunate but more favorable. We should probably not do https://github.com/nodejs/node/pull/12857 then.

[Trott]

Should this remain open? If so, is there a suggested path to closure? @nodejs/streams

[mcollina]

@Trott yes. I need to update the tracking list, but I've been silently refactoring parts of core to remove these. The current list is not that big. It's a slow process :/.

[ryzokuken]

@mcollina could you post an updated list of those so people could join forces? Would love to take some of this off your plate.

[mcollina]

@ryzokuken this is something you can help with, basically you'd have to run grep -R _readableState * and grep -R _writableState * inside the lib folder. Ignore all files with _stream* prefix, and everything in lib/internal/streams/ folder.

[ryzokuken]

@mcollina I believe I'm only supposed to take care of the stuff in lib, right?

[mcollina]

Yes!

[ryzokuken]

@mcollina Hmm, and looking at the uses, I believe I might have to add new functions to certain classes in order to eliminate the use of _readableState, for instance. Does that sound fine to you?

[mcollina]

If you could just post an updated list it would be a great start. I know for experience that some of those are
hard.

[ryzokuken]

List of offences

_readableState

_http_server.js

• https://github.com/nodejs/node/blob/b003c86bf45d0689d7e77fce0489a6de888b0782/lib/_http_server.js#L563

internal/child_process.js

• https://github.com/nodejs/node/blob/b003c86bf45d0689d7e77fce0489a6de888b0782/lib/internal/child_process.js#L246

internal/http2/compat.js

• https://github.com/nodejs/node/blob/b003c86bf45d0689d7e77fce0489a6de888b0782/lib/internal/http2/compat.js#L252
• https://github.com/nodejs/node/blob/b003c86bf45d0689d7e77fce0489a6de888b0782/lib/internal/http2/compat.js#L355

internal/http2/core.js

• https://github.com/nodejs/node/blob/b003c86bf45d0689d7e77fce0489a6de888b0782/lib/internal/http2/core.js#L359
• https://github.com/nodejs/node/blob/b003c86bf45d0689d7e77fce0489a6de888b0782/lib/internal/http2/core.js#L397
• https://github.com/nodejs/node/blob/b003c86bf45d0689d7e77fce0489a6de888b0782/lib/internal/http2/core.js#L1548
• https://github.com/nodejs/node/blob/b003c86bf45d0689d7e77fce0489a6de888b0782/lib/internal/http2/core.js#L1826
• https://github.com/nodejs/node/blob/b003c86bf45d0689d7e77fce0489a6de888b0782/lib/internal/http2/core.js#L1871

internal/process/stdio.js

• https://github.com/nodejs/node/blob/b003c86bf45d0689d7e77fce0489a6de888b0782/lib/internal/process/stdio.js#L106
• https://github.com/nodejs/node/blob/b003c86bf45d0689d7e77fce0489a6de888b0782/lib/internal/process/stdio.js#L115

lib/net.js

• https://github.com/nodejs/node/blob/b003c86bf45d0689d7e77fce0489a6de888b0782/lib/net.js#L351
• https://github.com/nodejs/node/blob/b003c86bf45d0689d7e77fce0489a6de888b0782/lib/net.js#L352
• https://github.com/nodejs/node/blob/b003c86bf45d0689d7e77fce0489a6de888b0782/lib/net.js#L378
• https://github.com/nodejs/node/blob/b003c86bf45d0689d7e77fce0489a6de888b0782/lib/net.js#L386

_writableState

_http_server.js

• https://github.com/nodejs/node/blob/b003c86bf45d0689d7e77fce0489a6de888b0782/lib/_http_server.js#L601

_tls_wrap.js

• https://github.com/nodejs/node/blob/ff4ca647d4fee54c7ccf969d721d5c5a8c7e6fd7/lib/_tls_wrap.js#L254
• https://github.com/nodejs/node/blob/ff4ca647d4fee54c7ccf969d721d5c5a8c7e6fd7/lib/_tls_wrap.js#L272

console.js

• https://github.com/nodejs/node/blob/ff4ca647d4fee54c7ccf969d721d5c5a8c7e6fd7/lib/console.js#L129

internal/http2/compat.js

• https://github.com/nodejs/node/blob/b003c86bf45d0689d7e77fce0489a6de888b0782/lib/internal/http2/compat.js#L392

internal/http2/core.js

• https://github.com/nodejs/node/blob/b003c86bf45d0689d7e77fce0489a6de888b0782/lib/internal/http2/core.js#L1473
• https://github.com/nodejs/node/blob/b003c86bf45d0689d7e77fce0489a6de888b0782/lib/internal/http2/core.js#L1549
• https://github.com/nodejs/node/blob/b003c86bf45d0689d7e77fce0489a6de888b0782/lib/internal/http2/core.js#L1872
• https://github.com/nodejs/node/blob/b003c86bf45d0689d7e77fce0489a6de888b0782/lib/internal/http2/core.js#L1873

internal/process/stdio.js

• https://github.com/nodejs/node/blob/b003c86bf45d0689d7e77fce0489a6de888b0782/lib/internal/process/stdio.js#L90

net.js

• https://github.com/nodejs/node/blob/b003c86bf45d0689d7e77fce0489a6de888b0782/lib/net.js#L300
• https://github.com/nodejs/node/blob/b003c86bf45d0689d7e77fce0489a6de888b0782/lib/net.js#L570
• https://github.com/nodejs/node/blob/b003c86bf45d0689d7e77fce0489a6de888b0782/lib/net.js#L785

zlib.js

• https://github.com/nodejs/node/blob/b003c86bf45d0689d7e77fce0489a6de888b0782/lib/zlib.js#L396
• https://github.com/nodejs/node/blob/b003c86bf45d0689d7e77fce0489a6de888b0782/lib/zlib.js#L438

[ryzokuken]

@mcollina looks good? ~Will add _writableState by today too.~ EDIT: Done.

[ryzokuken]

@mcollina I see that a ton of these are accessing the ended and ending properties. Perhaps we could refactor properties like this into separate publicly accessible properties for such streams?

[mcollina]

Yes, go ahead and add writableEnding and writableEnded.

[mcollina]

Also, we have ending, ended and finished. We should definitely clarify and document the three of them. cc @mafintosh.

[brettz9]

FWIW, I'm finding a need (at least based on my admittedly limited experience with streams) for introspecting on the _readableState.encoding (so as to pass a user's stream encoding on to a child process stream).

[mcollina]

Would you like to send a PR to add a stream.readableEncoding getter (and maybe setter)?

[brettz9]

I expect a few days to be able to get to it, but I should be able to...

[brettz9]

My apologies, it seems getting the Node environment set up is not trivial, or at least, I'd probably need more hand-holding than it'd be worth.

FWIW, I came up with the following for lib/stream.js which it seems to me may be consonant with what you were looking for:

Object.defineProperty(Stream.prototype, 'readableEncoding', {
  enumerable: false,
  get() {
    return (!this.readable || !this._readableState) ?
      null :
      this._readableState.encoding;
  },
  set(value) {
    this.setEncoding && this.setEncoding(value);
  }
});

[benhalverson]

@mcollina does this still need to be worked on?

[mcollina]

Yes it does! Would you like to work on it? It would be great to have an up-to-date list of offenders.

[benhalverson]

Yes! how do i know which ones still need to be worked on?

[mcollina]

First of all we should compile an up-to-date version of that list. Using grep _readableState and grep _writableState should give you up-to-date results.

[benhalverson]

If I dont include the /internal/streams/* or _stream* https://github.com/nodejs/node/issues/445#issuecomment-381319831
I see these files have _writeableState

./net.js:  if (this._writableState.finished)
./net.js:  const state = this._writableState;
./net.js:    if (!self._writableState.ended)
./_http_server.js:    var ws = socket._writableState;
./internal/http2/core.js:  const { ending, finished } = stream._writableState;
./internal/http2/core.js:      writableState: this._writableState
./internal/http2/core.js:    if (this._writableState.finished) {
./internal/http2/compat.js:           stream._writableState.ended ||
./internal/process/stdio.js:        stdin._writableState.ended = true;
./internal/console/constructor.js:    if (err !== null && !stream._writableState.errorEmitted) {
./zlib.js:  var ws = this._writableState;
./zlib.js:  var ws = this._writableState;

These files have _readableState

./net.js:        self._readableState);
./net.js:  if (!self.readable || self._readableState.ended) {
./child_process.js:        child.stdout._readableState &&
./child_process.js:        child.stdout._readableState.encoding
./child_process.js:        child.stderr._readableState &&
./child_process.js:        child.stderr._readableState.encoding
./child_process.js:      var encoding = child.stdout._readableState.encoding;
./child_process.js:      var encoding = child.stderr._readableState.encoding;
./_http_server.js:  if (!req._consuming && !req._readableState.resumeScheduled)
./_http_incoming.js:  this._readableState.readingMore = true;
./_http_incoming.js:    this._readableState.readingMore = false;
./internal/child_process.js:    if (!stream || !stream.readable || stream._readableState.readableListening)
./internal/worker.js:    if (!stdout._readableState.ended) {
./internal/worker.js:    if (!stderr._readableState.ended) {
./internal/http2/core.js:    this._readableState.readingMore = true;
./internal/http2/core.js:      readableState: this._readableState,
./internal/http2/core.js:      this._readableState.readingMore = false;
./internal/http2/compat.js:  if (!state.didRead && !req._readableState.resumeScheduled)
./internal/http2/compat.js:    return this._readableState.ended ||
./internal/js_stream_socket.js:          stream._readableState.objectMode === true) {
./internal/stream_base_commons.js:  if (stream._readableState.endEmitted) {
./internal/process/stdio.js:      stdin._readableState.reading = false;
./internal/process/stdio.js:      if (stdin._handle.reading && !stdin._readableState.flowing) {
./internal/process/stdio.js:        stdin._readableState.reading = false;

[mcollina]

I think you can start adding:

• stream.readableEncoding
• stream.writableEncoding
• stream.readableObjectMode
• stream.readableEnded
• stream.writableObjectMode
• stream.writableEnded

[ryzokuken]

@mcollina when I first began working on this, I didn't have enough know-how to perform refactors the size which this required, but I do now. Let me know if there's any methods (maybe a few from this list) I could help implement.

[mcollina]

We need to refactor the cases where readMore is being touched. It shouldn’t be. We might provide a constructor option, or some other mechanism to achieve that behavior

[aug2uag]

lib/_stream_writeable.js:368 references the ending property for the ended state and documented in doc/api/stream.md:497

is this intended or would the ending and ended states be separated?

[mcollina]

Those are separate state.

@Trott I do not think this is an issue for hacktoberfest.

[aug2uag]

@mcollina i submitted PR #29988 that may be useful

[jasnell]

@nodejs/streams ... what is the status on this?

[mcollina]

There are still a few cases of this:

_http_incoming.js:53:  this._readableState.readingMore = true;
_http_incoming.js:106:    this._readableState.readingMore = false;
_http_server.js:703:  if (!req._consuming && !req._readableState.resumeScheduled)
internal/fs/streams.js:155:    return this._readableState.autoDestroy;
internal/fs/streams.js:158:    this._readableState.autoDestroy = val;
internal/child_process.js:428:      stream._stdio._readableState.reading = false;
internal/stream_base_commons.js:215:  if (stream._readableState.endEmitted) {
internal/http2/core.js:1808:    this._readableState.readingMore = true;
internal/http2/core.js:1866:      readableState: this._readableState,
internal/http2/core.js:2030:      this._readableState.readingMore = false;
internal/http2/compat.js:273:  if (!state.didRead && !req._readableState.resumeScheduled)
internal/bootstrap/switches/is_main_thread.js:206:    stdin._readableState.reading = false;
internal/bootstrap/switches/is_main_thread.js:221:      stdin._readableState.reading = false;
internal/quic/core.js:2604:    this._readableState.readingMore = true;
internal/quic/core.js:2723:      readableState: this._readableState,
internal/quic/core.js:2836:      this._readableState.readingMore = false;
zlib.js:361:  const ws = this._writableState;
zlib.js:398:  const ws = this._writableState;
_http_outgoing.js:819:    this.socket._writableState.corked = 1;
_http_server.js:755:    const ws = socket._writableState;
net.js:792:  const state = this._writableState;

Most of those are __hard__ to fix and they show some problems in the underlining machinery. I don't think we should expose readingMore (or reading)., and allow implementors to change that... however that seems what is needed here.

What do you think @ronag?

[ronag]

We could avoid some of these that are not strictly stream state and put them on the object itself behind symbols:

e.g.

_http_incoming.js:53:  this._readableState.readingMore = true;
_http_incoming.js:106:    this._readableState.readingMore = false;
_http_server.js:703:  if (!req._consuming && !req._readableState.resumeScheduled)
internal/http2/core.js:1808:    this._readableState.readingMore = true;
internal/http2/core.js:2030:      this._readableState.readingMore = false;
internal/http2/compat.js:273:  if (!state.didRead && !req._readableState.resumeScheduled)
internal/quic/core.js:2604:    this._readableState.readingMore = true;
internal/quic/core.js:2723:      readableState: this._readableState,
internal/quic/core.js:2836:      this._readableState.readingMore = false;

The zlib ones should be possible to fix as there are corresponding public API's.

zlib.js:361:  const ws = this._writableState;
zlib.js:398:  const ws = this._writableState;

HTTP outgoing there is nothing we can do about, it tries to be a Writable.

_http_outgoing.js:819:    this.socket._writableState.corked = 1;

This seems like a noop.

net.js:792:  const state = this._writableState;

I still think there are things we can further improve if someone wants to put the effort into it.

I'm a little concerned about quic accessing internal stream properties. However, @jasnell is still working on the js side of quic. I will try and help out later and see if we can avoid it there.

[aug2uag]

If the challenges can be reasonably enumerated I'd love to give this a go.

[mcollina]

Part of the problem that each case is unique and writing a detailed description on how to solve it takes as much time as doing the work.

[aug2uag]

I hope there's time for both! Would make for neat reading

or if there could be test cases

[mcollina]

I would recommend start with zlib.