net: replace _writableState with writableFinished by Trott · Pull Request #27974 · nodejs/node · GitHub

Replace use of quasi-private streams _writableState property in net.js with
use of the public streams finished() method.

Refs: #445

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

/ping @mcollina

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

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

Since this already had a few approvals I'm not going to block it but in my opinion this:

hinders readability (I have to jump to to eos() definition to see what it does).
trades simplicity and efficiency for complexity (eos() does a lot of unneeded work in this case).
still uses the "private" _writableState property indirectly via eos().

I see only disadvantages.

Commit message title and body should be updated as they are misleading. There is no _finished property.

LGTM

Since this already had a few approvals I'm not going to block it but in my opinion this:

hinders readability (I have to jump to to eos() definition to see what it does).

trades simplicity and efficiency for complexity (eos() does a lot of unneeded work in this case).

still uses the "private" _writableState property indirectly via eos().

I see only disadvantages.

@lpinca I find myself agreeing more than disagreeing with your points. If you were trying to adjust this code in the spirit of #445, what would you recommend? (I can think of a few things, but opted for this as it didn't involve adding new stuff to the stream module, which I would expect to meet resistance.) Or is #445 mostly misguided in your view?

I would add a getter to Writable.prototype like

but yes I think #445 is a bit misguided because it means exposing most (if not all) attributes of ReadableState and WritableState. There are cases also in userland where you really need to read those properties. end-of-stream is just an example.

I think _readableState and _writableState should be public and documented as "use at your own risk".

I would add a getter to Writable.prototype like

https://github.com/nodejs/node/blob/aa8b820a/lib/_stream_writable.js#L354-L362

https://github.com/nodejs/node/blob/aa8b820a/lib/_stream_writable.js#L597-L605

but yes I think #445 is a bit misguided because it means exposing most (if not all) attributes of ReadableState and WritableState. There are cases also in userland where you really need to read those properties. end-of-stream is just an example.

I think _readableState and _writableState should be public and documented as "use at your own risk".

Hey, @nodejs/streams, what do you think?

