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

net: use `_final` instead of `on('finish')` by addaleax · Pull Request #18608 · nodejs/node · GitHub

/ node Public

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

Closed
addaleax wants to merge 3 commits into
nodejs:masterfrom
addaleax:net-final
ConversationCommits (3)ChecksFiles changed
Closed

net: use _final instead of on('finish')#18608
addaleax wants to merge 3 commits into
nodejs:masterfrom
addaleax:net-final

Conversation

addaleax commented Feb 7, 2018

Copy link
Copy Markdown
Member

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 added the net Issues and PRs related to the net subsystem. label Feb 7, 2018
lpinca approved these changes Feb 7, 2018
View reviewed changes
Comment thread lib/net.js Outdated

lpinca Feb 7, 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

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

addaleax Feb 7, 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

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

addaleax mentioned this pull request Feb 7, 2018
Closed
2 tasks

BridgeAR commented Feb 10, 2018

Copy link
Copy Markdown
Member

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

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

addaleax commented Feb 10, 2018

Copy link
Copy Markdown
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 commented Feb 11, 2018 •
edited
Loading

Copy link
Copy Markdown
Member Author

New CI: https://ci.nodejs.org/job/node-test-commit/16161/
New CI: https://ci.nodejs.org/job/node-test-commit/16301/

addaleax added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Feb 16, 2018
BridgeAR requested a review from mcollina February 17, 2018 13:57
mcollina reviewed Feb 17, 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

Good work!

Comment thread lib/internal/streams/destroy.js Outdated

mcollina Feb 17, 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 you add a unit test for those? Maybe also place them in a separate commit, if we want to backport them separately.

addaleax Feb 19, 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

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

mcollina Feb 20, 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

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 removed the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Feb 18, 2018

addaleax commented Feb 20, 2018

Copy link
Copy Markdown
Member Author

@mcollina Thanks for the pointer, done!

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

addaleax added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Feb 20, 2018
mcollina approved these changes Feb 21, 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

addaleax commented Feb 21, 2018

Copy link
Copy Markdown
Member Author

Landed in 906bbef

addaleax closed this Feb 21, 2018
addaleax deleted the net-final branch February 21, 2018 19:28
addaleax removed the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Feb 21, 2018

lpinca commented Feb 21, 2018

Copy link
Copy Markdown
Member

@addaleax it seems this landed without metadata.

addaleax commented Feb 21, 2018

Copy link
Copy Markdown
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
net: use _final instead of on('finish')
03ddd13 
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')
ea86656 
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
net: use _final instead of on('finish')
a4d4248 
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
net: use _final instead of on('finish')
1f77b39 
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')
bf41a41 
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 mentioned this pull request Jul 9, 2018
Merged
rvagg pushed a commit that referenced this pull request Aug 16, 2018
net: use _final instead of on('finish')
3d38bab 
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>
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

Assignees

No one assigned

Labels

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.

6 participants

Back | FazBrowse Home | New Git URL