net: inline and simplify onSocketEnd #18607

addaleax · 2018-02-06T23:55:51Z

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines

Affected core subsystem(s)

net

addaleax · 2018-02-06T23:56:51Z

CI: https://ci.nodejs.org/job/node-test-commit/15999/
CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1258/

lpinca

SGTM

lpinca · 2018-02-07T09:42:47Z

lib/net.js

@@ -667,6 +640,16 @@ function onread(nread, buffer) {
    maybeDestroy(self);


I wonder if this if statement can be removed. readable is automatically set to false when 'end' is emitted and maybeDestroy() is called by the handler below.

Yeah, but end hasn’t necessarily been emitted here. But you’re right, the handler below was supposed to be in the else block for this.

Yes but if I'm not wrong 'end' will not be emitted on the same tick, so why aren't we only relying on the 'end' event? Is it to avoid attaching a listener?

Is it to avoid attaching a listener?

Yes, I assume that’s it. Maybe it isn’t that important?

I don't know, I don't think it will make a big difference but there is also no real advantage in removing the "fast case" so let's keep it as is :)

FWIW we are now emitting '_socketEnd' and attaching the listener all the time as 'end' is not emitted on the same tick.

I also think this branch can create a race where 'close' can be emitted before 'end' as _handle.close() is called before the 'end' event is emitted if maybeDestroy() succeeds.

TL;DR I think it's better to always rely on the 'end' event.

TL;DR I think it's better to always rely on the 'end' event.

I am cool with that if we can agree that the fast-pathing is not worth it … I like the simplicity :)

Updated!

lpinca · 2018-02-07T09:44:15Z

lib/net.js

+    self.write = writeAfterFIN;
+    self.destroySoon();
+  }
+
  // internal end event so that we know that the actual socket
  // is no longer readable, and we can start the shutdown
  // procedure. No need to wait for all the data to be consumed.


Can we remove this event ('_socketEnd') or is it kept to avoid possible breakage?

@lpinca It’s still used in another location … I think after #18608 things could be refactored that way, yes

mcollina

LGTM with CI and CITGM green

lpinca · 2018-02-08T07:56:15Z

lib/net.js

