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.

Sign up for GitHub

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: add support for finished after .destroy() #36635

Closed
wants to merge 1 commit into from
Closed

net: add support for finished after .destroy() #36635

wants to merge 1 commit into from

Conversation

ronag
Copy link
Member

@ronag ronag commented Dec 26, 2020 •
edited
Loading

Calling finished(socket, cb) would previously not
invoked the callback if the socket was already detroyed.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@ronag ronag added net Issues and PRs related to the net subsystem. stream Issues and PRs related to the stream subsystem. labels Dec 26, 2020
@ronag ronag requested review from mcollina and mafintosh December 26, 2020 13:42
@nodejs-github-bot nodejs-github-bot added the http Issues or PRs related to the http subsystem. label Dec 26, 2020
@ronag ronag requested a review from jasnell December 26, 2020 13:42
@ronag
Copy link
Member Author

ronag commented Dec 26, 2020

@nodejs/net @nodejs/streams

mcollina
mcollina approved these changes Dec 26, 2020
View reviewed changes
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

@ronag ronag added the request-ci Add this label to start a Jenkins CI on a PR. label Dec 26, 2020
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 26, 2020
@nodejs-github-bot
Copy link
Collaborator

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

@ronag ronag added the request-ci Add this label to start a Jenkins CI on a PR. label Dec 26, 2020
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 26, 2020
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Dec 26, 2020 •
edited by ronag
Loading

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

@ronag ronag added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 26, 2020
@ronag ronag added the commit-queue Add this label to land a pull request using GitHub Actions. label Dec 28, 2020
@github-actions github-actions bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Dec 28, 2020
@github-actions
Copy link
Contributor

