Skip to content

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

Closed
addaleax wants to merge 3 commits into
nodejs:masterfrom
addaleax:net-final
Closed

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

Conversation

@addaleax

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

@lpincalpincaFeb 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.

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

@addaleaxaddaleaxFeb 7, 2018

Copy link
Copy Markdown
MemberAuthor

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!

@addaleaxaddaleax mentioned this pull request Feb 7, 2018
Closed
2 tasks
@BridgeAR

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

addaleax commented Feb 10, 2018

Copy link
Copy Markdown
MemberAuthor

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

addaleax commented Feb 11, 2018•
edited
Loading

Copy link
Copy Markdown
MemberAuthor

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

@addaleaxaddaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 16, 2018
mcollina
mcollina reviewed Feb 17, 2018
View reviewed changes

@mcollinamcollina 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.

Good work!

Comment threadlib/internal/streams/destroy.js Outdated

@mcollinamcollinaFeb 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.

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

@addaleaxaddaleaxFeb 19, 2018

Copy link
Copy Markdown
MemberAuthor

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

@mcollinamcollinaFeb 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.

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

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

addaleax commented Feb 20, 2018

Copy link
Copy Markdown
MemberAuthor

@mcollina Thanks for the pointer, done!

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

@addaleaxaddaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 20, 2018
mcollina
mcollina approved these changes Feb 21, 2018
View reviewed changes

@mcollinamcollina 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.

LGTM

@addaleax

addaleax commented Feb 21, 2018

Copy link
Copy Markdown
MemberAuthor

Landed in 906bbef

@addaleax
addaleax deleted the net-final branch February 21, 2018 19:28
@addaleaxaddaleax 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

lpinca commented Feb 21, 2018

Copy link
Copy Markdown
Member

@addaleax it seems this landed without metadata.

@addaleax

addaleax commented Feb 21, 2018

Copy link
Copy Markdown
MemberAuthor

@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
@addaleax
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
@addaleax
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
@addaleax@BridgeAR
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
@addaleax@MayaLekova
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
@addaleax
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>
@MylesBorinsMylesBorins mentioned this pull request Jul 9, 2018
Merged
rvagg pushed a commit that referenced this pull request Aug 16, 2018
@addaleax@rvagg
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>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mcollinamcollinamcollina approved these changes
@jasnelljasnelljasnell approved these changes
@lpincalpincalpinca approved these changes

Assignees

No one assigned

Labels

netIssues 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

@addaleax@BridgeAR@lpinca@mcollina@jasnell@nodejs-github-bot