Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cleanup _writableState and _readableState access across codebase #445

Closed
14 of 44 tasks
vkurchatkin opened this issue Jan 15, 2015 · 67 comments
Closed
14 of 44 tasks

Cleanup _writableState and _readableState access across codebase #445

vkurchatkin opened this issue Jan 15, 2015 · 67 comments
Labels
stalled Issues and PRs that are stalled. stream Issues and PRs related to the stream subsystem.

Comments

@vkurchatkin
Copy link
Contributor

vkurchatkin commented Jan 15, 2015

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

lib/_debug_agent.js

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 967b5db by @isaacs;
  • L408 var needPause = socket._writableState.needDrain. Added in 085dd30 by @isaacs ;
  • L445 req._readableState.resumeScheduled. Not sure where this originated, but in 2efe4ab @indutny added oldMode check here;

lib/_tls_legacy.js

  • L421 this._writableState.finished;
  • L508 self._readableState.length > 0;

lib/_tls_wrap.js

lib/child_process.js

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' (crypto: remove use of this._readableState #610);

lib/fs.js

  • L1624 allocNewPool(this._readableState.highWaterMark);

lib/net.js

  • L162 this._writableState.decodeStrings = false (Cleanup stream state in net #465);
  • L174 this._readableState.flowing = false (Cleanup stream state in net #465);
  • L196 this._readableState.ended (Cleanup stream state in net #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) (Cleanup stream state in net #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 (Cleanup stream state in net #465);
  • L433 self._writableState.errorEmitted = true (Cleanup stream state in net #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 (Cleanup stream state in net #465);

lib/zlib.js

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
Copy link
Contributor

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 pushed a commit that referenced this issue Jan 15, 2015
Use public `readableObjectMode` option to construct `Transform`
instead of accessing private `_readableState.objectMode`.

Partially addresses #445.

PR-URL: #270
Reviewed-By: Fedor Indutny <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Bert Belder <[email protected]>
Reviewed-By: Chris Dickinson <[email protected]>
@chrisdickinson
Copy link
Contributor

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
Copy link
Contributor Author

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

@chrisdickinson
Copy link
Contributor

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
Copy link
Contributor Author

@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
Copy link
Contributor

We might add lib/zlib.js:

@trevnorris trevnorris added the stream Issues and PRs related to the stream subsystem. label Jan 22, 2015
calvinmetcalf added a commit to calvinmetcalf/io.js that referenced this issue Jan 26, 2015
Per nodejs#445 this removes a reference to this._readableState in hash._flush. It was
used to get the encoding on the readable side to pass to the writable side but
omiting it just causes the stream to handle the encoding issues.
vkurchatkin pushed a commit that referenced this issue Jan 28, 2015
Per #445 this removes a reference to this._readableState in hash._flush. It was
used to get the encoding on the readable side to pass to the writable side but
omitting it just causes the stream to handle the encoding issues.

PR-URL: #610
Reviewed-By: Chris Dickinson <[email protected]>
Reviewed-By: Fedor Indutny <[email protected]>
Reviewed-By: Vladimir Kurchatkin <[email protected]>
calvinmetcalf added a commit to calvinmetcalf/io.js that referenced this issue Jan 30, 2015
Makes streams lazy by default so crypto does not need to monky patch them in
order to get good performance. This helps the state objects become part of the
private api in nodejs#445.
@chrisdickinson
Copy link
Contributor

Cross posting this from nodejs/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.

@brendanashworth brendanashworth added good first issue Issues that are suitable for first-time contributors. and removed good first issue Issues that are suitable for first-time contributors. labels Aug 29, 2015
@Trott
Copy link
Member

Trott commented Jan 19, 2016

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

@indutny
Copy link
Member

indutny commented Jan 19, 2016

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

@Fishrock123
Copy link
Contributor

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

nodejs-github-bot pushed a commit that referenced this issue Aug 29, 2020
Refs: #445

PR-URL: #34885
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Ricky Zhou <[email protected]>
richardlau pushed a commit that referenced this issue Sep 1, 2020
Refs: #445

PR-URL: #34884
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
richardlau pushed a commit that referenced this issue Sep 1, 2020
Refs: #445

PR-URL: #34885
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Ricky Zhou <[email protected]>
addaleax pushed a commit that referenced this issue Sep 22, 2020
Refs: #445

PR-URL: #34884
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
addaleax pushed a commit that referenced this issue Sep 22, 2020
Refs: #445

PR-URL: #34885
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Ricky Zhou <[email protected]>
addaleax pushed a commit that referenced this issue Sep 22, 2020
Refs: #445

PR-URL: #34884
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
addaleax pushed a commit that referenced this issue Sep 22, 2020
Refs: #445

PR-URL: #34885
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Ricky Zhou <[email protected]>
Trott added a commit to Trott/io.js that referenced this issue Oct 19, 2022
Trott added a commit to Trott/io.js that referenced this issue Oct 19, 2022
nodejs-github-bot pushed a commit that referenced this issue Oct 21, 2022
Ref: #445
PR-URL: #45063
Refs: #445
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Paolo Insogna <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
Reviewed-By: James M Snell <[email protected]>
RafaelGSS pushed a commit that referenced this issue Nov 1, 2022
Ref: #445
PR-URL: #45063
Refs: #445
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Paolo Insogna <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
Reviewed-By: James M Snell <[email protected]>
RafaelGSS pushed a commit that referenced this issue Nov 10, 2022
Ref: #445
PR-URL: #45063
Refs: #445
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Paolo Insogna <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@lubas569

This comment was marked as off-topic.

danielleadams pushed a commit that referenced this issue Dec 30, 2022
Ref: #445
PR-URL: #45063
Refs: #445
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Paolo Insogna <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
Reviewed-By: James M Snell <[email protected]>
danielleadams pushed a commit that referenced this issue Dec 30, 2022
Ref: #445
PR-URL: #45063
Refs: #445
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Paolo Insogna <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
Reviewed-By: James M Snell <[email protected]>
danielleadams pushed a commit that referenced this issue Jan 3, 2023
Ref: #445
PR-URL: #45063
Refs: #445
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Paolo Insogna <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@RedYetiDev
Copy link
Member

RedYetiDev commented Sep 24, 2024

(The stalled bot doesn't appear to be working, so I've manually closed. Feel free to reopen if this is no longer stalled)

@RedYetiDev RedYetiDev closed this as not planned Won't fix, can't repro, duplicate, stale Sep 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stalled Issues and PRs that are stalled. stream Issues and PRs related to the stream subsystem.
Projects
None yet
Development

No branches or pull requests