net: replace _writableState with writableFinished

[Trott]

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

[Trott]

/ping @mcollina

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[lpinca]

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]

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

[mcollina]

LGTM

[Trott]

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?

[lpinca]

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

[Trott]

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

[Trott]

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

[ronag]

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

[lpinca]

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

[mcollina]

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.

[lpinca]

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.

[Trott]

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.

[nodejs-github-bot]

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

[Trott]

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

[nodejs-github-bot]

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

[Trott]

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

[Trott]

Landed in 3832713