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

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

Closed
wants to merge 2 commits into from
Closed

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

wants to merge 2 commits into from

Conversation

kumarrishav
Copy link
Contributor

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

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
Member

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

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

@kumarrishav
Copy link
Contributor Author

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

CC: @richardlau

@richardlau
Copy link
Member

@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

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

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

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

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@kumarrishav
Copy link
Contributor Author

can we go ahead with review and merge?

@kumarrishav
Copy link
Contributor Author

can we target this for v18.20.3 release?

@ratneshraval
Copy link

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

@kumarrishav
Copy link
Contributor Author

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

@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

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
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Linkgoron Linkgoron Linkgoron approved these changes

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.

None yet
5 participants
@kumarrishav @nodejs-github-bot @Linkgoron @richardlau @ratneshraval