FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

[v18.x] http: remove closeIdleConnections function while calling server close. by kumarrishav · Pull Request #52336 · nodejs/node · GitHub

/ node Public

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

Closed
kumarrishav wants to merge 2 commits into
nodejs:v18.x-stagingfrom
kumarrishav:patch-1
ConversationCommits (2)ChecksFiles changed
Closed

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

Conversation

kumarrishav commented Apr 2, 2024

Copy link
Copy Markdown
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 commented Apr 2, 2024

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/http
  • @nodejs/net

nodejs-github-bot added http Issues or PRs related to the http subsystem. needs-ci PRs that need a full CI run. v18.x labels Apr 2, 2024
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
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 mentioned this pull request Apr 2, 2024
Closed

Linkgoron commented Apr 2, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor

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

kumarrishav commented Apr 2, 2024

Copy link
Copy Markdown
Contributor Author

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

kumarrishav commented Apr 3, 2024

Copy link
Copy Markdown
Contributor Author

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

CC: @richardlau

richardlau commented Apr 3, 2024

Copy link
Copy Markdown
Member

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

CC: @richardlau

v18.x-staging.

kumarrishav changed the base branch from v18.x to v18.x-staging April 3, 2024 14:40

kumarrishav commented Apr 3, 2024

Copy link
Copy Markdown
Contributor Author

Done

richardlau added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 3, 2024
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 commented Apr 3, 2024

Copy link
Copy Markdown
Collaborator

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

richardlau mentioned this pull request Apr 3, 2024
Closed

nodejs-github-bot commented Apr 4, 2024

Copy link
Copy Markdown
Collaborator

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

nodejs-github-bot commented Apr 7, 2024

Copy link
Copy Markdown
Collaborator

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

nodejs-github-bot commented Apr 7, 2024

Copy link
Copy Markdown
Collaborator

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

nodejs-github-bot commented Apr 8, 2024

Copy link
Copy Markdown
Collaborator

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

nodejs-github-bot commented Apr 8, 2024

Copy link
Copy Markdown
Collaborator

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

kumarrishav commented Apr 8, 2024

Copy link
Copy Markdown
Contributor Author

can we go ahead with review and merge?

kumarrishav commented Apr 10, 2024

Copy link
Copy Markdown
Contributor Author

can we target this for v18.20.3 release?

ratneshraval commented Apr 10, 2024

Copy link
Copy Markdown

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

kumarrishav commented Apr 10, 2024

Copy link
Copy Markdown
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 commented Apr 15, 2024

Copy link
Copy Markdown
Contributor Author

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

richardlau pushed a commit that referenced this pull request Apr 17, 2024
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 commented Apr 17, 2024

Copy link
Copy Markdown
Member

Landed in 6689a98.

richardlau closed this Apr 17, 2024
richardlau mentioned this pull request May 16, 2024
Merged
JoMC98 mentioned this pull request Jul 19, 2024
Closed
liustack added a commit to liustack/pptwise that referenced this pull request Jul 25, 2026
fix(cli): carry S1 review fixes into the serve wave
4975411 
Four quick-adds the S1 review flagged for S2 to bring along:

- close(): explicit closeIdleConnections()/closeAllConnections() fallback,
  typeof-guarded for Node 18.0-18.18 (the methods land in 18.2.0,
  nodejs/node#42812) — Node's own close() briefly auto-invoked
  closeIdleConnections internally on some 18.x releases via a mistaken
  v19-only backport, reverted in 18.20.3 (nodejs/node#52336), so close()
  alone can't be assumed sufficient on any given patch.
- Extend the ENOENT watch-setup comment: a pages/or assets/ directory
  created after startup is never picked up until pptfast serve restarts.
- Two watch tests: a file appearing in assets/ triggers a rebuild, and a
  deck.spec.json-only edit (no pages/ touch) triggers one too — S1's own
  coverage only exercised pages/*.json.
- One-line comment at runPreview's moved mkdir(outDir) noting the
  deliberate after-render timing.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer
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.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

Back | FazBrowse Home | New Git URL