Commit Queue failed
- Loading data for nodejs/node/pull/36635
✔  Done loading data for nodejs/node/pull/36635
----------------------------------- PR info ------------------------------------
Title      net: add support for finished after .destroy() (#36635)
Author     Robert Nagy  (@ronag)
Branch     ronag:net-finished -> nodejs:master
Labels     author ready, dont-land-on-v10.x, dont-land-on-v12.x, http, net, stream
Commits    4
 - net: add support for finished after .destroy()
 - fixup
 - Update lib/internal/streams/end-of-stream.js
 - Update test/parallel/test-net-finished.js
Committers 2
 - Robert Nagy 
 - GitHub 
PR-URL: https://github.com/nodejs/node/pull/36635
Reviewed-By: Matteo Collina 
Reviewed-By: Luigi Pinca 
Reviewed-By: Benjamin Gruenbaum 
Reviewed-By: Rich Trott 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/36635
Reviewed-By: Matteo Collina 
Reviewed-By: Luigi Pinca 
Reviewed-By: Benjamin Gruenbaum 
Reviewed-By: Rich Trott 
--------------------------------------------------------------------------------
   ✔  Last GitHub Actions successful
   ℹ  Last Full PR CI on 2020-12-26T16:03:40Z: https://ci.nodejs.org/job/node-test-pull-request/35087/
- Querying data for job/node-test-pull-request/35087/
✔  Build data downloaded
   ✔  Last Jenkins CI successful
   ℹ  This PR was created on Sat, 26 Dec 2020 13:42:02 GMT
   ✔  Approvals: 4
   ✔  - Matteo Collina (@mcollina) (TSC): https://github.com/nodejs/node/pull/36635#pullrequestreview-558878340
   ✔  - Luigi Pinca (@lpinca): https://github.com/nodejs/node/pull/36635#pullrequestreview-558882856
   ✔  - Benjamin Gruenbaum (@benjamingr): https://github.com/nodejs/node/pull/36635#pullrequestreview-558893180
   ✔  - Rich Trott (@Trott) (TSC): https://github.com/nodejs/node/pull/36635#pullrequestreview-558935403
   ✖  This PR needs to wait 7 more hours to land
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu
https://github.com/nodejs/node/actions/runs/448230565

@targos targos added commit-queue Add this label to land a pull request using GitHub Actions. and removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. commit-queue Add this label to land a pull request using GitHub Actions. labels Dec 28, 2020
@ronag ronag added the commit-queue Add this label to land a pull request using GitHub Actions. label Dec 30, 2020
@github-actions github-actions bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Dec 30, 2020
@github-actions
Copy link
Contributor

Commit Queue failed
- Loading data for nodejs/node/pull/36635
✔  Done loading data for nodejs/node/pull/36635
----------------------------------- PR info ------------------------------------
Title      net: add support for finished after .destroy() (#36635)
Author     Robert Nagy  (@ronag)
Branch     ronag:net-finished -> nodejs:master
Labels     author ready, dont-land-on-v10.x, dont-land-on-v12.x, http, net, stream
Commits    1
 - net: add support for finished after .destroy()
Committers 1
 - Robert Nagy 
PR-URL: https://github.com/nodejs/node/pull/36635
Reviewed-By: Matteo Collina 
Reviewed-By: Luigi Pinca 
Reviewed-By: Benjamin Gruenbaum 
Reviewed-By: Rich Trott 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/36635
Reviewed-By: Matteo Collina 
Reviewed-By: Luigi Pinca 
Reviewed-By: Benjamin Gruenbaum 
Reviewed-By: Rich Trott 
--------------------------------------------------------------------------------
   ⚠  Commits were pushed since the last review:
   ⚠  - net: add support for finished after .destroy()
   ✖  GitHub CI is still running
   ℹ  Last Full PR CI on 2020-12-30T15:28:27Z: https://ci.nodejs.org/job/node-test-pull-request/35087/
   ⚠  Commits were pushed after the last Full PR CI run:
   ⚠  - net: add support for finished after .destroy()
- Querying data for job/node-test-pull-request/35087/
✔  Build data downloaded
   ✔  Last Jenkins CI successful
   ℹ  This PR was created on Sat, 26 Dec 2020 13:42:02 GMT
   ✔  Approvals: 4
   ✔  - Matteo Collina (@mcollina) (TSC): https://github.com/nodejs/node/pull/36635#pullrequestreview-558878340
   ✔  - Luigi Pinca (@lpinca): https://github.com/nodejs/node/pull/36635#pullrequestreview-558882856
   ✔  - Benjamin Gruenbaum (@benjamingr): https://github.com/nodejs/node/pull/36635#pullrequestreview-558893180
   ✔  - Rich Trott (@Trott) (TSC): https://github.com/nodejs/node/pull/36635#pullrequestreview-558935403
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu
https://github.com/nodejs/node/actions/runs/453124569

@github-actions github-actions bot added the commit-queue-failed An error occurred while landing this pull request using GitHub Actions. label Dec 30, 2020
@ronag ronag added request-ci Add this label to start a Jenkins CI on a PR. and removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels Dec 30, 2020
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 30, 2020
@ronag ronag added the request-ci Add this label to start a Jenkins CI on a PR. label Dec 30, 2020
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 30, 2020
@ronag
Copy link
Member Author

ronag commented Dec 31, 2020

@Trott Would you mind help landing this? At the moment I am unable to land PR's and the bots don't seem to work.

@mcollina mcollina added the request-ci Add this label to start a Jenkins CI on a PR. label Dec 31, 2020
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 31, 2020
@ronag ronag added the request-ci Add this label to start a Jenkins CI on a PR. label Jan 2, 2021
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 2, 2021
@aduh95 aduh95 added the request-ci Add this label to start a Jenkins CI on a PR. label Jan 3, 2021
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 3, 2021
@nodejs-github-bot
Copy link
Collaborator

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

@jasnell
Copy link
Member

jasnell commented Jan 4, 2021

Landed in 2da3611

@jasnell jasnell closed this Jan 4, 2021
jasnell pushed a commit that referenced this pull request Jan 4, 2021
@ronag @jasnell
net: add support for finished after .destroy()
2da3611 
Calling `finished(socket, cb)` would previously not
invoked the callback if the socket was already detroyed.

PR-URL: #36635
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@MylesBorins
Copy link
Contributor

hey @ronag this doesn't land cleanly on v15.x. Would you be able to backport?

@ronag ronag self-assigned this Jan 5, 2021
@mcollina mcollina mentioned this pull request Mar 29, 2021
Closed
3 tasks
@MylesBorins
Copy link
Contributor

Adding don't land labels for all current branches as this was reverted from master

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RaisinTen RaisinTen RaisinTen left review comments

@lpinca lpinca lpinca approved these changes

@benjamingr benjamingr benjamingr approved these changes

@Trott Trott Trott approved these changes

@jasnell jasnell jasnell approved these changes

@mcollina mcollina mcollina approved these changes

@mafintosh mafintosh Awaiting requested review from mafintosh

Assignees

@ronag ronag

Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. http Issues or PRs related to the http subsystem. net Issues and PRs related to the net subsystem. stream Issues and PRs related to the stream subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.