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: check if socket is null before destroy #43796

Closed
wants to merge 1 commit into from
Closed

http: check if socket is null before destroy #43796

wants to merge 1 commit into from

Conversation

F3n67u
Copy link
Member

@F3n67u F3n67u commented Jul 12, 2022

Fix #43795

@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 Jul 12, 2022
@F3n67u F3n67u changed the title http: add socket null check http: check socket if is null before destory Jul 12, 2022
@F3n67u F3n67u changed the title http: check socket if is null before destory http: check if socket is null before destory Jul 12, 2022
@F3n67u F3n67u requested a review from ShogunPanda July 12, 2022 11:57
@mscdex
Copy link
Contributor

mscdex commented Jul 12, 2022

s/destory/destroy/ in commit message

mcollina
mcollina approved these changes Jul 12, 2022
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 Jul 12, 2022
@F3n67u
Copy link
Member Author

F3n67u commented Jul 12, 2022

s/destory/destroy/ in commit message

Fixed. Thank you.

@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jul 12, 2022
@nodejs-github-bot
Copy link
Collaborator

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

@mscdex mscdex changed the title http: check if socket is null before destory http: check if socket is null before destroy Jul 12, 2022
ShogunPanda
ShogunPanda approved these changes Jul 12, 2022
View reviewed changes
Copy link
Contributor

@ShogunPanda ShogunPanda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

lpinca
lpinca reviewed Jul 12, 2022
View reviewed changes
lib/_http_server.js
@@ -487,15 +487,15 @@ Server.prototype.closeAllConnections = function() {
const connections = this[kConnections].all();

for (let i = 0, l = connections.length; i < l; i++) {
connections[i].socket.destroy();
connections[i].socket?.destroy();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you explain how/when the socket is nulled?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's related to the flaky GC test I'm investigating (sorry, don't have a laptop to link stuff at the moment).

I noticed that the test instrumenting was setting a object to null at the end but I could never finish the investigation (on my list for Friday).

This was referenced Jul 13, 2022
Open
Open
@lpinca
Copy link
Member

lpinca commented Jul 24, 2022 •
edited
Loading

I think this is superseded by #43949. I'm adding the blocked label to prevent this from landing.

@lpinca lpinca added the blocked PRs that are blocked by other issues or PRs. label Jul 24, 2022
@F3n67u
Copy link
Member Author

F3n67u commented Jul 24, 2022

I think this is superseded by #43949. I'm adding the blocked label to prevent this from landing.

Thank you. I close this pr in favor of #43949. I am 90% sure that #43949 will address the problem this pr want to solve.

@F3n67u F3n67u closed this Jul 24, 2022
@F3n67u F3n67u deleted the closeIdleConnections branch July 24, 2022 05:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ShogunPanda ShogunPanda ShogunPanda approved these changes

@lpinca lpinca lpinca left review comments

@mcollina mcollina mcollina approved these changes

@BethGriggs BethGriggs BethGriggs approved these changes

@RafaelGSS RafaelGSS RafaelGSS approved these changes

Assignees
No one assigned
Labels
blocked PRs that are blocked by other issues or PRs. 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.

Server.closeIdleConnections break gc tests
8 participants
@F3n67u @nodejs-github-bot @mscdex @lpinca @mcollina @ShogunPanda @BethGriggs @RafaelGSS