Skip to content

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

Closed
kumarrishav wants to merge 2 commits intonodejs:v18.x-stagingfrom
kumarrishav:patch-1
Closed

[v18.x] http: remove closeIdleConnections function while calling server close.#52336
kumarrishav wants to merge 2 commits intonodejs:v18.x-stagingfrom
kumarrishav:patch-1

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