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

streams: remove all references to readableState/writableState from docs #12855

Closed

Conversation

calvinmetcalf
Copy link
Contributor

@calvinmetcalf calvinmetcalf commented May 5, 2017

This removes all the refereces to readableState and writableState from the
docs by adding some computed properties to allow for the same functionality
that was exposed by the use of readableState and writableState.

We add writeBuffer and readBuffer getters for the state buffers and
we have a flowing getter for readableState.flowing, I also add a
._setFlowing for the internal places we need to set the flowing state.

Part of the readableState/writableState mega issue #445.
Fixes #6799.

cc @nodejs/streams

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

streams

(edit @mcollina: added link to a open bug on this)

@nodejs-github-bot nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label May 5, 2017
@calvinmetcalf calvinmetcalf added the stream Issues and PRs related to the stream subsystem. label May 5, 2017
@calvinmetcalf calvinmetcalf added http Issues or PRs related to the http subsystem. net Issues and PRs related to the net subsystem. labels May 5, 2017
@mscdex
Copy link
Contributor

mscdex commented May 5, 2017

There could be a problem with adding a non-underscore-prefixed property to all Readable instances (e.g. readBuffer, flowing, etc.). Have you at least ran CITGM on such changes?

I could easily see (network) protocol implementors using names such as readBuffer and writeBuffer on their stream instances. This is why having just a single property (e.g. _readableState) for accessing stream state is better because it means less potential collisions with custom stream objects.

@mscdex mscdex added wip Issues and PRs that are still a work in progress. and removed wip Issues and PRs that are still a work in progress. labels May 5, 2017
@mscdex
Copy link
Contributor

mscdex commented May 5, 2017

The commit description is a bit misleading, it sounds like it's only making docs changes or making code changes just to change docs in various ways (except I'm guessing the code changes are instead really meant to provide a more stable interface for stream state?).

@mscdex
Copy link
Contributor

mscdex commented May 5, 2017

Also, as with #12860, is performance impacted by these changes?

@mcollina
Copy link
Member

I will run the benchmarks of the relevant subsystems when I get back.

@mcollina
Copy link
Member

@mscdex how much time the benchmarks should run? I'm 8 hours in the benchmark/compare.js http benchmark, and it's still at 31%. Do you want to see something specific?

@mscdex
Copy link
Contributor

mscdex commented May 16, 2017

@mcollina I don't believe the existing benchmarks will exercise the (http) code paths changed by this PR. Specifically, it looks like connect/upgrade http requests need to be sent/received with listeners for those events added. Might need to craft a new little benchmark to test this scenario.

@mcollina
Copy link
Member

mcollina commented May 16, 2017

benchmarks from net:

