net: do not inherit the no-half-open enforcer#18974

lpinca · 2018-02-24T08:54:24Z

Socket.prototype.destroySoon() is called as soon as UV_EOF is read
if the allowHalfOpen option is disabled. This already works as a
"no-half-open enforcer" so there is no need to inherit another from
stream.Duplex.

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)

net

lpinca · 2018-02-24T08:56:36Z

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

lpinca · 2018-03-04T13:39:43Z

Ping @nodejs/collaborators.

lpinca · 2018-03-04T13:47:13Z

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

addaleax · 2018-03-04T16:23:39Z

lib/net.js

I am not really a fan of this … not calling the Duplex constructor seems like a bad idea in general, and it’s going to be an extra obstacle while working on merging all StreamBase JS wrappers…
Can we instead make the Duplex constructor not attach the event handler if options.allowHalfOpen is set? That would help all other Duplex streams as well…

Yes, after #18953 my idea was to override the allowHalfOpen option when calling the Duplex constructor in order to never inherit the listener but to do that the options object has to be copied. This was done for performance reasons.

I'm open to suggestions.

@lpinca Yeah, good point.
I don’t think we can get around that without making some kind of change to the Duplex API, but I would be okay with that.
The best I can come up with right now would be this:
Add
Duplex.kHasHalfOpenEnforcer=Symbol('kHasHalfOpenEnforcer');// or non-enumerableDuplex.prototype[Duplex.kHasHalfOpenEnforcer]=true;net.Socket.prototype[Duplex.kHasHalfOpenEnforcer]=false;
Check for that property in the Duplex constructor
Another approach might be to create an internal Duplex constructor that omits the listener addition, which could serve as a base class of the “real”, publicly exposed Duplex stream…

I'm not very happy with option 1, adapting the base class to the needs of the child seems kinda hackish. Will think a bit about the second approach.

addaleax · 2018-03-04T16:28:28Z

lib/net.js

… and, as I understand it, would allow us to get rid of this, if we just transform allowHalfOpen correctly to begin with?
Also, I don’t know what the motivation for this was, but I don’t get why the default is not allowing half-open connections.

This is unrelated but options is always an object so there is no reason to see if it's truthy.

benjamingr · 2018-03-05T08:05:35Z

I'm fine with either the solution here or the one addaleax suggested.

lpinca · 2018-03-05T10:18:25Z

@addaleax @benjamingr PTAL when you have some time. I will reorganize commits if this is acceptable.

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

mcollina · 2018-03-05T10:25:33Z

I'm -1 on the overall approach. Why cannot it be implemented only in Duplex, and the duplicating code removed from Socket? We should strive for that ideally.

If we go with this approach, we must move all the defined properties from Duplex to DuplexBase, or re-implement them for Socket if that implementation would be different.

This goes against the generic intent of reducing how we rely on streams internals.

lpinca · 2018-03-05T10:41:59Z

@mcollina read discussion in #18974 (comment)

net.Socket has its own "no-half-open" enforcer so we should not inherit it from Duplex even when the allowHalfOpen option is false. It should never be inherited as it's only useless overhead.

To do that we can set the allowHalfOpen option to true before calling the Duplex constructor but the options object must be copied. That degrades performance.

Workarounds:

Original approach: dc18709
@addaleax's suggestions: net: do not inherit the no-half-open enforcer #18974 (comment)

As said I'm open to suggestions.

addaleax · 2018-03-05T10:53:29Z

@lpinca Matteo has a point … what’s the downside of removing the extra code in net.Socket and just using what Duplex provides?

lpinca · 2018-03-05T11:33:35Z

That means doing the work twice, no? First to add the listener in the Duplex constructor and then to remove it unconditionally in the net.Socket constructor. I would rather copy the options object in that case.

addaleax · 2018-03-05T11:35:02Z

@lpinca If we want to use the Duplex mechanism, why would we remove the listener in the net.Socket destructor?

lpinca · 2018-03-05T11:39:07Z

Assume that we instantiate the net.Socket with the allowHalfOpen option set to false.

constsocket=newSocket({allowHalfOpen: false});

The Duplex constructor will add the no-half-open enforcer but we don't need it as the Socket already has its own no-half-open enforcer so we have to remove it in the Socket constructor.

