net: simplify net.Socket#end() by addaleax · Pull Request #18708 · nodejs/node

addaleax · 2018-02-11T03:05:25Z

writable is already set by the streams side, and
there is a handler waiting for the writable side to finish
which already takes care of the other cleanup code that
was previously there; both of these things can therefore be removed.

Checklist

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

Affected core subsystem(s)

net

`writable` is already set by the streams side, and there is a handler waiting for the writable side to finish which already takes care of the other cleanup code that was previously there; both of these things can therefore be removed.

BridgeAR · 2018-02-11T17:40:00Z

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

mcollina

This requires a full CITGM run.
I would flag it as "backing for LTS", as I would not backport there anytime soon out of caution.

mcollina · 2018-02-13T14:42:50Z

lib/net.js

@@ -524,16 +524,8 @@ Socket.prototype._read = function(n) {

 Socket.prototype.end = function(data, encoding) {
  stream.Duplex.prototype.end.call(this, data, encoding);


While we are at it, can you add a callback to end()? that's the full signature for streams (https://nodejs.org/api/stream.html#stream_writable_end_chunk_encoding_callback).

mcollina · 2018-02-13T14:47:53Z

lib/net.js

  stream.Duplex.prototype.end.call(this, data, encoding);
-  this.writable = false;
  DTRACE_NET_STREAM_END(this);
  LTTNG_NET_STREAM_END(this);


Can we move these to _destroy instead and use a stock end implementation?

I think would be a breaking change in light of your comment about not returning this below :/ I’d agree that that would be ideal, definitely.

mcollina · 2018-02-13T14:48:37Z

lib/net.js

-  else
-    maybeDestroy(this);
-
  return this;


wow, this differs from node streams, as end does not return this. We might want to add it to streams too.

I'll send a follow up PR to this one, which will be a semver-major.

mcollina

LGTM

BridgeAR · 2018-02-16T18:11:55Z

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

`writable` is already set by the streams side, and there is a handler waiting for the writable side to finish which already takes care of the other cleanup code that was previously there; both of these things can therefore be removed. PR-URL: nodejs#18708 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

BridgeAR · 2018-02-17T13:38:02Z

Landed in 590eace 🎉

`writable` is already set by the streams side, and there is a handler waiting for the writable side to finish which already takes care of the other cleanup code that was previously there; both of these things can therefore be removed. PR-URL: #18708 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

`writable` is already set by the streams side, and there is a handler waiting for the writable side to finish which already takes care of the other cleanup code that was previously there; both of these things can therefore be removed. PR-URL: nodejs#18708 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

`writable` is already set by the streams side, and there is a handler waiting for the writable side to finish which already takes care of the other cleanup code that was previously there; both of these things can therefore be removed. PR-URL: #18708 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

PR-URL: #23937 Refs: #18708 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>

net: simplify net.Socket#end()

4bb7b4b

`writable` is already set by the streams side, and there is a handler waiting for the writable side to finish which already takes care of the other cleanup code that was previously there; both of these things can therefore be removed.

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

lpinca approved these changes Feb 11, 2018

View reviewed changes

BridgeAR approved these changes Feb 11, 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 11, 2018

cjihrig approved these changes Feb 11, 2018

View reviewed changes

jasnell approved these changes Feb 13, 2018

View reviewed changes

jasnell requested a review from mcollina February 13, 2018 14:31

mcollina reviewed Feb 13, 2018

View reviewed changes

[squash] mcollina comment: add callback

4ba6592

mcollina approved these changes Feb 13, 2018

View reviewed changes

BridgeAR closed this Feb 17, 2018

MylesBorins mentioned this pull request Feb 21, 2018

v9.6.0 proposal #18902

Merged

MylesBorins mentioned this pull request Aug 16, 2018

v8.12.0 proposal #21593

Merged

Ajido mentioned this pull request Oct 28, 2018

doc: add optional callback to socket.end #23937

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

net: simplify net.Socket#end()#18708

net: simplify net.Socket#end()#18708
addaleax wants to merge 2 commits intonodejs:masterfrom
addaleax:net-simplify-end

addaleax commented Feb 11, 2018

Uh oh!

BridgeAR commented Feb 11, 2018

Uh oh!

mcollina left a comment

Uh oh!

mcollina Feb 13, 2018

Uh oh!

addaleax Feb 13, 2018

Uh oh!

mcollina Feb 13, 2018

Uh oh!

addaleax Feb 13, 2018

Uh oh!

mcollina Feb 13, 2018

Uh oh!

mcollina Feb 13, 2018

Uh oh!

mcollina left a comment

Uh oh!

BridgeAR commented Feb 16, 2018

Uh oh!

BridgeAR commented Feb 17, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

		@@ -524,16 +524,8 @@ Socket.prototype._read = function(n) {

		Socket.prototype.end = function(data, encoding) {
		stream.Duplex.prototype.end.call(this, data, encoding);

Uh oh!

Conversation

addaleax commented Feb 11, 2018

Checklist

Affected core subsystem(s)

Uh oh!

BridgeAR commented Feb 11, 2018

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

mcollina Feb 13, 2018

Choose a reason for hiding this comment

Uh oh!

addaleax Feb 13, 2018

Choose a reason for hiding this comment

Uh oh!

mcollina Feb 13, 2018

Choose a reason for hiding this comment

Uh oh!

addaleax Feb 13, 2018

Choose a reason for hiding this comment

Uh oh!

mcollina Feb 13, 2018

Choose a reason for hiding this comment

Uh oh!

mcollina Feb 13, 2018

Choose a reason for hiding this comment

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

BridgeAR commented Feb 16, 2018

Uh oh!

BridgeAR commented Feb 17, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants