streams: remove all references to readableState/writableState from docs by calvinmetcalf · Pull Request #12855 · nodejs/node · GitHub

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)

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.

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?).

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

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

@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?

@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.

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.

Some nits

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

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

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

same here.

@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.

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

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

LGTM

@calvinmetcalf are you adding the benchmark as well?

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

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);
  });
}

ok added the benchmark

CI: https://ci.nodejs.org/job/node-test-pull-request/8122/

fixed the failing test

@mscdex can you have a look at this one?

Also @nodejs/streams.

Perhaps this part should be rewritten as it appears to give the illusion that you can assign values via this property.

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 ?

CI: https://ci.nodejs.org/job/node-test-pull-request/12035/
CITGTM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1142/

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/.

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

CITGM https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1153/
CITGM (master): https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1154/

CITGM seems clear, landing

Landed as e20af33.

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>

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

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

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 added the stream Issues and PRs related to the stream subsystem. label May 5, 2017

benjamingr approved these changes May 5, 2017

View reviewed changes

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 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

mcollina requested changes May 16, 2017

View reviewed changes

calvinmetcalf commented May 16, 2017

View reviewed changes

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

mcollina approved these changes May 17, 2017

View reviewed changes

calvinmetcalf force-pushed the rm-readable-state-docs branch from 6b498f2 to 6daccce Compare May 17, 2017 14:36

calvinmetcalf force-pushed the rm-readable-state-docs branch from 6daccce to 4f64775 Compare May 18, 2017 13:40

mscdex reviewed May 18, 2017

View reviewed changes

mcollina added the semver-minor PRs that contain new features and should be released in the next minor version. label Dec 11, 2017

BridgeAR approved these changes Dec 11, 2017

View reviewed changes

BridgeAR added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Dec 15, 2017

mcollina closed this Dec 15, 2017

mcollina deleted the rm-readable-state-docs branch December 15, 2017 22:11

addaleax removed the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Dec 29, 2017

MylesBorins mentioned this pull request Jan 10, 2018

v9.4.0 proposal #18069

Merged

MylesBorins added the dont-land-on-v8.x label Aug 2, 2018

streams: remove all references to readableState/writableState from docs by calvinmetcalf · Pull Request #12855 · nodejs/node · GitHub

Uh oh!

Uh oh!

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

streams: remove all references to readableState/writableState from docs#12855calvinmetcalf wants to merge 7 commits intonodejs:masternodejs/node:masterfrom calvinmetcalf:rm-readable-state-docscalvinmetcalf/io.js:rm-readable-state-docsCopy head branch name to clipboard

Conversation

calvinmetcalf commented May 5, 2017 • edited by mcollina Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Uh oh!

mscdex commented May 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mscdex commented May 5, 2017

Uh oh!

mscdex commented May 5, 2017

Uh oh!

mcollina commented May 11, 2017

Uh oh!

mcollina commented May 16, 2017

Uh oh!

mscdex commented May 16, 2017

Uh oh!

mcollina commented May 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

mcollina May 16, 2017

Choose a reason for hiding this comment

Uh oh!

mcollina May 16, 2017

Choose a reason for hiding this comment

Uh oh!

mcollina May 16, 2017

Choose a reason for hiding this comment

Uh oh!

mcollina May 16, 2017

Choose a reason for hiding this comment

Uh oh!

mscdex commented May 16, 2017

Uh oh!

calvinmetcalf May 16, 2017

Choose a reason for hiding this comment

Uh oh!

calvinmetcalf commented May 16, 2017

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

calvinmetcalf commented May 17, 2017

Uh oh!

mcollina commented May 17, 2017

Uh oh!

calvinmetcalf commented May 17, 2017

Uh oh!

mcollina commented May 17, 2017

Uh oh!

calvinmetcalf commented May 18, 2017

Uh oh!

mcollina commented May 18, 2017

Uh oh!

mscdex May 18, 2017

Choose a reason for hiding this comment

Uh oh!

mcollina commented Dec 11, 2017

Uh oh!

mcollina commented Dec 11, 2017

Uh oh!

mcollina commented Dec 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

apapirovski commented Dec 12, 2017

Uh oh!

mcollina commented Dec 15, 2017

Uh oh!

mcollina commented Dec 15, 2017

Uh oh!

streams: remove all references to readableState/writableState from docs#12855
calvinmetcalf wants to merge 7 commits into
nodejs:masterfrom
calvinmetcalf:rm-readable-state-docs

calvinmetcalf commented May 5, 2017 •
edited by mcollina
Loading

mscdex commented May 5, 2017 •
edited
Loading

mcollina commented May 16, 2017 •
edited
Loading

mcollina commented Dec 12, 2017 •
edited
Loading