Skip to content

[v18.x] http: remove closeIdleConnections function while calling server close. #52336

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
Closed
kumarrishav wants to merge 2 commits into from
Closed

[v18.x] http: remove closeIdleConnections function while calling server close. #52336

kumarrishav wants to merge 2 commits into from

Conversation

@kumarrishav
Copy link
Contributor

@kumarrishav kumarrishav commented Apr 2, 2024

Correcting the #50194 backporting mistake.
closeIdleConnections shouldnot be called while server.close in node v18. This behavior is for node v19 and above.

fixes: #52330, #51677

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Apr 2, 2024

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. v18.x Issues that can be reproduced on v18.x or PRs targeting the v18.x-staging branch. labels Apr 2, 2024
@kumarrishav kumarrishav changed the title http: remove closeIdleConnections function while calling server close. [v18.x] http: remove closeIdleConnections function while calling server close. Apr 2, 2024
@kumarrishav
http: remove closeIdleConnections function while calling server close
1a435a9 
Correcting the nodejs#50194 backporting mistake.
closeIdleConnections shouldnot be called while server.close in node v18.
This behavior is for node v19 and above.

fixes: nodejs#52330, nodejs#51677
@kumarrishav kumarrishav mentioned this pull request Apr 2, 2024
Closed
@Linkgoron
Copy link
Contributor

Linkgoron commented Apr 2, 2024 •
edited
Loading

the timeout tests should not be deleted, as they check the interval change which is still kept.

@kumarrishav
Copy link
Contributor Author

kumarrishav commented Apr 2, 2024

Correct. I doubted that too. I misunderstood the previous message regarding deleting test. @Linkgoron

@kumarrishav
Copy link
Contributor Author

kumarrishav commented Apr 3, 2024

@Linkgoron should i be raising the PR against v18.x or v18.x-staging branch?

CC: @richardlau

@richardlau
Copy link
Member

richardlau commented Apr 3, 2024

@Linkgoron should i be raising the PR against v18.x or v18.x-staging branch?

CC: @richardlau

v18.x-staging.

@kumarrishav kumarrishav changed the base branch from v18.x to v18.x-staging April 3, 2024 14:40
@kumarrishav
Copy link
Contributor Author

kumarrishav commented Apr 3, 2024

Done

@richardlau richardlau added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 3, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 3, 2024
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Apr 3, 2024

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

@richardlau richardlau mentioned this pull request Apr 3, 2024
Open
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Apr 4, 2024

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

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Apr 7, 2024

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

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Apr 7, 2024

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

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Apr 8, 2024

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

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Apr 8, 2024

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

@kumarrishav
Copy link
Contributor Author

kumarrishav commented Apr 8, 2024

can we go ahead with review and merge?

@kumarrishav
Copy link
Contributor Author

kumarrishav commented Apr 10, 2024

can we target this for v18.20.3 release?

@ratneshraval
Copy link

ratneshraval commented Apr 10, 2024

Will there be separate PR for v19/20/21 ? or does this propagate to newer versions?

@kumarrishav
Copy link
Contributor Author

kumarrishav commented Apr 10, 2024

Will there be separate PR for v19/20/21 ? or does this propagate to newer versions?

No. This new behavior is expected in v19 and above. https://nodejs.org/docs/latest-v19.x/api/http.html#serverclosecallback

This PR is only for v18 where this is not expected and was a backporting mistake. https://nodejs.org/docs/latest-v18.x/api/http.html#serverclosecallback

@kumarrishav
Copy link
Contributor Author

kumarrishav commented Apr 15, 2024

@richardlau can we get this merge and land on v18?

richardlau pushed a commit that referenced this pull request Apr 17, 2024
@kumarrishav @richardlau
http: remove closeIdleConnections function while calling server close
6689a98 
Correcting the #50194 backporting mistake.
closeIdleConnections shouldnot be called while server.close in node v18.
This behavior is for node v19 and above.

Fixes: #52330
Fixes: #51677
PR-URL: #52336
Refs: #50194
Reviewed-By: Nitzan Uziely <linkgoron@gmail.com>
@richardlau
Copy link
Member

richardlau commented Apr 17, 2024

Landed in 6689a98.

@richardlau richardlau closed this Apr 17, 2024
@richardlau richardlau mentioned this pull request May 16, 2024
Merged
@JoMC98 JoMC98 mentioned this pull request Jul 19, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@Linkgoron Linkgoron Linkgoron approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

http Issues or PRs related to the http subsystem. needs-ci PRs that need a full CI run. v18.x Issues that can be reproduced on v18.x or PRs targeting the v18.x-staging branch.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@kumarrishav @nodejs-github-bot @Linkgoron @richardlau @ratneshraval