★ wanayoo — archive 1999 https://github.com/nodejs/node/pull/18708Nouvelle recherche | Portail wanayoo
Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Closed
wants to merge 2 commits into from
Closed

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

wants to merge 2 commits into from

Conversation

addaleax
Copy link
Member

@addaleax addaleax commented Feb 11, 2018

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

@addaleax
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 nodejs-github-bot added the net label Feb 11, 2018
@BridgeAR
Copy link
Member

@BridgeAR BridgeAR commented Feb 11, 2018

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

@BridgeAR BridgeAR added the author ready label Feb 11, 2018
@jasnell jasnell requested a review from mcollina Feb 13, 2018
mcollina
mcollina reviewed Feb 13, 2018
View changes
Copy link
Member

@mcollina mcollina left a comment

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.

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);
Copy link
Member

@mcollina mcollina Feb 13, 2018

Choose a reason for hiding this comment

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

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

Copy link
Member Author

@addaleax addaleax Feb 13, 2018

Choose a reason for hiding this comment

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

yup, done!

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);
this.writable = false;
DTRACE_NET_STREAM_END(this);
LTTNG_NET_STREAM_END(this);
Copy link
Member

@mcollina mcollina Feb 13, 2018

Choose a reason for hiding this comment

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

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

Copy link
Member Author

@addaleax addaleax Feb 13, 2018

Choose a reason for hiding this comment

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

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.

lib/net.js
this.read(0);
else
maybeDestroy(this);

return this;
Copy link
Member

@mcollina mcollina Feb 13, 2018

Choose a reason for hiding this comment

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

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

Copy link
Member

@mcollina mcollina Feb 13, 2018

Choose a reason for hiding this comment

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

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

mcollina
mcollina approved these changes Feb 13, 2018
View changes
Copy link
Member

@mcollina mcollina left a comment

LGTM

@BridgeAR
Copy link
Member

@BridgeAR BridgeAR commented Feb 16, 2018

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

BridgeAR pushed a commit to BridgeAR/node that referenced this issue Feb 17, 2018
@addaleax @BridgeAR
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
Copy link
Member

@BridgeAR BridgeAR commented Feb 17, 2018

Landed in 590eace 🎉

@BridgeAR BridgeAR closed this Feb 17, 2018
MylesBorins pushed a commit that referenced this issue Feb 21, 2018
@addaleax @MylesBorins
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 MylesBorins mentioned this pull request Feb 21, 2018
Merged
MylesBorins pushed a commit that referenced this issue Feb 21, 2018
@addaleax @MylesBorins
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 issue Feb 21, 2018
@addaleax @MylesBorins
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>
@addaleax addaleax added this to To do in StreamBase refactor via automation Apr 1, 2018
@addaleax addaleax moved this from To do to Done in StreamBase refactor Apr 1, 2018
MayaLekova pushed a commit to MayaLekova/node that referenced this issue May 8, 2018
@addaleax @MayaLekova
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 issue Aug 7, 2018
@addaleax @MylesBorins
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 issue Aug 7, 2018
@addaleax @MylesBorins
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 issue Aug 9, 2018
@addaleax @MylesBorins
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 MylesBorins mentioned this pull request Aug 16, 2018
Merged
MylesBorins pushed a commit that referenced this issue Aug 16, 2018
@addaleax @MylesBorins
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 Ajido mentioned this pull request Oct 28, 2018
Closed
2 tasks
vsemozhetbyt pushed a commit that referenced this issue Oct 29, 2018
@Ajido @vsemozhetbyt
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 issue Nov 1, 2018
@Ajido @targos
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 issue Nov 26, 2018
@Ajido @MylesBorins
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 issue Nov 28, 2018
@Ajido @rvagg
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 issue Nov 29, 2018
@Ajido @MylesBorins
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mcollina mcollina

@jasnell jasnell

@lpinca lpinca

@cjihrig cjihrig

@BridgeAR BridgeAR

Assignees
No one assigned
Labels
author ready net
Projects
No open projects
StreamBase refactor
  
Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@addaleax @BridgeAR @mcollina @jasnell @lpinca @cjihrig @nodejs-github-bot