This PR is the way to go?
A getter like @lpinca proposes above is the way to go?
Close this PR and leave the code alone. (Cleanup _writableState and _readableState access across codebase #445 should probably be closed? Maybe document _readableState and _writableState, even if we discourage people from using them?)

(Is @nodejs/streams the wrong team to ask? Who is the right team?)

Note sure if the callback can be called synchronously? Just to be safe... put it after the end() call? i.e. keep the order as it was

eos() unconditionally adds one listener for the 'finish' event and one for 'close' event (and actually another for 'end' event even if the readable option is false).

If _writableState.finished is already true then the 'finish' event won't be emitted and it will wait for 'close'. Even if this is emitted synchronously it is not a problem because it means the socket is already destroyed and no longer writable.

If _writableState.finished is false then the 'finish' event won't be emitted until socket.end() is called and if this already happened then the socket is no longer writable.

So the order should not matter. However I can see a case where the behavior is different.

'use strict';

const net = require('net');

const server = net.createServer(function(socket) {
  socket.write('foo');
});

server.listen(function() {
  const socket = net.connect(this.address().port, function() {
    socket.on('finish', function() {
      socket.destroySoon();
    });
    socket.on('close', function() {
      console.log('close');
    });

    setTimeout(function() {
      socket.end();
    }, 100);
  });
});

Yes, it's a fabricated example that probably will never happen in reality but with this patch the socket is not destroyed ('close' is not emitted) until the buffered data is actually read. This is not the case with the current implementation of destroySoon().

Close this PR and leave the code alone. (#445 should probably be closed? Maybe document _readableState and _writableState, even if we discourage people from using them?)

Accessing _readableState and _writableState directly is really a bad idea. They contain our buffering system, and those should not really be touched. I fully agree on #445.

A getter like @lpinca proposes above is the way to go?

I'm +1 to that approach (#27974 (comment)).
It's also consistent with the approach we have been following to fix #445.

This PR is the way to go?

I'm +1 to this PR as well. However I concur that finished does a lot of things, and adding a getter would be the most performant way to solve the problem.

(Is @nodejs/streams the wrong team to ask? Who is the right team?)

@nodejs/streams is likely the team to target for this question.

Accessing _readableState and _writableState directly is really a bad idea. They contain our buffering system, and those should not really be touched.

I think if someone messes with the internal buffers it means they either know what they are doing or want to break things intentionally. This is the reason for "use at your own risk".

Take this data for example https://github.com/search?p=1&q=_readableState&type=Code, just from the first results we see use of

resumeScheduled
flowing
pipesCount

Yes, exposing all of that via getters is an option, but it does not seem the right solution to me.

Marking this WIP for now but if someone else wants to open a PR that does this the "right" way before I get around to it, by all means, please do.

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

Used the writableFinished property as implemented by @zero1five in #28007. Removed WIP label. I think this is ready for re-reviews.

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

(@addaleax Sorry, I didn't meant to request a re-review from you since you had already re-reviewed, but I don't see a way to undo my request in the GitHub interface.)

Landed in 3832713

Replace usage of quasi-private _writableState.finished with public writableFinished property. PR-URL: #27974 Refs: #445 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

This comment has been minimized.

Sign in to view

addaleax approved these changes May 30, 2019

View reviewed changes

benjamingr approved these changes May 30, 2019

View reviewed changes

Trott added author ready PRs that have at least one approval, no outstanding review comments, and a CI started. net Issues and PRs related to the net subsystem. stream Issues and PRs related to the stream subsystem. labels May 30, 2019

cjihrig approved these changes May 30, 2019

View reviewed changes

jasnell approved these changes May 31, 2019

View reviewed changes

mcollina approved these changes May 31, 2019

View reviewed changes

Trott removed the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label May 31, 2019

Trott force-pushed the finished branch from b4d1a4b to d63d4b8 Compare May 31, 2019 19:26

Trott changed the title net: replace _finished with finished() net: replace _writableState with finished() May 31, 2019

ronag reviewed Jun 1, 2019

View reviewed changes

lpinca mentioned this pull request Jun 2, 2019

http2: use getter replace use directly _witableState.finished #28007

Closed

4 tasks

Trott added the wip Issues and PRs that are still a work in progress. label Jun 2, 2019

Trott closed this Jun 25, 2019

Trott force-pushed the finished branch from d63d4b8 to f11a4ec Compare June 25, 2019 21:51

Trott removed the wip Issues and PRs that are still a work in progress. label Jun 25, 2019

Trott reopened this Jun 25, 2019

addaleax approved these changes Jun 25, 2019

View reviewed changes

Trott added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Jun 26, 2019

Trott requested review from addaleax, benjamingr, cjihrig, jasnell, lpinca and mcollina June 26, 2019 01:59

Trott changed the title net: replace _writableState with finished() net: replace _writableState with writableFinished Jun 26, 2019

cjihrig approved these changes Jun 26, 2019

View reviewed changes

lpinca approved these changes Jun 26, 2019

View reviewed changes

Trott closed this Jun 26, 2019

Trott force-pushed the finished branch from 17967b7 to 3832713 Compare June 26, 2019 15:15

Trott deleted the finished branch June 26, 2019 15:15

Trott merged commit 3832713 into nodejs:master Jun 26, 2019

targos mentioned this pull request Jul 2, 2019

Release v12.6.0 proposal #28508

Merged

This was referenced Jul 3, 2019

2019-07-03 Version 12.6.0 (Current) @targos beevelop/docker-nodejs#288

Closed

2019-07-03 Version 12.6.0 (Current) @targos beevelop/docker-nodejs#289

Closed

2019-07-03 Version 12.6.0 (Current) @targos beevelop/docker-nodejs#290

Closed

net: replace _writableState with writableFinished by Trott · Pull Request #27974 · nodejs/node · GitHub

Uh oh!

Uh oh!

net: replace _writableState with writableFinished - #27974

net: replace _writableState with writableFinished#27974Trott merged 0 commit intonodejs:masternodejs/node:masterfrom Trott:finishedTrott/io.js:finishedCopy head branch name to clipboard

Conversation

Trott commented May 30, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

Trott commented May 30, 2019

Uh oh!

This comment has been minimized.

nodejs-github-bot commented May 30, 2019

Uh oh!

nodejs-github-bot commented May 30, 2019

Uh oh!

lpinca commented May 30, 2019

Uh oh!

lpinca commented May 31, 2019

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

Trott commented May 31, 2019

Uh oh!

lpinca commented Jun 1, 2019

Uh oh!

Trott commented Jun 1, 2019

Uh oh!

Trott commented Jun 1, 2019

Uh oh!

ronag Jun 1, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lpinca Jun 1, 2019

Choose a reason for hiding this comment

Uh oh!

mcollina commented Jun 1, 2019

Uh oh!

lpinca commented Jun 1, 2019

Uh oh!

Trott commented Jun 2, 2019

Uh oh!

nodejs-github-bot commented Jun 25, 2019

Uh oh!

Trott commented Jun 25, 2019

Uh oh!

nodejs-github-bot commented Jun 26, 2019

Uh oh!

Trott commented Jun 26, 2019

Uh oh!

Trott commented Jun 26, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

net: replace _writableState with writableFinished#27974
Trott merged 0 commit into
nodejs:masterfrom
Trott:finished

Trott commented May 30, 2019 •
edited
Loading

ronag Jun 1, 2019 •
edited
Loading