lpinca · 2018-03-05T11:44:46Z

Removing the net.Socket no-half-open enforcer in favor of the one provided by Duplex is not an option as they don't work in same way.

The no-half-open enforcer in Duplex calls end() on next tick
It doesn't call Socket.prototype.destroySoon().

benjamingr

Still LGTM and nicer now.

mcollina · 2018-03-05T13:43:22Z

The destroySoon() method uses this._writableState.finishedhttps://github.com/nodejs/node/blob/master/lib/net.js#L535. We should aim to remove that (maybe even moving destroySoon() under streams?).

I'd be actually 👍 in improving the allowHalfOpen behavior in the Duplex so that this could be supported. I do not see a reason to have allowHalfOpen in Duplex at all if our own Socket implementation (the quintessence of a Duplex) cannot use it.

mcollina

Making my -1 explicit and flagging it at as semvermajor, because socket instanceof Duplex would now return false.

addaleax · 2018-03-05T13:51:03Z

because socket instanceof Duplex would now return false.

@mcollina That’s incorrect, it does return true.

mcollina · 2018-03-05T14:18:18Z

lib/net.js

Can you please add a note along the following lines: Socket is still inheriting from Duplex, but we are just using a slimmed down constructor. socket instanceOf Duplex would keep returning true

@mcollina updated, PTAL.

mcollina

LGTM with a nit.

BridgeAR · 2018-03-06T19:29:13Z

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

Add ability to subclass `stream.Duplex` without inheriting the "no-half-open enforcer" regardless of the value of the `allowHalfOpen` option.

`Socket.prototype.destroySoon()` is called as soon as `UV_EOF` is read if the `allowHalfOpen` option is disabled. This already works as a "no-half-open enforcer" so there is no need to inherit another from `stream.Duplex`.

lpinca · 2018-03-07T10:14:32Z

Rebased and added a new test.
New CI: https://ci.nodejs.org/job/node-test-pull-request/13564/

mcollina

LGTM

Add ability to subclass `stream.Duplex` without inheriting the "no-half-open enforcer" regardless of the value of the `allowHalfOpen` option. PR-URL: #18974 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Chen Gang <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

`Socket.prototype.destroySoon()` is called as soon as `UV_EOF` is read if the `allowHalfOpen` option is disabled. This already works as a "no-half-open enforcer" so there is no need to inherit another from `stream.Duplex`. PR-URL: #18974 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Chen Gang <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

lpinca · 2018-03-07T15:06:49Z

Landed in 42e9b48...4e86f9b.

Add ability to subclass `stream.Duplex` without inheriting the "no-half-open enforcer" regardless of the value of the `allowHalfOpen` option. PR-URL: #18974 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Chen Gang <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

`Socket.prototype.destroySoon()` is called as soon as `UV_EOF` is read if the `allowHalfOpen` option is disabled. This already works as a "no-half-open enforcer" so there is no need to inherit another from `stream.Duplex`. PR-URL: #18974 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Chen Gang <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

Add ability to subclass `stream.Duplex` without inheriting the "no-half-open enforcer" regardless of the value of the `allowHalfOpen` option. PR-URL: #18974 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Chen Gang <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

`Socket.prototype.destroySoon()` is called as soon as `UV_EOF` is read if the `allowHalfOpen` option is disabled. This already works as a "no-half-open enforcer" so there is no need to inherit another from `stream.Duplex`. PR-URL: #18974 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Chen Gang <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

Add ability to subclass `stream.Duplex` without inheriting the "no-half-open enforcer" regardless of the value of the `allowHalfOpen` option. PR-URL: nodejs#18974 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Chen Gang <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

`Socket.prototype.destroySoon()` is called as soon as `UV_EOF` is read if the `allowHalfOpen` option is disabled. This already works as a "no-half-open enforcer" so there is no need to inherit another from `stream.Duplex`. PR-URL: nodejs#18974 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Chen Gang <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

jasnell · 2018-08-17T23:18:47Z

Should this be backported to 8.x? If so, a separate backport PR is needed

lpinca · 2018-08-18T05:29:17Z