@@ -664,7 +644,13 @@ function onread(nread, buffer) {

  if (self.readableLength === 0) {
    self.readable = false;


Sorry to pester but this should be no longer needed now. readableLength is 0 so 'end' will be emitted on the next tick and it will already set readable to false.

It's also possible that the 'end' event will never be emitted, for example if onread() returns an error before reaching this point, so I wonder if it's better to only add the 'end' listener here instead of the constructor.

this should be no longer needed now

Hm, I could have sworn there were test failures when I tired that… anyway, it seems to pass now, so: Done!

It's also possible that the 'end' event will never be emitted, for example if onread() returns an error before reaching this point, so I wonder if it's better to only add the 'end' listener here instead of the constructor.

That’s not really the common case, is it? I don’t think I have a strong preference.

Indeed it isn't, I'm perfectly fine with this as is. Thanks for bearing with me.

addaleax · 2018-02-10T19:50:32Z

CI: https://ci.nodejs.org/job/node-test-commit/16137/

BridgeAR · 2018-02-10T19:53:41Z

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

BridgeAR · 2018-02-16T17:42:22Z

lib/net.js

    self.readable = false;
-    maybeDestroy(self);
+  }*/


Can you please remove the commented code?

addaleax · 2018-02-20T23:12:37Z

@BridgeAR done!

CI: https://ci.nodejs.org/job/node-test-commit/16394/

addaleax · 2018-02-21T19:51:07Z

Rebased with a minor conflict

New CI: https://ci.nodejs.org/job/node-test-commit/16422/

BridgeAR · 2018-02-22T10:11:11Z

Landed in 281d00e 🎉

PR-URL: nodejs#18607 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

MylesBorins · 2018-03-06T20:50:42Z

Should this be backported to v9.x-staging? If yes please follow the guide and raise a backport PR, if not let me know or add the dont-land-on label.

PR-URL: nodejs#18607 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Backport-PR-URL: #19194 PR-URL: #18607 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

PR-URL: nodejs#18607 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

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

lpinca approved these changes Feb 7, 2018

View reviewed changes

mcollina approved these changes Feb 7, 2018

View reviewed changes

lpinca reviewed Feb 8, 2018

View reviewed changes

addaleax force-pushed the net-simplify-onsocketend branch from e2319db to d689778 Compare February 9, 2018 15:34

jasnell approved these changes Feb 9, 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 Feb 10, 2018

BridgeAR reviewed Feb 16, 2018

View reviewed changes

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

addaleax force-pushed the net-simplify-onsocketend branch from 283eb9d to ea9254e Compare February 20, 2018 23:07

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

addaleax force-pushed the net-simplify-onsocketend branch 3 times, most recently from ad319df to ddefbbc Compare February 21, 2018 19:36

addaleax force-pushed the master branch from 906bbef to 03ddd13 Compare February 21, 2018 19:43

net: inline and simplify onSocketEnd

ec1a014

addaleax force-pushed the net-simplify-onsocketend branch from ddefbbc to ec1a014 Compare February 21, 2018 19:44

BridgeAR closed this Feb 22, 2018

addaleax deleted the net-simplify-onsocketend branch February 22, 2018 17:12

MylesBorins added the backport-requested-v9.x label Mar 6, 2018

lpinca mentioned this pull request Mar 7, 2018

[v9.x backport] backport 18607, 18953 #19194

Closed

MylesBorins mentioned this pull request Mar 7, 2018

v9.8.0 proposal #19181

Merged

addaleax added dont-land-on-v8.x and removed backport-requested-v9.x labels Jun 29, 2018

addaleax mentioned this pull request Nov 23, 2018

'close' listener stop working with socket.removeAllListeners('end') #24577

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

net: inline and simplify onSocketEnd #18607

net: inline and simplify onSocketEnd #18607

addaleax commented Feb 6, 2018

addaleax commented Feb 6, 2018

lpinca left a comment

lpinca Feb 7, 2018

addaleax Feb 7, 2018

lpinca Feb 7, 2018 •

edited

Loading

addaleax Feb 7, 2018

lpinca Feb 7, 2018

lpinca Feb 7, 2018

addaleax Feb 7, 2018

lpinca Feb 7, 2018

addaleax Feb 7, 2018

lpinca Feb 7, 2018

mcollina left a comment

lpinca Feb 8, 2018 •

edited

Loading

addaleax Feb 9, 2018

lpinca Feb 9, 2018 •

edited

Loading

addaleax commented Feb 10, 2018

BridgeAR commented Feb 10, 2018

BridgeAR Feb 16, 2018

addaleax commented Feb 20, 2018

addaleax commented Feb 21, 2018

BridgeAR commented Feb 22, 2018

MylesBorins commented Mar 6, 2018

		@@ -667,6 +640,16 @@ function onread(nread, buffer) {
		maybeDestroy(self);

		@@ -664,7 +644,13 @@ function onread(nread, buffer) {

		if (self.readableLength === 0) {
		self.readable = false;

net: inline and simplify onSocketEnd #18607

net: inline and simplify onSocketEnd #18607

Conversation

addaleax commented Feb 6, 2018

Checklist

Affected core subsystem(s)

addaleax commented Feb 6, 2018

lpinca left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lpinca Feb 7, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcollina left a comment

Choose a reason for hiding this comment

lpinca Feb 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lpinca Feb 9, 2018 • edited Loading

Choose a reason for hiding this comment

addaleax commented Feb 10, 2018

BridgeAR commented Feb 10, 2018

Choose a reason for hiding this comment

addaleax commented Feb 20, 2018

addaleax commented Feb 21, 2018

BridgeAR commented Feb 22, 2018

MylesBorins commented Mar 6, 2018

lpinca Feb 7, 2018 •

edited

Loading

lpinca Feb 8, 2018 •

edited

Loading

lpinca Feb 9, 2018 •

edited

Loading