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

addaleax
Copy link
Member

@addaleax 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

@nodejs-github-bot nodejs-github-bot added the net Issues and PRs related to the net subsystem. label 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
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

@BridgeAR
Copy link
Member

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

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

@addaleax
Copy link
Member Author

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
Copy link
Member Author

addaleax commented Feb 11, 2018

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

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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 PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 18, 2018
@addaleax
Copy link
Member Author

@mcollina Thanks for the pointer, done!

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

@addaleax 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
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

LGTM

@addaleax
Copy link
Member Author

Landed in 906bbef

@addaleax addaleax closed this Feb 21, 2018
@addaleax addaleax deleted the net-final branch February 21, 2018 19:28
@addaleax addaleax removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 21, 2018
@lpinca
Copy link
Member

lpinca commented Feb 21, 2018

@addaleax it seems this landed without metadata.

@addaleax
Copy link
Member Author

@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
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
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>
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request May 1, 2018
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 pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
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
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 mentioned this pull request Jul 9, 2018
rvagg pushed a commit that referenced this pull request Aug 16, 2018
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
Labels
net Issues and PRs related to the net subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants