★ wanayoo — archive 1999 https://github.com/nodejs/node/pull/18608Nouvelle 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

net: use `_final` instead of `on('finish')` #18608

Closed
wants to merge 3 commits into from

Conversation

6 participants
@addaleax
Copy link
Member

addaleax commented Feb 7, 2018

Shutting down the connection is what _final is there for.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

net

@lpinca

lpinca approved these changes Feb 7, 2018

lib/net.js Outdated
return self._handle.shutdown(req);
}

// the user has called .end(), and all the bytes have been
// sent out to the other side.
function onSocketFinish() {
Socket.prototype._final = function(cb) {
// If still connecting - defer handling 'finish' until 'connect' will happen

This comment has been minimized.

@lpinca

lpinca Feb 7, 2018

Member

Nit: I'd remove handling 'finish' as we are no longer handling the 'finish' event.

This comment has been minimized.

@addaleax

addaleax Feb 7, 2018

Author Member

@lpinca done! And thanks for the reviews, it’s really helpful to talk this through with you!

@addaleax addaleax referenced this pull request Feb 7, 2018

Closed

net: inline and simplify onSocketEnd #18607

2 of 2 tasks complete
@jasnell

jasnell approved these changes Feb 9, 2018

@BridgeAR

This comment has been minimized.

Copy link
Member

BridgeAR commented Feb 10, 2018

@addaleax please always trigger a CI after opening a PR :-)

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

@addaleax

This comment has been minimized.

Copy link
Member Author

addaleax commented Feb 10, 2018

please always trigger a CI after opening a PR :-)

I usually wait until the first review or so, since the PR likely needs to be updated after it anyway.

@addaleax

This comment has been minimized.

@BridgeAR BridgeAR requested a review from mcollina Feb 17, 2018

@mcollina
Copy link
Member

mcollina left a comment

Good work!

@@ -55,6 +55,8 @@ function undestroy() {
this._writableState.destroyed = false;
this._writableState.ended = false;
this._writableState.ending = false;
this._writableState.finalCalled = false;
this._writableState.prefinished = false;

This comment has been minimized.

@mcollina

mcollina Feb 17, 2018

Member

can you add a unit test for those? Maybe also place them in a separate commit, if we want to backport them separately.

This comment has been minimized.

@addaleax

addaleax Feb 19, 2018

Author Member

@mcollina Do you know where those tests are? In any case, these lines are tested in the sense that tests do fail without them...

This comment has been minimized.

@mcollina

mcollina Feb 20, 2018

Member

We need test that can be run as part of readable-stream.

Here are the current tests:

test/parallel/test-stream-duplex-destroy.js
test/parallel/test-stream-readable-destroy.js
test/parallel/test-stream-transform-destroy.js
test/parallel/test-stream-writable-destroy.js

@BridgeAR BridgeAR removed the author ready label Feb 18, 2018

addaleax added some commits Feb 7, 2018

net: use `_final` instead of `on('finish')`
Shutting down the connection is what `_final` is there for.

@addaleax addaleax force-pushed the addaleax:net-final branch from 461d558 to 356ac44 Feb 20, 2018

@addaleax

This comment has been minimized.

Copy link
Member Author

addaleax commented Feb 20, 2018

@mcollina
Copy link
Member

mcollina left a comment

LGTM

@addaleax

This comment has been minimized.

Copy link
Member Author

addaleax commented Feb 21, 2018

Landed in 906bbef

@addaleax addaleax closed this Feb 21, 2018

@addaleax addaleax deleted the addaleax:net-final branch Feb 21, 2018

@addaleax addaleax removed the author ready label Feb 21, 2018

@lpinca

This comment has been minimized.

Copy link
Member

lpinca commented Feb 21, 2018

@addaleax it seems this landed without metadata.

@addaleax

This comment has been minimized.

Copy link
Member Author

addaleax commented Feb 21, 2018

@lpinca I’ve force-pushed that mistake away, thanks for pointing it out.

addaleax added a commit that referenced this pull request Feb 21, 2018

net: use `_final` instead of `on('finish')`
Shutting down the connection is what `_final` is there for.

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

addaleax added a commit to addaleax/node that referenced this pull request Feb 26, 2018

net: use `_final` instead of `on('finish')`
Shutting down the connection is what `_final` is there for.

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

addaleax added a commit to addaleax/node that referenced this pull request Feb 27, 2018

net: use `_final` instead of `on('finish')`
Shutting down the connection is what `_final` is there for.

PR-URL: nodejs#18608
Reviewed-By: Luigi Pinca <luigipinca@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

BridgeAR added a commit to BridgeAR/node that referenced this pull request May 1, 2018

net: use `_final` instead of `on('finish')`
Shutting down the connection is what `_final` is there for.

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

MayaLekova added a commit to MayaLekova/node that referenced this pull request May 8, 2018

net: use `_final` instead of `on('finish')`
Shutting down the connection is what `_final` is there for.

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

addaleax added a commit that referenced this pull request Jun 29, 2018

net: use `_final` instead of `on('finish')`
Shutting down the connection is what `_final` is there for.

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

@MylesBorins MylesBorins referenced this pull request Jul 9, 2018

Merged

v8.12.0 proposal #21593

rvagg added a commit that referenced this pull request Aug 16, 2018

net: use `_final` instead of `on('finish')`
Shutting down the connection is what `_final` is there for.

PR-URL: #18608
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.