I would skip this and eventually backport #19779 which is basically a revert of this one, but #19779 depends on #18953. Let me know.

nodejs-github-bot added the net Issues and PRs related to the net subsystem. label Feb 24, 2018

benjamingr approved these changes Mar 4, 2018
View reviewed changes

lpinca added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 4, 2018

addaleax reviewed Mar 4, 2018
View reviewed changes

addaleax removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 4, 2018

lpinca force-pushed the avoid/inheriting-onend-listener branch from 1d5513d to a1c475dCompare March 5, 2018 10:15

addaleax approved these changes Mar 5, 2018
View reviewed changes

benjamingr approved these changes Mar 5, 2018
View reviewed changes

mcollina added the semver-major PRs that contain breaking changes and should be released in the next major version. label Mar 5, 2018

mcollina requested changes Mar 5, 2018
View reviewed changes

mcollina reviewed Mar 5, 2018
View reviewed changes

mcollina approved these changes Mar 5, 2018
View reviewed changes

lpinca removed the semver-major PRs that contain breaking changes and should be released in the next major version. label Mar 5, 2018

lpinca force-pushed the avoid/inheriting-onend-listener branch from a1c475d to a40a803Compare March 5, 2018 15:48

jasnell approved these changes Mar 5, 2018
View reviewed changes

MoonBall approved these changes Mar 5, 2018
View reviewed changes

lpinca force-pushed the avoid/inheriting-onend-listener branch from a40a803 to e530c3fCompare March 5, 2018 20:14

BridgeAR approved these changes Mar 6, 2018
View reviewed changes

BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 6, 2018

lpinca added 2 commits March 7, 2018 10:33

stream: make Duplex inherits from DuplexBase
dc3969e
Add ability to subclass `stream.Duplex` without inheriting the "no-half-open enforcer" regardless of the value of the `allowHalfOpen` option.

net: do not inherit the no-half-open enforcer
5b73a79
`Socket.prototype.destroySoon()` is called as soon as `UV_EOF` is read if the `allowHalfOpen` option is disabled. This already works as a "no-half-open enforcer" so there is no need to inherit another from `stream.Duplex`.

lpinca force-pushed the avoid/inheriting-onend-listener branch from e530c3f to 5b73a79Compare March 7, 2018 10:12

mcollina approved these changes Mar 7, 2018
View reviewed changes

lpinca closed this Mar 7, 2018

lpinca deleted the avoid/inheriting-onend-listener branch March 7, 2018 15:07

targos removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 17, 2018

targos mentioned this pull request Mar 18, 2018
v9.9.0 proposal #19428
Merged

lpinca mentioned this pull request Apr 3, 2018
stream: change in stdout/stderr events from child process (missing finish) #19764
Closed

jasnell added the backport-requested-v8.x label Aug 17, 2018

Uh oh!

net: do not inherit the no-half-open enforcer#18974

net: do not inherit the no-half-open enforcer #18974

Uh oh!

Conversation

lpinca commented Feb 24, 2018

Checklist

Affected core subsystem(s)

Uh oh!

lpinca commented Feb 24, 2018

Uh oh!

lpinca commented Mar 4, 2018

Uh oh!

lpinca commented Mar 4, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

benjamingr commented Mar 5, 2018

Uh oh!

lpinca commented Mar 5, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mcollina commented Mar 5, 2018

Uh oh!

lpinca commented Mar 5, 2018

Uh oh!

addaleax commented Mar 5, 2018

Uh oh!

lpinca commented Mar 5, 2018

Uh oh!

addaleax commented Mar 5, 2018

Uh oh!

lpinca commented Mar 5, 2018

Uh oh!

lpinca commented Mar 5, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

benjamingr left a comment

Choose a reason for hiding this comment

Uh oh!

mcollina commented Mar 5, 2018

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

addaleax commented Mar 5, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

BridgeAR commented Mar 6, 2018

Uh oh!

lpinca commented Mar 7, 2018

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

lpinca commented Mar 7, 2018

Uh oh!

jasnell commented Aug 17, 2018

Uh oh!

lpinca commented Aug 18, 2018

lpinca commented Mar 5, 2018•
edited
Loading

lpinca commented Mar 5, 2018•
edited
Loading