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: change totalSocketCount only on socket creation or when the socket is closed #40572

Merged
merged 1 commit into from
Nov 10, 2021
Merged

http: change totalSocketCount only on socket creation or when the socket is closed #40572

merged 1 commit into from
Nov 10, 2021

Conversation

jodevsa
Copy link
Contributor

@jodevsa jodevsa commented Oct 22, 2021 •
edited
Loading

@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 Oct 22, 2021
Trott
Trott reviewed Oct 22, 2021
View reviewed changes
Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

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

Can you please add a test?

@jodevsa jodevsa force-pushed the remove_socket_count_increment branch from a74c6c9 to d50c671 Compare October 23, 2021 00:20
@jodevsa
Copy link
Contributor Author

jodevsa commented Oct 23, 2021

@Trott
I added tests, but turns out it's not a bug :D I'll add more details in the issue, and see how to move forward. I think we have another bug, like the socketCount going down to -1 🙈

@jodevsa jodevsa changed the title http: do not increment socketCount on existing socket reuse http: add tests for http agent totalSocketCount Oct 23, 2021
@jodevsa jodevsa mentioned this pull request Oct 23, 2021
Closed
@jodevsa jodevsa force-pushed the remove_socket_count_increment branch from d50c671 to c907459 Compare October 23, 2021 08:07
@jodevsa jodevsa changed the title http: add tests for http agent totalSocketCount http: change totalSocketCount only on socket creation or when the socket is closed Oct 23, 2021
@jodevsa jodevsa force-pushed the remove_socket_count_increment branch 2 times, most recently from 56415f8 to 2116b6a Compare October 23, 2021 08:18
mcollina
mcollina approved these changes Oct 23, 2021
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

@jodevsa
Copy link
Contributor Author

jodevsa commented Oct 23, 2021

@mcollina is there a way to rerun the failed test?

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot

This comment has been minimized.

Sign in to view
@jodevsa jodevsa force-pushed the remove_socket_count_increment branch 2 times, most recently from aa0cced to 63d8bf7 Compare October 23, 2021 20:00
@jodevsa jodevsa requested a review from lpinca October 23, 2021 20:00
@jodevsa jodevsa force-pushed the remove_socket_count_increment branch from 72c7b65 to 09ab76b Compare October 23, 2021 20:18
@github-actions github-actions bot mentioned this pull request Oct 24, 2021
Open
31 tasks
@jodevsa jodevsa force-pushed the remove_socket_count_increment branch 2 times, most recently from 9b9bf8c to e3f9bdc Compare October 24, 2021 11:34
@jodevsa jodevsa requested a review from lpinca October 24, 2021 11:36
@Trott
Copy link
Member

Trott commented Nov 8, 2021

Current failures are #40738

Now that that has closed, I imagine a rebase against master and a force push to the PR branch would fix things.

@jodevsa
Copy link
Contributor Author

jodevsa commented Nov 8, 2021

@Trott I will try it out. Thanks for the update :)

@github-actions github-actions bot mentioned this pull request Nov 9, 2021
Open
23 tasks
@jodevsa
Copy link
Contributor Author

jodevsa commented Nov 9, 2021

@Trott 1 failed :(

@nodejs-github-bot

This comment has been minimized.

Sign in to view
mcollina
mcollina approved these changes Nov 9, 2021
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
Copy link
Member

mcollina commented Nov 9, 2021

@nodejs/build do you know why asan fails? Can we land anyway?

I'm trying to land this using "node-core-utils" and it is stuck at

Querying API for job/node-test-pull-request/40795/

@Trott
Copy link
Member

Trott commented Nov 9, 2021

@nodejs/build do you know why asan fails?

I don't know why ASAN fails frequently, but there's a Re-run Workflow button that I always click. I've re-run it here.

Can we land anyway?

Please don't. If it's a real failure, we want to know.

@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot
Copy link
Collaborator

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

@github-actions github-actions bot mentioned this pull request Nov 10, 2021
Open
26 tasks
@Trott Trott added the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 10, 2021
@jodevsa @Trott
http: change totalSocketCount only on socket creation/close
d8f1823 
PR-URL: nodejs#40572
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Voltrex <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
Reviewed-By: Ricky Zhou <[email protected]>
@Trott Trott force-pushed the remove_socket_count_increment branch from ef2c276 to d8f1823 Compare November 10, 2021 02:16
@Trott
Copy link
Member

Trott commented Nov 10, 2021

Landed in d8f1823

@Trott Trott merged commit d8f1823 into nodejs:master Nov 10, 2021
@Trott
Copy link
Member

Trott commented Nov 10, 2021

Thanks for the contribution! 🎉

And thanks for your patience and persistence!

This was referenced Nov 11, 2021
Open
Open
Open
Open
Open
targos pushed a commit that referenced this pull request Nov 21, 2021
@jodevsa @targos
http: change totalSocketCount only on socket creation/close
80ce97f 
PR-URL: #40572
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Voltrex <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
Reviewed-By: Ricky Zhou <[email protected]>
@targos targos mentioned this pull request Nov 26, 2021
Merged
danielleadams pushed a commit that referenced this pull request Jan 30, 2022
@jodevsa @danielleadams
http: change totalSocketCount only on socket creation/close
5a72f46 
PR-URL: #40572
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Voltrex <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
Reviewed-By: Ricky Zhou <[email protected]>
danielleadams pushed a commit that referenced this pull request Feb 1, 2022
@jodevsa @danielleadams
http: change totalSocketCount only on socket creation/close
be53296 
PR-URL: #40572
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Voltrex <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
Reviewed-By: Ricky Zhou <[email protected]>
@danielleadams danielleadams mentioned this pull request Feb 1, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Trott Trott Trott left review comments

@VoltrexKeyva VoltrexKeyva VoltrexKeyva approved these changes

@rickyes rickyes rickyes approved these changes

@JungMinu JungMinu JungMinu approved these changes

@lpinca lpinca lpinca approved these changes

@jasnell jasnell jasnell approved these changes

@mcollina mcollina mcollina approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue Add this label to land a pull request using GitHub Actions. http Issues or PRs related to the http subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Can someone take a look at this code in _http_agent.js there may be a bug.