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

http: start connections checking interval on listen #48611

Merged
merged 1 commit into from
Jul 24, 2023
Merged

http: start connections checking interval on listen #48611

merged 1 commit into from
Jul 24, 2023

Conversation

ShogunPanda
Copy link
Contributor

This PR fixes a memory leak by checking HTTP connections only if the server is listening.

Fixes: #48604.

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/http
  • @nodejs/net

@nodejs-github-bot nodejs-github-bot added http Issues or PRs related to the http subsystem. needs-ci PRs that need a full CI run. labels Jun 30, 2023
mcollina
mcollina approved these changes Jun 30, 2023
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

@mcollina mcollina added the request-ci Add this label to start a Jenkins CI on a PR. label Jun 30, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jun 30, 2023
@nodejs-github-bot
Copy link
Collaborator

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

mscdex
mscdex reviewed Jun 30, 2023
View reviewed changes
lib/_http_server.js Outdated Show resolved Hide resolved
mscdex
mscdex reviewed Jun 30, 2023
View reviewed changes
lib/_http_server.js Outdated Show resolved Hide resolved
lpinca
lpinca reviewed Jun 30, 2023
View reviewed changes
lib/_http_server.js Outdated Show resolved Hide resolved
lib/_http_server.js Outdated Show resolved Hide resolved
lib/_http_server.js Outdated Show resolved Hide resolved
lib/_http_server.js Outdated Show resolved Hide resolved
test/parallel/test-http-server-connections-checking-leak.js Show resolved Hide resolved
This was referenced Jul 1, 2023
Open
Open
Open
@ShogunPanda ShogunPanda force-pushed the http-server-check-on-listening branch from 7b23c64 to 94b4764 Compare July 3, 2023 07:33
@ShogunPanda
Copy link
Contributor Author

@lpinca I reworked tests using Countdown. I noticed setInterval was unnecessary.
What about now?

@ShogunPanda ShogunPanda force-pushed the http-server-check-on-listening branch from 3929991 to e86f329 Compare July 3, 2023 11:00
@ShogunPanda ShogunPanda force-pushed the http-server-check-on-listening branch from e86f329 to bbf9f88 Compare July 3, 2023 14:00
@github-actions github-actions bot mentioned this pull request Jul 4, 2023
Open
22 tasks
@ShogunPanda ShogunPanda added the request-ci Add this label to start a Jenkins CI on a PR. label Jul 4, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jul 4, 2023
@nodejs-github-bot
Copy link
Collaborator

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

@mcollina mcollina mentioned this pull request Jul 4, 2023
Merged
@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@ShogunPanda ShogunPanda added commit-queue Add this label to land a pull request using GitHub Actions. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. labels Jul 24, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jul 24, 2023
@nodejs-github-bot nodejs-github-bot merged commit 8490318 into nodejs:main Jul 24, 2023
30 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in 8490318

@ShogunPanda ShogunPanda deleted the http-server-check-on-listening branch July 24, 2023 21:26
This was referenced Jul 25, 2023
Open
Open
Open
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Jul 27, 2023
@ShogunPanda @lpinca @Ceres6
http: start connections checking interval on listen
1650c76 
Co-authored-by: Luigi Pinca <luigipinca@gmail.com>
PR-URL: nodejs#48611
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
This was referenced Jul 28, 2023
Open
Open
Open
Open
pluris pushed a commit to pluris/node that referenced this pull request Aug 6, 2023
@ShogunPanda @lpinca
http: start connections checking interval on listen
4b934c5 
Co-authored-by: Luigi Pinca <luigipinca@gmail.com>
PR-URL: nodejs#48611
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
pluris pushed a commit to pluris/node that referenced this pull request Aug 7, 2023
@ShogunPanda @lpinca
http: start connections checking interval on listen
78591a3 
Co-authored-by: Luigi Pinca <luigipinca@gmail.com>
PR-URL: nodejs#48611
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
@ShogunPanda @lpinca @Ceres6
http: start connections checking interval on listen
a47a904 
Co-authored-by: Luigi Pinca <luigipinca@gmail.com>
PR-URL: nodejs#48611
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
@ShogunPanda @lpinca @Ceres6
http: start connections checking interval on listen
cd76484 
Co-authored-by: Luigi Pinca <luigipinca@gmail.com>
PR-URL: nodejs#48611
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
UlisesGascon pushed a commit to UlisesGascon/node that referenced this pull request Aug 14, 2023
@ShogunPanda @lpinca @UlisesGascon
http: start connections checking interval on listen
be547b3 
Co-authored-by: Luigi Pinca <luigipinca@gmail.com>
PR-URL: nodejs#48611
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
RafaelGSS pushed a commit that referenced this pull request Aug 15, 2023
@ShogunPanda @lpinca @RafaelGSS
http: start connections checking interval on listen
222ce81 
Co-authored-by: Luigi Pinca <luigipinca@gmail.com>
PR-URL: #48611
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
@UlisesGascon UlisesGascon mentioned this pull request Aug 15, 2023
Merged
RafaelGSS pushed a commit that referenced this pull request Aug 17, 2023
@ShogunPanda @lpinca @RafaelGSS
http: start connections checking interval on listen
742597b 
Co-authored-by: Luigi Pinca <luigipinca@gmail.com>
PR-URL: #48611
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
@mcollina mcollina mentioned this pull request Sep 11, 2023
Open
targos pushed a commit that referenced this pull request Nov 27, 2023
@ShogunPanda @lpinca @targos
http: start connections checking interval on listen
8874b2e 
Co-authored-by: Luigi Pinca <luigipinca@gmail.com>
PR-URL: #48611
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
@targos targos mentioned this pull request Nov 28, 2023
Merged
sercher added a commit to sercher/graaljs that referenced this pull request Apr 25, 2024
@sercher @lpinca
http: start connections checking interval on listen
b46a0f0 
Co-authored-by: Luigi Pinca <luigipinca@gmail.com>
PR-URL: nodejs/node#48611
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
sercher added a commit to sercher/graaljs that referenced this pull request Apr 25, 2024
@sercher @lpinca
http: start connections checking interval on listen
1147592 
Co-authored-by: Luigi Pinca <luigipinca@gmail.com>
PR-URL: nodejs/node#48611
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@theanarkh theanarkh theanarkh left review comments

@mscdex mscdex mscdex left review comments

@marco-ippolito marco-ippolito marco-ippolito approved these changes

@RafaelGSS RafaelGSS RafaelGSS approved these changes

@lpinca lpinca lpinca approved these changes

@mcollina mcollina mcollina approved these changes

Assignees
No one assigned
Labels
commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. http Issues or PRs related to the http subsystem. needs-ci PRs that need a full CI run.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

http: memory leak if a server is not closed
9 participants
@ShogunPanda @nodejs-github-bot @StefanStojanovic @lpinca @mcollina @mscdex @theanarkh @RafaelGSS @marco-ippolito