FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

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

/ node Public

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

Closed
addaleax wants to merge 2 commits into
nodejs:masterfrom
addaleax:net-simplify-end
ConversationCommits (2)ChecksFiles changed
Closed

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

Conversation

addaleax commented Feb 11, 2018

Copy link
Copy Markdown
Member

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

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

BridgeAR commented Feb 11, 2018

Copy link
Copy Markdown
Member

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

BridgeAR added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Feb 11, 2018
jasnell requested a review from mcollina February 13, 2018 14:31
mcollina reviewed Feb 13, 2018
View reviewed changes

mcollina left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

Comment thread lib/net.js Outdated
@@ -524,16 +524,8 @@ Socket.prototype._read = function(n) {

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

mcollina Feb 13, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

addaleax Feb 13, 2018

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

yup, done!

Comment thread lib/net.js
stream.Duplex.prototype.end.call(this, data, encoding);
this.writable = false;
DTRACE_NET_STREAM_END(this);
LTTNG_NET_STREAM_END(this);

mcollina Feb 13, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

addaleax Feb 13, 2018

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

Comment thread lib/net.js
else
maybeDestroy(this);

return this;

mcollina Feb 13, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

mcollina Feb 13, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

mcollina approved these changes Feb 13, 2018
View reviewed changes

mcollina left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM

BridgeAR commented Feb 16, 2018

Copy link
Copy Markdown
Member

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

BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Feb 17, 2018
net: simplify net.Socket#end()
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: 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 commented Feb 17, 2018

Copy link
Copy Markdown
Member

Landed in 590eace 🎉

BridgeAR closed this Feb 17, 2018
MylesBorins pushed a commit that referenced this pull request Feb 21, 2018
net: simplify net.Socket#end()
144be3d 
`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>
MylesBorins mentioned this pull request Feb 21, 2018
Merged
MylesBorins pushed a commit that referenced this pull request Feb 21, 2018
net: simplify net.Socket#end()
f54659c 
`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>
MylesBorins pushed a commit that referenced this pull request Feb 21, 2018
net: simplify net.Socket#end()
b9ea4c4 
`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>
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
net: simplify net.Socket#end()
97bbd39 
`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>
MylesBorins pushed a commit that referenced this pull request Aug 7, 2018
net: simplify net.Socket#end()
29f358f 
`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>
MylesBorins pushed a commit that referenced this pull request Aug 7, 2018
net: simplify net.Socket#end()
6c7df32 
`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>
MylesBorins pushed a commit that referenced this pull request Aug 9, 2018
net: simplify net.Socket#end()
4ad924f 
`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>
MylesBorins mentioned this pull request Aug 16, 2018
Merged
MylesBorins pushed a commit that referenced this pull request Aug 16, 2018
net: simplify net.Socket#end()
4a267f0 
`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>
Ajido mentioned this pull request Oct 28, 2018
Closed
2 tasks
vsemozhetbyt pushed a commit that referenced this pull request Oct 29, 2018
doc: add optional callback to socket.end()
9697c08 
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>
targos pushed a commit that referenced this pull request Nov 1, 2018
doc: add optional callback to socket.end()
f01a806 
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>
MylesBorins pushed a commit that referenced this pull request Nov 26, 2018
doc: add optional callback to socket.end()
64c683b 
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>
rvagg pushed a commit that referenced this pull request Nov 28, 2018
doc: add optional callback to socket.end()
e14226c 
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>
MylesBorins pushed a commit that referenced this pull request Nov 29, 2018
doc: add optional callback to socket.end()
cc22a2a 
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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

mcollina mcollina approved these changes
jasnell jasnell approved these changes
lpinca lpinca approved these changes
cjihrig cjihrig approved these changes
BridgeAR BridgeAR approved these changes

Assignees

No one assigned

Labels

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.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

Back | FazBrowse Home | New Git URL