$ cat compare-remove-access-net.csv | Rscript benchmark/compare.R
                                                   improvement confidence    p.value
 net/net-c2s-cork.js dur=5 type="buf" len=1024         -0.64 %            0.12929603
 net/net-c2s-cork.js dur=5 type="buf" len=128           0.06 %            0.86520957
 net/net-c2s-cork.js dur=5 type="buf" len=16           -0.24 %            0.27783431
 net/net-c2s-cork.js dur=5 type="buf" len=32           -0.01 %            0.94641307
 net/net-c2s-cork.js dur=5 type="buf" len=4             0.08 %            0.65879417
 net/net-c2s-cork.js dur=5 type="buf" len=512          -0.20 %            0.59834906
 net/net-c2s-cork.js dur=5 type="buf" len=64           -0.13 %            0.62751004
 net/net-c2s-cork.js dur=5 type="buf" len=8             0.01 %            0.96812210
 net/net-c2s.js dur=5 type="asc" len=102400             0.83 %            0.06647777
 net/net-c2s.js dur=5 type="asc" len=16777216          -0.20 %            0.38523291
 net/net-c2s.js dur=5 type="buf" len=102400            -0.23 %            0.51936416
 net/net-c2s.js dur=5 type="buf" len=16777216           0.15 %            0.59419811
 net/net-c2s.js dur=5 type="utf" len=102400             0.08 %            0.88572038
 net/net-c2s.js dur=5 type="utf" len=16777216           0.16 %            0.40569601
 net/net-pipe.js dur=5 type="asc" len=102400            0.62 %            0.22726849
 net/net-pipe.js dur=5 type="asc" len=16777216         -0.38 %            0.35615455
 net/net-pipe.js dur=5 type="buf" len=102400           -0.14 %            0.71665559
 net/net-pipe.js dur=5 type="buf" len=16777216          0.14 %            0.65102610
 net/net-pipe.js dur=5 type="utf" len=102400            0.24 %            0.67870301
 net/net-pipe.js dur=5 type="utf" len=16777216         -0.31 %            0.22689837
 net/net-s2c.js dur=5 type="asc" len=102400             0.29 %            0.48483245
 net/net-s2c.js dur=5 type="asc" len=16777216          -0.07 %            0.72761635
 net/net-s2c.js dur=5 type="buf" len=102400             0.01 %            0.95935582
 net/net-s2c.js dur=5 type="buf" len=16777216           0.02 %            0.94113175
 net/net-s2c.js dur=5 type="utf" len=102400             0.02 %            0.98168133
 net/net-s2c.js dur=5 type="utf" len=16777216           0.15 %            0.45777973
 net/tcp-raw-c2s.js dur=5 type="asc" len=102400         0.67 %            0.12001540
 net/tcp-raw-c2s.js dur=5 type="asc" len=16777216      -0.05 %            0.70525192
 net/tcp-raw-c2s.js dur=5 type="buf" len=102400         0.97 %            0.06538486
 net/tcp-raw-c2s.js dur=5 type="buf" len=16777216       0.15 %            0.46833092
 net/tcp-raw-c2s.js dur=5 type="utf" len=102400         0.78 %            0.27088741
 net/tcp-raw-c2s.js dur=5 type="utf" len=16777216      -0.11 %            0.42554888
 net/tcp-raw-pipe.js dur=5 type="asc" len=102400       -2.02 %            0.62683795
 net/tcp-raw-pipe.js dur=5 type="asc" len=16777216     -3.46 %            0.27002966
 net/tcp-raw-pipe.js dur=5 type="buf" len=102400       -3.53 %            0.40890117
 net/tcp-raw-pipe.js dur=5 type="buf" len=16777216     -4.60 %            0.10957039
 net/tcp-raw-pipe.js dur=5 type="utf" len=102400       -3.05 %            0.47004913
 net/tcp-raw-pipe.js dur=5 type="utf" len=16777216     -3.83 %            0.26013753
 net/tcp-raw-s2c.js dur=5 type="asc" len=102400         0.64 %            0.14627038
 net/tcp-raw-s2c.js dur=5 type="asc" len=16777216      -0.14 %            0.36022011
 net/tcp-raw-s2c.js dur=5 type="buf" len=102400         0.19 %            0.40813902
 net/tcp-raw-s2c.js dur=5 type="buf" len=16777216      -0.04 %            0.92686921
 net/tcp-raw-s2c.js dur=5 type="utf" len=102400        -0.31 %            0.58679716
 net/tcp-raw-s2c.js dur=5 type="utf" len=16777216      -0.11 %            0.56693651

Benchmarks for streams:

                                          improvement confidence    p.value
 streams/readable-bigread.js n=1000            0.14 %            0.89385560
 streams/readable-bigunevenread.js n=1000     -0.16 %            0.41762734
 streams/readable-boundaryread.js n=2000      -0.04 %            0.83768171
 streams/readable-readall.js n=5000           -0.20 %            0.49418135
 streams/readable-unevenread.js n=1000         0.30 %            0.45258734
 streams/writable-manywrites.js n=2000000     -1.02 %          * 0.03359655

It seems there is no downside for this patch, even though it is definitely making it more complicated for the engine.

@mscdex can we copy-and-past an existing one and add a listener? I'm not entirely familiar with that code path. It's exercised for websockets I presume. Anyway, I do not think it will cause anything more that what we saw.

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some nits

@@ -481,7 +481,7 @@ function onParserExecuteCommon(server, socket, parser, state, ret, d) {

// TODO(isaacs): Need a way to reset a stream to fresh state
// IE, not flowing, and not explicitly paused.
socket._readableState.flowing = null;
socket._setFlowing(null);
server.emit(eventName, req, socket, bodyHead);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

while we are at it, can we remove the TODO(isaacs) and call this _resetFlowing() instead?

@@ -61,6 +61,12 @@ function Duplex(options) {
this.once('end', onend);
}

Object.defineProperty(Duplex.prototype, 'writeBuffer', {
get: function() {
return this._writableState.getBuffer();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to be protected, as this._writableState  could not be initialized yet.


// so that http_client can set it
Readable.prototype._setFlowing = function(state) {
this._readableState.flowing = state;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same for all those three. In the constructor this._readableState might not be populated.

@@ -291,6 +291,12 @@ Writable.prototype.setDefaultEncoding = function setDefaultEncoding(encoding) {
return this;
};

Object.defineProperty(Writable.prototype, 'writeBuffer', {
get: function() {
return this._writableState.getBuffer();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here.

@mscdex
Copy link
Contributor

mscdex commented May 16, 2017

@mcollina I don't think the net changes are really worth worrying about since they are not hot paths. One requires setting the pauseOnCreate option when connecting a socket and the other requires accessing socket.bytesWritten.

The streams changes should not be affected at all because they do not utilize the new getters/setters themselves.

As far as creating a new http benchmark for this, you might be able to use an existing connect/upgrade test as a guide.

lib/net.js Outdated
@@ -221,7 +221,7 @@ function Socket(options) {
// stop the handle from reading and pause the stream
this._handle.reading = false;
this._handle.readStop();
this._readableState.flowing = false;
this._setFlowing(false);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mcollina this is why it's _setFlowing and not _resetflowing

@calvinmetcalf calvinmetcalf force-pushed the rm-readable-state-docs branch 2 times, most recently from e57b0ab to 6b498f2 Compare May 16, 2017 15:11
@calvinmetcalf
Copy link
Contributor Author

ok updated with changes readBuffer and writeBuffer become readableBuffer and writableBuffer, also broke it up into separate commits for the separate subsystems

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@calvinmetcalf are you adding the benchmark as well?

@calvinmetcalf
Copy link
Contributor Author

@mcollina I'm not super familiar with benchmarks so I can if somebody wants to walk me through it

@mcollina
Copy link
Member

This PR cause no regression in the upgrade path, however that has a regression even in master to v6.

$ cat compare-pr-12855.csv | Rscript benchmark/compare.R
                        improvement confidence   p.value
 http/upgrade.js n=1000     -1.96 %            0.8006479
 http/upgrade.js n=5        -0.55 %            0.8759959

The benchmark code:

'use strict';

const common = require('../common.js');
const PORT = common.PORT;
const net = require('net');

const bench = common.createBenchmark(main, {
  n: [5, 1000]
});

const reqData = 'GET / HTTP/1.1\r\n' +
  'Upgrade: WebSocket\r\n' +
  'Connection: Upgrade\r\n' +
  '\r\n' +
  'WjN}|M(6';

const resData = 'HTTP/1.1 101 Web Socket Protocol Handshake\r\n' +
  'Upgrade: WebSocket\r\n' +
  'Connection: Upgrade\r\n' +
  '\r\n\r\n';

function main(conf) {
  process.env.PORT = PORT;
  var server = require('../fixtures/simple-http-server.js')
  .listen(process.env.PORT || common.PORT)
  .on('listening', function() {
    bench.start()
    doBench(server.address(), +conf.n, function() {
      bench.end(+conf.n);
      server.close();
    });
  })
  .on('upgrade', function(req, socket, upgradeHead) {
    socket.resume();
    socket.write(resData);
    socket.end();
  })
}

function doBench(address, count, done) {
  if (count === 0) {
    done()
    return
  }

  const conn = net.createConnection(address.port);
  conn.write(reqData);
  conn.resume()

  conn.on('end', function() {
    doBench(address, count - 1, done);
  });
}

@calvinmetcalf
Copy link
Contributor Author

ok added the benchmark

@mcollina
Copy link
Member

@calvinmetcalf
Copy link
Contributor Author

fixed the failing test

@mcollina
Copy link
Member

@mscdex can you have a look at this one?

Also @nodejs/streams.

@mcollina
Copy link
Member

I have addressed the various nits.

I've added the semver-minor tag as it's adding new functionality.

@BridgeAR @jasnell please get another pass.

@calvinmetcalf @nodejs/streams ?

@mcollina
Copy link
Member

mcollina commented Dec 12, 2017

Ava tests are failing. Running CITGM on master to check if there are differences: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1145/.

@apapirovski
Copy link
Member

There was a domain-related PR that broke it. Here's the fix which needs some approvals: #17588

@mcollina
Copy link
Member

CITGM seems clear, landing

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 15, 2017
@mcollina
Copy link
Member

Landed as e20af33.

@mcollina mcollina closed this Dec 15, 2017
@mcollina mcollina deleted the rm-readable-state-docs branch December 15, 2017 22:11
mcollina pushed a commit that referenced this pull request Dec 15, 2017
This adds computed properties to readable and writable streams to
allow access to the readable buffer, the writable buffer, and flow
state without accessing the readable or writable state.

These are the only uses of readable and writable state in the docs
so adding these work arounds allows them to be removed from the docs.

This also updates net, http_client and http_server to use the new
methods instead of manipulating readable and writable state directly.

See: #445
PR-URL: #12855
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
@addaleax addaleax removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 29, 2017
MylesBorins pushed a commit that referenced this pull request Jan 8, 2018
This adds computed properties to readable and writable streams to
allow access to the readable buffer, the writable buffer, and flow
state without accessing the readable or writable state.

These are the only uses of readable and writable state in the docs
so adding these work arounds allows them to be removed from the docs.

This also updates net, http_client and http_server to use the new
methods instead of manipulating readable and writable state directly.

See: #445
PR-URL: #12855
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jan 9, 2018
This adds computed properties to readable and writable streams to
allow access to the readable buffer, the writable buffer, and flow
state without accessing the readable or writable state.

These are the only uses of readable and writable state in the docs
so adding these work arounds allows them to be removed from the docs.

This also updates net, http_client and http_server to use the new
methods instead of manipulating readable and writable state directly.

See: #445
PR-URL: #12855
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Jan 10, 2018
MylesBorins added a commit that referenced this pull request Jan 10, 2018
Notable change:

* async_hooks:
  - deprecate AsyncHooks Sensitive API and runInAsyncIdScope. Neither
    API were documented. (Andreas Madsen)
    #16972
* deps:
  - update nghttp2 to 1.29.0 (James M Snell)
    #17908
  - upgrade npm to 5.6.0 (Kat Marchán)
    #17535
  - cherry-pick 50f7455 from upstream V8 (Michaël Zasso)
    #16591
* events:
  - remove reaches into _events internals (Anatoli Papirovski)
    #17440
* http:
  - add rawPacket in err of `clientError` event (XadillaX)
    #17672
* http2:
  - implement maxSessionMemory (James M Snell)
    #17967
  - add initial support for originSet (James M Snell)
    #17935
  - add altsvc support (James M Snell)
    #17917
  - perf_hooks integration (James M Snell)
    #17906
* net:
  - remove Socket.prototype.write (Anna Henningsen)
    #17644
  - remove Socket.prototype.listen (Ruben Bridgewater)
    #13735
* repl:
  - show lexically scoped vars in tab completion (Michaël Zasso)
    #16591
* stream:
  - rm {writeable/readable}State.length (Calvin Metcalf)
    #12857
  - add flow and buffer properties to streams (Calvin Metcalf)
    #12855
* util:
  - allow wildcards in NODE_DEBUG variable (Tyler)
    #17609
* zlib:
  - add ArrayBuffer support (Jem Bezooyen)
    #16042
* Addedew collaborator**
  - [starkwang](https://github.com/starkwang) Weijia Wang
* Addedew TSC member**
  - [danbev](https://github.com/danbev) Daniel Bevenius

PR-URL: #18069
MylesBorins added a commit that referenced this pull request Jan 10, 2018
Notable change:

* async_hooks:
  - deprecate AsyncHooks Sensitive API and runInAsyncIdScope. Neither
    API were documented. (Andreas Madsen)
    #16972
* deps:
  - update nghttp2 to 1.29.0 (James M Snell)
    #17908
  - upgrade npm to 5.6.0 (Kat Marchán)
    #17535
  - cherry-pick 50f7455 from upstream V8 (Michaël Zasso)
    #16591
* events:
  - remove reaches into _events internals (Anatoli Papirovski)
    #17440
* http:
  - add rawPacket in err of `clientError` event (XadillaX)
    #17672
* http2:
  - implement maxSessionMemory (James M Snell)
    #17967
  - add initial support for originSet (James M Snell)
    #17935
  - add altsvc support (James M Snell)
    #17917
  - perf_hooks integration (James M Snell)
    #17906
  - Refactoring and cleanup of Http2Session and Http2Stream destroy
    (James M Snell) #17406
* net:
  - remove Socket.prototype.write (Anna Henningsen)
    #17644
  - remove Socket.prototype.listen (Ruben Bridgewater)
    #13735
* repl:
  - show lexically scoped vars in tab completion (Michaël Zasso)
    #16591
* stream:
  - rm {writeable/readable}State.length (Calvin Metcalf)
    #12857
  - add flow and buffer properties to streams (Calvin Metcalf)
    #12855
* util:
  - allow wildcards in NODE_DEBUG variable (Tyler)
    #17609
* zlib:
  - add ArrayBuffer support (Jem Bezooyen)
    #16042
* Addedew collaborator**
  - [starkwang](https://github.com/starkwang) Weijia Wang
* Addedew TSC member**
  - [danbev](https://github.com/danbev) Daniel Bevenius

PR-URL: #18069
MylesBorins added a commit that referenced this pull request Jan 10, 2018
Notable change:

* async_hooks:
  - deprecate AsyncHooks Sensitive API and runInAsyncIdScope. Neither
    API were documented. (Andreas Madsen)
    #16972
* deps:
  - update nghttp2 to 1.29.0 (James M Snell)
    #17908
  - upgrade npm to 5.6.0 (Kat Marchán)
    #17535
  - cherry-pick 50f7455 from upstream V8 (Michaël Zasso)
    #16591
* events:
  - remove reaches into _events internals (Anatoli Papirovski)
    #17440
* http:
  - add rawPacket in err of `clientError` event (XadillaX)
    #17672
* http2:
  - implement maxSessionMemory (James M Snell)
    #17967
  - add initial support for originSet (James M Snell)
    #17935
  - add altsvc support (James M Snell)
    #17917
  - perf_hooks integration (James M Snell)
    #17906
  - Refactoring and cleanup of Http2Session and Http2Stream destroy
    (James M Snell) #17406
* net:
  - remove Socket.prototype.write (Anna Henningsen)
    #17644
  - remove Socket.prototype.listen (Ruben Bridgewater)
    #13735
* repl:
  - show lexically scoped vars in tab completion (Michaël Zasso)
    #16591
* stream:
  - rm {writeable/readable}State.length (Calvin Metcalf)
    #12857
  - add flow and buffer properties to streams (Calvin Metcalf)
    #12855
* util:
  - allow wildcards in NODE_DEBUG variable (Tyler)
    #17609
* zlib:
  - add ArrayBuffer support (Jem Bezooyen)
    #16042
* Addedew collaborator**
  - [starkwang](https://github.com/starkwang) Weijia Wang
* Addedew TSC member**
  - [danbev](https://github.com/danbev) Daniel Bevenius

PR-URL: #18069
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
http Issues or PRs related to the http subsystem. lib / src Issues and PRs related to general changes in the lib or src directory. net Issues and PRs related to the net subsystem. semver-minor PRs that contain new features and should be released in the next minor version. stream Issues and PRs related to the stream subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

doc,streams: internal "private" properties are publicly documented
10 participants